Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Apache>
  3. Dev
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
christophe.jaillet at wanadoo
Aug 7, 2021, 12:00 AM
Post #1 of 8 (439 views)
Permalink
Hi,

Le 06/08/2021 à 15:10, covener@apache.org a écrit :
> Author: covener
> Date: Fri Aug 6 13:10:45 2021
> New Revision: 1892038
>
> URL: http://svn.apache.org/viewvc?rev=1892038&view=rev
> Log:
> fix int overflow in ap_timeout_parameter_parse
>
> signed integer overflow in ap_timeout_parameter_parse under fuzzing
>
> Modified:
> httpd/httpd/trunk/server/util.c
>
> Modified: httpd/httpd/trunk/server/util.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1892038&r1=1892037&r2=1892038&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/util.c (original)
> +++ httpd/httpd/trunk/server/util.c Fri Aug 6 13:10:45 2021
> @@ -2676,6 +2676,7 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> char *endp;
> const char *time_str;
> apr_int64_t tout;
> + apr_uint64_t check;
>
> tout = apr_strtoi64(timeout_parameter, &endp, 10);
> if (errno) {
> @@ -2688,16 +2689,20 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> time_str = endp;
> }
>
> + if (tout < 0) {
> + return APR_ERANGE;
> + }
> +
> switch (*time_str) {
> /* Time is in seconds */
> case 's':
> case 'S':
> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout);
> + check = apr_time_from_sec(tout);
> break;
> case 'h':
> case 'H':
> /* Time is in hours */
> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 3600);
> + check = apr_time_from_sec(tout * 3600);
> break;
> case 'm':
> case 'M':
> @@ -2705,12 +2710,12 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> /* Time is in milliseconds */
> case 's':
> case 'S':
> - *timeout = (apr_interval_time_t) tout * 1000;
> + check = tout * 1000;
> break;
> /* Time is in minutes */
> case 'i':
> case 'I':
> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 60);
> + check = apr_time_from_sec(tout * 60);
> break;
> default:
> return APR_EGENERAL;
> @@ -2719,6 +2724,10 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> default:
> return APR_EGENERAL;
> }
> + if (check > APR_INT64_MAX || check < 0) {

why "|| check < 0"?
'check' is unsigned (i.e. apr_uint64_t).

If paranoiac, and want to prevent overflow or wrap around after the
multiplication, I think that we should compute the maximum acceptable
value pour each case ('s', 'h', ...) and test against it.

CJ
> + return APR_ERANGE;
> + }
> + *timeout = (apr_interval_time_t) check;
> return APR_SUCCESS;
> }
>
>
>
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c [ In reply to ]
covener at gmail
Aug 7, 2021, 3:45 AM
Post #2 of 8 (439 views)
Permalink
On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Hi,
>
> Le 06/08/2021 à 15:10, covener@apache.org a écrit :
> > Author: covener
> > Date: Fri Aug 6 13:10:45 2021
> > New Revision: 1892038
> >
> > URL: http://svn.apache.org/viewvc?rev=1892038&view=rev
> > Log:
> > fix int overflow in ap_timeout_parameter_parse
> >
> > signed integer overflow in ap_timeout_parameter_parse under fuzzing
> >
> > Modified:
> > httpd/httpd/trunk/server/util.c
> >
> > Modified: httpd/httpd/trunk/server/util.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1892038&r1=1892037&r2=1892038&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/server/util.c (original)
> > +++ httpd/httpd/trunk/server/util.c Fri Aug 6 13:10:45 2021
> > @@ -2676,6 +2676,7 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > char *endp;
> > const char *time_str;
> > apr_int64_t tout;
> > + apr_uint64_t check;
> >
> > tout = apr_strtoi64(timeout_parameter, &endp, 10);
> > if (errno) {
> > @@ -2688,16 +2689,20 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > time_str = endp;
> > }
> >
> > + if (tout < 0) {
> > + return APR_ERANGE;
> > + }
> > +
> > switch (*time_str) {
> > /* Time is in seconds */
> > case 's':
> > case 'S':
> > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout);
> > + check = apr_time_from_sec(tout);
> > break;
> > case 'h':
> > case 'H':
> > /* Time is in hours */
> > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 3600);
> > + check = apr_time_from_sec(tout * 3600);
> > break;
> > case 'm':
> > case 'M':
> > @@ -2705,12 +2710,12 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > /* Time is in milliseconds */
> > case 's':
> > case 'S':
> > - *timeout = (apr_interval_time_t) tout * 1000;
> > + check = tout * 1000;
> > break;
> > /* Time is in minutes */
> > case 'i':
> > case 'I':
> > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 60);
> > + check = apr_time_from_sec(tout * 60);
> > break;
> > default:
> > return APR_EGENERAL;
> > @@ -2719,6 +2724,10 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > default:
> > return APR_EGENERAL;
> > }
> > + if (check > APR_INT64_MAX || check < 0) {
>
> why "|| check < 0"?
> 'check' is unsigned (i.e. apr_uint64_t).
>

ty, changed in 1892063.
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c [ In reply to ]
covener at gmail
Aug 7, 2021, 11:14 AM
Post #3 of 8 (439 views)
Permalink
Seems like the fuzzer got farther but is still failing


util.c:2713:26: runtime error: signed integer overflow:
9999999999999999 * 1000 cannot be represented in type 'long'
#0 0x4be247 in ap_timeout_parameter_parse /src/httpd/server/util.c:2713:26

case 'M':
switch (*(++time_str)) {
/* Time is in milliseconds */
case 's':
case 'S':
check = tout * 1000;
break;



Does this require a cast?

On Sat, Aug 7, 2021 at 6:45 AM Eric Covener <covener@gmail.com> wrote:
>
> On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > Hi,
> >
> > Le 06/08/2021 à 15:10, covener@apache.org a écrit :
> > > Author: covener
> > > Date: Fri Aug 6 13:10:45 2021
> > > New Revision: 1892038
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1892038&view=rev
> > > Log:
> > > fix int overflow in ap_timeout_parameter_parse
> > >
> > > signed integer overflow in ap_timeout_parameter_parse under fuzzing
> > >
> > > Modified:
> > > httpd/httpd/trunk/server/util.c
> > >
> > > Modified: httpd/httpd/trunk/server/util.c
> > > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1892038&r1=1892037&r2=1892038&view=diff
> > > ==============================================================================
> > > --- httpd/httpd/trunk/server/util.c (original)
> > > +++ httpd/httpd/trunk/server/util.c Fri Aug 6 13:10:45 2021
> > > @@ -2676,6 +2676,7 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > char *endp;
> > > const char *time_str;
> > > apr_int64_t tout;
> > > + apr_uint64_t check;
> > >
> > > tout = apr_strtoi64(timeout_parameter, &endp, 10);
> > > if (errno) {
> > > @@ -2688,16 +2689,20 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > time_str = endp;
> > > }
> > >
> > > + if (tout < 0) {
> > > + return APR_ERANGE;
> > > + }
> > > +
> > > switch (*time_str) {
> > > /* Time is in seconds */
> > > case 's':
> > > case 'S':
> > > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout);
> > > + check = apr_time_from_sec(tout);
> > > break;
> > > case 'h':
> > > case 'H':
> > > /* Time is in hours */
> > > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 3600);
> > > + check = apr_time_from_sec(tout * 3600);
> > > break;
> > > case 'm':
> > > case 'M':
> > > @@ -2705,12 +2710,12 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > /* Time is in milliseconds */
> > > case 's':
> > > case 'S':
> > > - *timeout = (apr_interval_time_t) tout * 1000;
> > > + check = tout * 1000;
> > > break;
> > > /* Time is in minutes */
> > > case 'i':
> > > case 'I':
> > > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 60);
> > > + check = apr_time_from_sec(tout * 60);
> > > break;
> > > default:
> > > return APR_EGENERAL;
> > > @@ -2719,6 +2724,10 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > default:
> > > return APR_EGENERAL;
> > > }
> > > + if (check > APR_INT64_MAX || check < 0) {
> >
> > why "|| check < 0"?
> > 'check' is unsigned (i.e. apr_uint64_t).
> >
>
> ty, changed in 1892063.



--
Eric Covener
covener@gmail.com
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c [ In reply to ]
covener at gmail
Aug 7, 2021, 11:33 AM
Post #4 of 8 (439 views)
Permalink
Maybe making the constants unsigned?

On Sat, Aug 7, 2021 at 2:14 PM Eric Covener <covener@gmail.com> wrote:
>
> Seems like the fuzzer got farther but is still failing
>
>
> util.c:2713:26: runtime error: signed integer overflow:
> 9999999999999999 * 1000 cannot be represented in type 'long'
> #0 0x4be247 in ap_timeout_parameter_parse /src/httpd/server/util.c:2713:26
>
> case 'M':
> switch (*(++time_str)) {
> /* Time is in milliseconds */
> case 's':
> case 'S':
> check = tout * 1000;
> break;
>
>
>
> Does this require a cast?
>
> On Sat, Aug 7, 2021 at 6:45 AM Eric Covener <covener@gmail.com> wrote:
> >
> > On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET
> > <christophe.jaillet@wanadoo.fr> wrote:
> > >
> > > Hi,
> > >
> > > Le 06/08/2021 à 15:10, covener@apache.org a écrit :
> > > > Author: covener
> > > > Date: Fri Aug 6 13:10:45 2021
> > > > New Revision: 1892038
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1892038&view=rev
> > > > Log:
> > > > fix int overflow in ap_timeout_parameter_parse
> > > >
> > > > signed integer overflow in ap_timeout_parameter_parse under fuzzing
> > > >
> > > > Modified:
> > > > httpd/httpd/trunk/server/util.c
> > > >
> > > > Modified: httpd/httpd/trunk/server/util.c
> > > > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1892038&r1=1892037&r2=1892038&view=diff
> > > > ==============================================================================
> > > > --- httpd/httpd/trunk/server/util.c (original)
> > > > +++ httpd/httpd/trunk/server/util.c Fri Aug 6 13:10:45 2021
> > > > @@ -2676,6 +2676,7 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > > char *endp;
> > > > const char *time_str;
> > > > apr_int64_t tout;
> > > > + apr_uint64_t check;
> > > >
> > > > tout = apr_strtoi64(timeout_parameter, &endp, 10);
> > > > if (errno) {
> > > > @@ -2688,16 +2689,20 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > > time_str = endp;
> > > > }
> > > >
> > > > + if (tout < 0) {
> > > > + return APR_ERANGE;
> > > > + }
> > > > +
> > > > switch (*time_str) {
> > > > /* Time is in seconds */
> > > > case 's':
> > > > case 'S':
> > > > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout);
> > > > + check = apr_time_from_sec(tout);
> > > > break;
> > > > case 'h':
> > > > case 'H':
> > > > /* Time is in hours */
> > > > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 3600);
> > > > + check = apr_time_from_sec(tout * 3600);
> > > > break;
> > > > case 'm':
> > > > case 'M':
> > > > @@ -2705,12 +2710,12 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > > /* Time is in milliseconds */
> > > > case 's':
> > > > case 'S':
> > > > - *timeout = (apr_interval_time_t) tout * 1000;
> > > > + check = tout * 1000;
> > > > break;
> > > > /* Time is in minutes */
> > > > case 'i':
> > > > case 'I':
> > > > - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 60);
> > > > + check = apr_time_from_sec(tout * 60);
> > > > break;
> > > > default:
> > > > return APR_EGENERAL;
> > > > @@ -2719,6 +2724,10 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > > default:
> > > > return APR_EGENERAL;
> > > > }
> > > > + if (check > APR_INT64_MAX || check < 0) {
> > >
> > > why "|| check < 0"?
> > > 'check' is unsigned (i.e. apr_uint64_t).
> > >
> >
> > ty, changed in 1892063.
>
>
>
> --
> Eric Covener
> covener@gmail.com



--
Eric Covener
covener@gmail.com
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c [ In reply to ]
christophe.jaillet at wanadoo
Aug 8, 2021, 2:03 PM
Post #5 of 8 (439 views)
Permalink
Hi,

attached, my proposal. (compile tested only, but you'll get the idea)
It applies on top of current trunk (so after 1892038,1892063).

I'm not sure that the "(check > APR_INT64_MAX || check < tout)" check in
your patches is enough to catch overflows.

CJ

Le 07/08/2021 à 20:33, Eric Covener a écrit :
> Maybe making the constants unsigned?
>
> On Sat, Aug 7, 2021 at 2:14 PM Eric Covener <covener@gmail.com> wrote:
>>
>> Seems like the fuzzer got farther but is still failing
>>
>>
>> util.c:2713:26: runtime error: signed integer overflow:
>> 9999999999999999 * 1000 cannot be represented in type 'long'
>> #0 0x4be247 in ap_timeout_parameter_parse /src/httpd/server/util.c:2713:26
>>
>> case 'M':
>> switch (*(++time_str)) {
>> /* Time is in milliseconds */
>> case 's':
>> case 'S':
>> check = tout * 1000;
>> break;
>>
>>
>>
>> Does this require a cast?
>>
>> On Sat, Aug 7, 2021 at 6:45 AM Eric Covener <covener@gmail.com> wrote:
>>>
>>> On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET
>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Le 06/08/2021 à 15:10, covener@apache.org a écrit :
>>>>> Author: covener
>>>>> Date: Fri Aug 6 13:10:45 2021
>>>>> New Revision: 1892038
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1892038&view=rev
>>>>> Log:
>>>>> fix int overflow in ap_timeout_parameter_parse
>>>>>
>>>>> signed integer overflow in ap_timeout_parameter_parse under fuzzing
>>>>>
>>>>> Modified:
>>>>> httpd/httpd/trunk/server/util.c
>>>>>
>>>>> Modified: httpd/httpd/trunk/server/util.c
>>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1892038&r1=1892037&r2=1892038&view=diff
>>>>> ==============================================================================
>>>>> --- httpd/httpd/trunk/server/util.c (original)
>>>>> +++ httpd/httpd/trunk/server/util.c Fri Aug 6 13:10:45 2021
>>>>> @@ -2676,6 +2676,7 @@ AP_DECLARE(apr_status_t) ap_timeout_para
>>>>> char *endp;
>>>>> const char *time_str;
>>>>> apr_int64_t tout;
>>>>> + apr_uint64_t check;
>>>>>
>>>>> tout = apr_strtoi64(timeout_parameter, &endp, 10);
>>>>> if (errno) {
>>>>> @@ -2688,16 +2689,20 @@ AP_DECLARE(apr_status_t) ap_timeout_para
>>>>> time_str = endp;
>>>>> }
>>>>>
>>>>> + if (tout < 0) {
>>>>> + return APR_ERANGE;
>>>>> + }
>>>>> +
>>>>> switch (*time_str) {
>>>>> /* Time is in seconds */
>>>>> case 's':
>>>>> case 'S':
>>>>> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout);
>>>>> + check = apr_time_from_sec(tout);
>>>>> break;
>>>>> case 'h':
>>>>> case 'H':
>>>>> /* Time is in hours */
>>>>> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 3600);
>>>>> + check = apr_time_from_sec(tout * 3600);
>>>>> break;
>>>>> case 'm':
>>>>> case 'M':
>>>>> @@ -2705,12 +2710,12 @@ AP_DECLARE(apr_status_t) ap_timeout_para
>>>>> /* Time is in milliseconds */
>>>>> case 's':
>>>>> case 'S':
>>>>> - *timeout = (apr_interval_time_t) tout * 1000;
>>>>> + check = tout * 1000;
>>>>> break;
>>>>> /* Time is in minutes */
>>>>> case 'i':
>>>>> case 'I':
>>>>> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 60);
>>>>> + check = apr_time_from_sec(tout * 60);
>>>>> break;
>>>>> default:
>>>>> return APR_EGENERAL;
>>>>> @@ -2719,6 +2724,10 @@ AP_DECLARE(apr_status_t) ap_timeout_para
>>>>> default:
>>>>> return APR_EGENERAL;
>>>>> }
>>>>> + if (check > APR_INT64_MAX || check < 0) {
>>>>
>>>> why "|| check < 0"?
>>>> 'check' is unsigned (i.e. apr_uint64_t).
>>>>
>>>
>>> ty, changed in 1892063.
>>
>>
>>
>> --
>> Eric Covener
>> covener@gmail.com
>
>
>

Attached Files:

Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c [ In reply to ]
covener at gmail
Aug 8, 2021, 2:28 PM
Post #6 of 8 (438 views)
Permalink
I finally got unlazy and got a local build w/ the UBISAN checking and
your patch works!

env CC=clang LD=clang CFLAGS=-fsanitize=undefined ./config.nice

Can you commit to trunk?

On Sun, Aug 8, 2021 at 5:03 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Hi,
>
> attached, my proposal. (compile tested only, but you'll get the idea)
> It applies on top of current trunk (so after 1892038,1892063).
>
> I'm not sure that the "(check > APR_INT64_MAX || check < tout)" check in
> your patches is enough to catch overflows.
>
> CJ
>
> Le 07/08/2021 à 20:33, Eric Covener a écrit :
> > Maybe making the constants unsigned?
> >
> > On Sat, Aug 7, 2021 at 2:14 PM Eric Covener <covener@gmail.com> wrote:
> >>
> >> Seems like the fuzzer got farther but is still failing
> >>
> >>
> >> util.c:2713:26: runtime error: signed integer overflow:
> >> 9999999999999999 * 1000 cannot be represented in type 'long'
> >> #0 0x4be247 in ap_timeout_parameter_parse /src/httpd/server/util.c:2713:26
> >>
> >> case 'M':
> >> switch (*(++time_str)) {
> >> /* Time is in milliseconds */
> >> case 's':
> >> case 'S':
> >> check = tout * 1000;
> >> break;
> >>
> >>
> >>
> >> Does this require a cast?
> >>
> >> On Sat, Aug 7, 2021 at 6:45 AM Eric Covener <covener@gmail.com> wrote:
> >>>
> >>> On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET
> >>> <christophe.jaillet@wanadoo.fr> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> Le 06/08/2021 à 15:10, covener@apache.org a écrit :
> >>>>> Author: covener
> >>>>> Date: Fri Aug 6 13:10:45 2021
> >>>>> New Revision: 1892038
> >>>>>
> >>>>> URL: http://svn.apache.org/viewvc?rev=1892038&view=rev
> >>>>> Log:
> >>>>> fix int overflow in ap_timeout_parameter_parse
> >>>>>
> >>>>> signed integer overflow in ap_timeout_parameter_parse under fuzzing
> >>>>>
> >>>>> Modified:
> >>>>> httpd/httpd/trunk/server/util.c
> >>>>>
> >>>>> Modified: httpd/httpd/trunk/server/util.c
> >>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1892038&r1=1892037&r2=1892038&view=diff
> >>>>> ==============================================================================
> >>>>> --- httpd/httpd/trunk/server/util.c (original)
> >>>>> +++ httpd/httpd/trunk/server/util.c Fri Aug 6 13:10:45 2021
> >>>>> @@ -2676,6 +2676,7 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> >>>>> char *endp;
> >>>>> const char *time_str;
> >>>>> apr_int64_t tout;
> >>>>> + apr_uint64_t check;
> >>>>>
> >>>>> tout = apr_strtoi64(timeout_parameter, &endp, 10);
> >>>>> if (errno) {
> >>>>> @@ -2688,16 +2689,20 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> >>>>> time_str = endp;
> >>>>> }
> >>>>>
> >>>>> + if (tout < 0) {
> >>>>> + return APR_ERANGE;
> >>>>> + }
> >>>>> +
> >>>>> switch (*time_str) {
> >>>>> /* Time is in seconds */
> >>>>> case 's':
> >>>>> case 'S':
> >>>>> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout);
> >>>>> + check = apr_time_from_sec(tout);
> >>>>> break;
> >>>>> case 'h':
> >>>>> case 'H':
> >>>>> /* Time is in hours */
> >>>>> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 3600);
> >>>>> + check = apr_time_from_sec(tout * 3600);
> >>>>> break;
> >>>>> case 'm':
> >>>>> case 'M':
> >>>>> @@ -2705,12 +2710,12 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> >>>>> /* Time is in milliseconds */
> >>>>> case 's':
> >>>>> case 'S':
> >>>>> - *timeout = (apr_interval_time_t) tout * 1000;
> >>>>> + check = tout * 1000;
> >>>>> break;
> >>>>> /* Time is in minutes */
> >>>>> case 'i':
> >>>>> case 'I':
> >>>>> - *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 60);
> >>>>> + check = apr_time_from_sec(tout * 60);
> >>>>> break;
> >>>>> default:
> >>>>> return APR_EGENERAL;
> >>>>> @@ -2719,6 +2724,10 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> >>>>> default:
> >>>>> return APR_EGENERAL;
> >>>>> }
> >>>>> + if (check > APR_INT64_MAX || check < 0) {
> >>>>
> >>>> why "|| check < 0"?
> >>>> 'check' is unsigned (i.e. apr_uint64_t).
> >>>>
> >>>
> >>> ty, changed in 1892063.
> >>
> >>
> >>
> >> --
> >> Eric Covener
> >> covener@gmail.com
> >
> >
> >
>


--
Eric Covener
covener@gmail.com
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c [ In reply to ]
christophe.jaillet at wanadoo
Aug 10, 2021, 11:52 AM
Post #7 of 8 (435 views)
Permalink
Le 08/08/2021 à 23:28, Eric Covener a écrit :
> I finally got unlazy and got a local build w/ the UBISAN checking and
> your patch works!
>
> env CC=clang LD=clang CFLAGS=-fsanitize=undefined ./config.nice
>
> Can you commit to trunk?

Now tested on my side as well and pushed as r1892185.

CJ



>
> On Sun, Aug 8, 2021 at 5:03 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> Hi,
>>
>> attached, my proposal. (compile tested only, but you'll get the idea)
>> It applies on top of current trunk (so after 1892038,1892063).
>>
>> I'm not sure that the "(check > APR_INT64_MAX || check < tout)" check in
>> your patches is enough to catch overflows.
>>
>> CJ
>>
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c [ In reply to ]
covener at gmail
Aug 10, 2021, 11:55 AM
Post #8 of 8 (435 views)
Permalink
On Tue, Aug 10, 2021 at 2:52 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 08/08/2021 à 23:28, Eric Covener a écrit :
> > I finally got unlazy and got a local build w/ the UBISAN checking and
> > your patch works!
> >
> > env CC=clang LD=clang CFLAGS=-fsanitize=undefined ./config.nice
> >
> > Can you commit to trunk?
>
> Now tested on my side as well and pushed as r1892185.
>
> CJ

ty, just proposed.

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal