Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM

From: Jeff King <peff@peff.net>
Date: Oct 15, 2016, 17:13 UTC
Message-ID: <20161015171325.k2jggjezfmhk3tz7@sigill.intra.peff.net>
In-Reply-To: <ccb15072-d949-fc84-ee45-45ba013f53c4@web.de>

On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:

Show 5 quoted lines

> Calculating offsets involving a NULL pointer is undefined.  It works in
> practice (for now?), but we should not rely on it.  Allocate first and
> then simply refer to the flexible array member by its name instead of
> performing pointer arithmetic up front.  The resulting code is slightly
> shorter, easier to read and doesn't rely on undefined behaviour.

Yeah, this NULL computation is pretty nasty. I recall trying to get rid of it, but I think it is impossible to do so portably while still using the generic xalloc_flex() helper. I'm not sure why I didn't think of just inlining it as you do here. It's not that many lines of code, and I I agree the result is conceptually simpler.

Show 6 quoted lines

>  #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
> -	(x) = NULL; /* silence -Wuninitialized for offset calculation */ \
> -	(x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \
> +	size_t flex_array_len_ = (len); \
> +	(x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
> +	memcpy((void *)(x)->flexname, (buf), flex_array_len_); \

This looks correct. I wondered at first why you bothered with flex_array_len, but it is to avoid evaluating the "len" parameter multiple times.

>  } while (0)
>  #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
>  	(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \

Now that xalloc_flex() has only this one caller remaining, perhaps it should just be inlined here, too, for simplicity.

-Peff