Re: [PATCH] Avoid memcpy-from-NULL in apr_brigade_flatten
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
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
> 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
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; >