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.
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.
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?
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 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.
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?
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | [EMAIL PROTECTED]
---------------------------------------------------------------------------
If at first you don't succeed, skydiving's out!
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
