httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Niklas Edmundsson <ni...@acc.umu.se>
Subject Re: httpd 2.2.8 segfaults
Date Sat, 23 Feb 2008 08:54:59 GMT
On Fri, 22 Feb 2008, Plüm, Rüdiger, VF-Group wrote:

>>>> In general, that patch looks truly suspicious since it seems to me
>>>> it's typecasting wildly and not even using its newly invented
>>>> MAX_APR_SIZE_T in all places, because (apr_size_t)(-1)
>> really is the
>>>> same thing, right?
>>>
>>> No, MAX_APR_SIZE_T and (apr_size_t)(-1) might be different
>> depending on the
>>> platform. MAX_APR_SIZE_T is ~(apr_size_t)(0).
>>
>> Won't both be 0xff...ff as long as apr_size_t is unsigned (which it
>> should be)? If not, the code makes even less sense..
>
> I thought the same so far. But there seem to be platforms that we support
> where this is not the case. Don't ask which platforms these are. Somebody?

size_t must be unsigned for (apr_size_t)(-1) to work, or this code 
will be rather bogus IMHO. Comparing the length to signed -1 doesn't 
seem really productive...

>> Both casting signed -1 to unsigned and flipping the bits of 0 are
>> standard methods to get the max-value possible to store in a
>> variable...
>>
>>> As I have overcome my confusion regarding apr_off_t / apr_size_t I
>>> hope to have a look into the problem and find a solution how to do
>>> all the casting stuff correctly.
>>
>> My tip would be: less casts. If they're needed they're usually a sign
>> of bad design or a thinko somewhere.
>

> Meanwhile I tried to clean this up in trunk. Can you please try the 
> attached patch?
>
> Keep in mind that MAX_APR_SIZE_T is not present in apr-util 1.2.x 
> and that you need to adjust this manually. Remote eyes welcome as 
> well.

I'm still not liking the casts and the mixed -1's, APR_SIZE_MAX and 
MAX_APR_SIZE_T...

In any case, I'll be busy for most of this weekend so I probably won't 
have time to try patches until monday...

> Index: apr_brigade.c
> ===================================================================
> --- apr_brigade.c       (revision 630122)
> +++ apr_brigade.c       (working copy)
> @@ -97,6 +97,7 @@
>     apr_bucket *e;
>     const char *s;
>     apr_size_t len;
> +    apr_uint64_t point64;
>     apr_status_t rv;
>
>     if (point < 0) {
> @@ -108,17 +109,25 @@
>         return APR_SUCCESS;
>     }
>
> +    /*
> +     * Try to reduce the following casting mess: We know that point will be
> +     * larger equal 0 now and forever and thus that point (apr_off_t) and
> +     * apr_size_t will fit into apr_uint64_t in any case.
> +     */
> +    point64 = (apr_uint64_t)point;
> +
>     APR_BRIGADE_CHECK_CONSISTENCY(b);
>
>     for (e = APR_BRIGADE_FIRST(b);
>          e != APR_BRIGADE_SENTINEL(b);
>          e = APR_BUCKET_NEXT(e))
>     {
> -        /* For an unknown length bucket, while 'point' is beyond the possible
> +        /* For an unknown length bucket, while 'point64' is beyond the possible
>          * size contained in apr_size_t, read and continue...
>          */
> -        if ((e->length == (apr_size_t)(-1)) && (point > APR_SIZE_MAX))
{
> -            /* point is too far out to simply split this bucket,
> +        if ((e->length == (apr_size_t)(-1))
> +            && (point64 > (apr_uint64_t)APR_SIZE_MAX)) {
> +            /* point64 is too far out to simply split this bucket,
>              * we must fix this bucket's size and keep going... */
>             rv = apr_bucket_read(e, &s, &len, APR_BLOCK_READ);
>             if (rv != APR_SUCCESS) {
> @@ -126,14 +135,15 @@
>                 return rv;
>             }
>         }
> -        else if (((apr_size_t)point < e->length) || (e->length == (apr_size_t)(-1)))
{
> -            /* We already consumed buckets where point is beyond
> +        else if ((point64 < (apr_uint64_t)e->length)
> +                 || (e->length == (apr_size_t)(-1))) {
> +            /* We already consumed buckets where point64 is beyond
>              * our interest ( point > MAX_APR_SIZE_T ), above.
> -             * Here point falls between 0 and MAX_APR_SIZE_T
> +             * Here point falls between 0 and MAX_APR_SIZE_T
>              * and is within this bucket, or this bucket's len
>              * is undefined, so now we are ready to split it.
>              * First try to split the bucket natively... */
> -            if ((rv = apr_bucket_split(e, (apr_size_t)point))
> +            if ((rv = apr_bucket_split(e, (apr_size_t)point64))
>                     != APR_ENOTIMPL) {
>                 *after_point = APR_BUCKET_NEXT(e);
>                 return rv;
> @@ -150,17 +160,17 @@
>             /* this assumes that len == e->length, which is okay because e
>              * might have been morphed by the apr_bucket_read() above, but
>              * if it was, the length would have been adjusted appropriately */
> -            if ((apr_size_t)point < e->length) {
> +            if (point64 < (apr_uint64_t)e->length) {
>                 rv = apr_bucket_split(e, (apr_size_t)point);
>                 *after_point = APR_BUCKET_NEXT(e);
>                 return rv;
>             }
>         }
> -        if (point == e->length) {
> +        if (point64 == (apr_uint64_t)e->length) {
>             *after_point = APR_BUCKET_NEXT(e);
>             return APR_SUCCESS;
>         }
> -        point -= e->length;
> +        point64 -= (apr_uint64_t)e->length;
>     }
>     *after_point = APR_BRIGADE_SENTINEL(b);
>     return APR_INCOMPLETE;
>
>
> Regards
>
> Rüdiger
>
>


/Nikke
-- 
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
  Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se      |     nikke@acc.umu.se
---------------------------------------------------------------------------
  Captain, I sense millions of minds focused on my cleavage.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=

Mime
View raw message