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 +0000 +++ 
> apr_brigade.c 2023-12-14 21:10:20.477289754 +0000 @@ -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;
>