Re: [PATCH] Avoid memcpy-from-NULL in apr_brigade_flatten

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.

Regards

Rüdiger