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
