apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: httpd 2.2.8 segfaults
Date Sun, 24 Feb 2008 15:02:31 GMT


On 02/24/2008 03:16 PM, Niklas Edmundsson wrote:
> On Sat, 23 Feb 2008, Ruediger Pluem wrote:
> 
>>> 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...
>>
>> Thats fine. Looking forward to your test results.
> 
> It seems to work after fixing the APR_SIZE_MAX/MAX_APR_SIZE_T thing, 
> this is (still) httpd-2.2.8 on Ubuntu 32bit LFS-enabled.

Just for clarification: The crashes are gone with this patch?
Then I would commit to apr-util trunk.

> 
> Compiling with optimization and all warnings (-O3 -W -Wall) I only get 
> these, which are not related to apr_brigade_partition:
> buckets/apr_brigade.c: In function 'apr_brigade_to_iovec':
> buckets/apr_brigade.c:359: warning: dereferencing type-punned pointer 
> will break strict-aliasing rules
> buckets/apr_brigade.c: In function 'apr_brigade_vprintf':
> buckets/apr_brigade.c:681: warning: comparison between signed and unsigned
> 
> The warning in apr_brigade_vprintf is trivial to fix, it should be 'int 
> written' instead of 'apr_size_t written' since that's what 
> apr_vformatter seems to return.

Thanks. Fixed on apr-util trunk in r630625.

> 
> Also, I get the point of (apr_size_t)(-1) now since it's the documented 
> "unknown bucket length" indicator. Ugly, but effective.
> 
> However, it seems to me that this leaves a hole for off-by-one 
> opportunities because on 32bit:
> 
> (apr_size_t)(-1) is 0xffffffff
> MAX_APR_SIZE_T is also 0xffffffff
> 
> This means that a bucket can hold max 0xfffffffe bytes right?

Hm. Yes and no, but a bucket of size 0xffffffff cannot be distinguished
from a bucket of unknown length which can may lead to "interesting"
code paths.

> 
> Without checking too much I would guess that passing 0xffffffff as 
> "point" argument to apr_brigade_partition could fall between the cracks 
> since the comparisons with MAX_APR_SIZE_T are all < or > ...

 From a quick checking I would say that apr_brigade_partition still works
as designed even in his edge case.

> 
>> From a deobfuscating view to avoid future bugs I would suggest:
> 1) Create proper defines for use with the bucket length, ie.
>    MAX_BUCKET_LEN and BUCKET_LEN_UNKNOWN or something. It's much
>    easier to read and keep track of.

Seems to be a valid point, but because of APR's versioning rules this
can be only done on trunk. We need to keep in mind that (apr_size_t)(-1)
is used in many places in apr-util / httpd. So it will be some tedious
work to get this replaced but I guess it's worth it.

> 
> 2) Wouldn't it make more sense of having apr_brigade_partition() being
>    a little more careful like apr_brigade_insert_file() and creating
>    buckets of at most MAX_BUCKET_SIZE bytes?

Effectively apr_brigade_partition does not create buckets that are larger
than the ones that were supplied as apr_bucket_split only creates buckets
of the same length or smaller buckets. So I do not see the point for an
additional "nanny" behaviour in apr_brigade_partition here. This should
be fixed by the caller.

Regards

RĂ¼diger


Mime
View raw message