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

2023-12-19 Thread Joe Orton
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



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

2023-12-18 Thread Ruediger Pluem



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



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

2023-12-18 Thread Ben Kallus
> 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()?

Sure thing. It's an EOS bucket.

> 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.

It might not be worth complicating the operation. I would think that
when someone wants to flatten a bucket brigade, they want the empty
buckets to be ignored, which is the current behavior with unoptimized
clang and gcc. You would know better than me, though :)

-Ben


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

2023-12-18 Thread Joe Orton
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.

Regards, Joe

> 
> --- apr_brigade-old.c 2023-12-14 21:12:48.616409321 + +++ 
> apr_brigade.c 2023-12-14 21:10:20.477289754 + @@ -278,7 +278,9 @@
>   *
>   * No, we only copy the data up to their requested size.  -- jre
>   */
> -memcpy(c, str, str_len);
> + if (str_len > 0) {
> +memcpy(c, str, str_len);
> + }
> 
>  c += str_len;
>  actual += str_len;
>