On Mon, Dec 18, 2023 at 04:09:36PM +0100, Ruediger Pluem wrote: > > > On 12/18/23 12:09 PM, Joe Orton wrote: > > On Thu, Dec 14, 2023 at 04:44:47PM -0500, Ben Kallus wrote: > >> memcpy to or from NULL is undefined behavior, even for copies of > >> length 0. See this godbolt link for an example of how this can cause > >> problems: https://gcc.godbolt.org/z/zfvnMMsds > >> > >> This patch avoids calling memcpy for 0-length buckets, so that buckets > >> with NULL data and 0 length don't cause UB when flattened. > >> > >> Addresses this bugzilla report from httpd: > >> https://bz.apache.org/bugzilla/show_bug.cgi?id=68278 > > > > Thanks for the patch, this is an interesting find. Can you say what > > bucket type was hit here - e.g. print b->type->name in gdb from inside > > apr_brigade_flatten()? > > > > It looks like metadata bucket types tend to give NULL on read() so I'm > > guessing that's the case here. It would trip up other apr_brigade_* > > functions in similar ways, e.g. split_line looks wrong too. You can make > > a case that ignoring FLUSH is "safe" but it's probably undesirable, and > > trying to flatten across an EOS is almost certainly wrong. > > > > So I'm not sure, maybe a better/safer response is to catch metadata > > buckets and treat them as end-of-brigade or an error rather than > > zero-length data buckets. apr_brigade_split_boundary has an API > > condition for that to explicitly return APR_INCOMPLETE. > > Also for the sake of compatibility I would only fail if we encounter > at non metadata bucket after a possible EOS bucket. > Any metadata before EOS I would just ignore when flattening.
I looked at this but it seems like the lowest risk will be to ignore the metadata buckets as in Ben's patch. There is no other place in APR which has special handling for EOS, and looking through httpd apr_brigade_flatten() is widely used with the input filters. So: no behaviour change, just fix possible crashes? I filed a PR - https://github.com/apache/apr/pull/52 Regards, Joe
