You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Stas Bekman <st...@stason.org> on 2002/01/28 18:44:32 UTC
apr_date's functions corrupt the caller's data.
apr_date's apr_date_parse_rfc function modifies the date string in the
caller's namespace and corrupts it. I didn't test all cases but only the
accepted formats, for the .h docs:
# before : parse_rfc: Sun, 06 Nov 94 08:49 GMT
# after : parse_rfc: Sun, 06 Nov 94 08:49 00T
# before : parse_rfc: Sun, 6 Nov 94 08:49 GMT
# after : parse_rfc: Sun, 6 Nov 94 08:49 00T
# before : parse_rfc: Sun, 06 Nov 94 8:49:37 GMT
# after : parse_rfc: Sun, 06 Nov 9408:49:37 GMT
# before : parse_rfc: Sun, 6 Nov 94 8:49:37 GMT
# after : parse_rfc: Sun, 6 Nov 9408:49:37 GMT
that happens in all cases similar to this:
timstr[6] = '0';
This code also could corrupt the data:
if ((date = strchr(date, ' ')) == NULL) /* Find space after
weekday */
return APR_DATE_BAD;
this one happens in _http version as well.
I've tried walking through the code with gdb, but it looks like code
gets optimized in a weird way, so I see completely bogus values of date
while stepping through it.
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: apr_date's functions corrupt the caller's data.
Posted by Stas Bekman <st...@stason.org>.
Stas Bekman wrote:
> Justin Erenkrantz wrote:
>
>> On Tue, Jan 29, 2002 at 12:45:16PM +0800, Stas Bekman wrote:
>>
>>> yes, but the documentation doesn't note this. Does this mean that any
>>> API that doesn't specify const, is free to change the data without
>>> saying so?
>>>
>>
>> In theory, yes. But, it *should* be noted in the documentation as
>> well. Please feel free to submit a patch to fix that. However,
>> the real thing to do is to make it so that it doesn't need to
>> modify the char* or make it copy when it sees the odd formats.
ok, it's tomorrow in my place :)
Do you think we still need to modify the docs, if now the function
doesn't modify its date arg?
Index: srclib/apr-util/misc/apr_date.c
===================================================================
RCS file: /home/cvspublic/apr-util/misc/apr_date.c,v
retrieving revision 1.8
diff -u -r1.8 apr_date.c
--- srclib/apr-util/misc/apr_date.c 19 Jan 2002 12:40:03 -0000 1.8
+++ srclib/apr-util/misc/apr_date.c 29 Jan 2002 17:31:51 -0000
@@ -349,6 +349,7 @@
apr_time_t result;
int mint, mon;
char *monstr, *timstr, *gmtstr;
+ char dummy[8];
static const int months[12] =
{
('J' << 16) | ('a' << 8) | 'n', ('F' << 16) | ('e' << 8) | 'b',
@@ -473,7 +474,9 @@
ds.tm_mday = ((date[0] - '0') * 10) + (date[1] - '0');
monstr = date + 3;
- timstr = date + 10;
+ strncpy(dummy, date + 10, 8);
+ timstr = dummy;
+
timstr[6] = '0';
timstr[7] = '0';
gmtstr = NULL;
@@ -488,7 +491,8 @@
ds.tm_mday = (date[0] - '0');
monstr = date + 2;
- timstr = date + 9;
+ strncpy(dummy, date + 9, 8);
+ timstr = dummy;
timstr[6] = '0';
timstr[7] = '0';
@@ -504,7 +508,8 @@
ds.tm_mday = ((date[0] - '0') * 10) + (date[1] - '0');
monstr = date + 3;
- timstr = date + 9;
+ strncpy(dummy, date + 9, 8);
+ timstr = dummy;
timstr[0] = '0';
gmtstr = date + 18;
@@ -519,7 +524,8 @@
ds.tm_mday = (date[0] - '0');
monstr = date + 2;
- timstr = date + 8;
+ strncpy(dummy, date + 8, 8);
+ timstr = dummy;
timstr[0] = '0';
gmtstr = date + 17;
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: apr_date's functions corrupt the caller's data.
Posted by Stas Bekman <st...@stason.org>.
Justin Erenkrantz wrote:
> On Tue, Jan 29, 2002 at 12:45:16PM +0800, Stas Bekman wrote:
>
>>yes, but the documentation doesn't note this. Does this mean that any
>>API that doesn't specify const, is free to change the data without
>>saying so?
>>
>
> In theory, yes. But, it *should* be noted in the documentation as
> well. Please feel free to submit a patch to fix that. However,
> the real thing to do is to make it so that it doesn't need to
> modify the char* or make it copy when it sees the odd formats.
ok, I'll try tomorrow.
>>Also I think the docs are wrong. the functions return the number of
>>seconds and not micro-seconds:
>>
>> * @return the apr_time_t number of microseconds since 1 Jan 1970 GMT, or
>> * 0 if this would be out of range or if the date is invalid.
>>
>
> It returns usecs not seconds. None of the apr functions use
> seconds. I'm not sure how you see that it returns seconds.
scratch that, it works correctly. I was testing with the perl interface
and it automatically converts these into seconds on type conversion.
Sorry for a false alarm.
>>>>This code also could corrupt the data:
>>>>
>>>> if ((date = strchr(date, ' ')) == NULL) /* Find space after
>>>>weekday */
>>>> return APR_DATE_BAD;
>>>>
>>>>this one happens in _http version as well.
>>>>
>>>>
>>>Do you have any thoughts on how to resolve this? We are assuming
>>>that we're working with a valid string. It'll stop at a term
>>>NULL. If you pass in a non-NULL terminated string, too bad, IMHO.
>>>
>>
>>I dunno, I think that the moment you want to modify a string you have to
>>copy it.
>>
>
> This has nothing to do with modifying the string. We want to
> change the local pointer to point *after* the space. This eases the
> later compuations. I think the strchr is safe in this context.
true again, the optimized code got me confused.
>>I build with --enable-maintainer-mode, which enables '-g -O2 ...'.
>>Could it be a problem of -O2?
>>
>
> Yes. I compile with "-g -Wall" - O2 turns on optimizations and
> makes stuff like this awful to debug. Compiling without that O2
> should make the gdb debugging easier. -- justin
Will try to fix things and send the patch soon.
Thanks Justin.
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: apr_date's functions corrupt the caller's data.
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Tue, Jan 29, 2002 at 12:45:16PM +0800, Stas Bekman wrote:
> yes, but the documentation doesn't note this. Does this mean that any
> API that doesn't specify const, is free to change the data without
> saying so?
In theory, yes. But, it *should* be noted in the documentation as
well. Please feel free to submit a patch to fix that. However,
the real thing to do is to make it so that it doesn't need to
modify the char* or make it copy when it sees the odd formats.
> Also I think the docs are wrong. the functions return the number of
> seconds and not micro-seconds:
>
> * @return the apr_time_t number of microseconds since 1 Jan 1970 GMT, or
> * 0 if this would be out of range or if the date is invalid.
It returns usecs not seconds. None of the apr functions use
seconds. I'm not sure how you see that it returns seconds.
> >>This code also could corrupt the data:
> >>
> >> if ((date = strchr(date, ' ')) == NULL) /* Find space after
> >>weekday */
> >> return APR_DATE_BAD;
> >>
> >>this one happens in _http version as well.
> >>
> >
> >Do you have any thoughts on how to resolve this? We are assuming
> >that we're working with a valid string. It'll stop at a term
> >NULL. If you pass in a non-NULL terminated string, too bad, IMHO.
>
>
> I dunno, I think that the moment you want to modify a string you have to
> copy it.
This has nothing to do with modifying the string. We want to
change the local pointer to point *after* the space. This eases the
later compuations. I think the strchr is safe in this context.
> I build with --enable-maintainer-mode, which enables '-g -O2 ...'.
> Could it be a problem of -O2?
Yes. I compile with "-g -Wall" - O2 turns on optimizations and
makes stuff like this awful to debug. Compiling without that O2
should make the gdb debugging easier. -- justin
Re: apr_date's functions corrupt the caller's data.
Posted by Stas Bekman <st...@stason.org>.
Justin Erenkrantz wrote:
> On Tue, Jan 29, 2002 at 01:44:32AM +0800, Stas Bekman wrote:
>
>>apr_date's apr_date_parse_rfc function modifies the date string in the
>>caller's namespace and corrupts it. I didn't test all cases but only the
>>accepted formats, for the .h docs:
>>
>
> That's why you are also passing in a char* not a const char*. I
> didn't want to rewrite all of the date code to handle the oddball
> dates - I figured it'd be easier to note that the char* could be
> changed rather than incur a copy.
yes, but the documentation doesn't note this. Does this mean that any
API that doesn't specify const, is free to change the data without
saying so?
Also I think the docs are wrong. the functions return the number of
seconds and not micro-seconds:
* @return the apr_time_t number of microseconds since 1 Jan 1970 GMT, or
* 0 if this would be out of range or if the date is invalid.
> Feel free to submit a patch that makes it const char* (probably only
> need to copy the date if it's an oddball). The only place that is
> using this code is mod_mbox, but we could do some cleanups so that
> others can use it with a const char* expectation.
yeah, I started to work on it, but then something weird happens when I
try to debug it.
>>This code also could corrupt the data:
>>
>> if ((date = strchr(date, ' ')) == NULL) /* Find space after
>>weekday */
>> return APR_DATE_BAD;
>>
>>this one happens in _http version as well.
>>
>
> Do you have any thoughts on how to resolve this? We are assuming
> that we're working with a valid string. It'll stop at a term
> NULL. If you pass in a non-NULL terminated string, too bad, IMHO.
I dunno, I think that the moment you want to modify a string you have to
copy it.
>>I've tried walking through the code with gdb, but it looks like code
>>gets optimized in a weird way, so I see completely bogus values of date
>>while stepping through it.
>>
>
> Make sure you are using -g - this code can be extremely optimized
> by the compiler, so stepping through with gcc's optimiziations on
> can cause headaches.
I build with --enable-maintainer-mode, which enables '-g -O2 ...'.
Could it be a problem of -O2?
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: apr_date's functions corrupt the caller's data.
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Tue, Jan 29, 2002 at 01:44:32AM +0800, Stas Bekman wrote:
> apr_date's apr_date_parse_rfc function modifies the date string in the
> caller's namespace and corrupts it. I didn't test all cases but only the
> accepted formats, for the .h docs:
That's why you are also passing in a char* not a const char*. I
didn't want to rewrite all of the date code to handle the oddball
dates - I figured it'd be easier to note that the char* could be
changed rather than incur a copy.
Feel free to submit a patch that makes it const char* (probably only
need to copy the date if it's an oddball). The only place that is
using this code is mod_mbox, but we could do some cleanups so that
others can use it with a const char* expectation.
> This code also could corrupt the data:
>
> if ((date = strchr(date, ' ')) == NULL) /* Find space after
> weekday */
> return APR_DATE_BAD;
>
> this one happens in _http version as well.
Do you have any thoughts on how to resolve this? We are assuming
that we're working with a valid string. It'll stop at a term
NULL. If you pass in a non-NULL terminated string, too bad, IMHO.
> I've tried walking through the code with gdb, but it looks like code
> gets optimized in a weird way, so I see completely bogus values of date
> while stepping through it.
Make sure you are using -g - this code can be extremely optimized
by the compiler, so stepping through with gcc's optimiziations on
can cause headaches. -- justin