[PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM

[PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM

[René Scharfe]

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.

NB: The cast to a (non-const) void pointer is necessary to keep support
for flexible array members declared as const.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
This patch allows the test suite to largely pass (t7063 still fails)
for clang 3.8 with -fsanitize=undefined and -DNO_UNALIGNED_LOADS.

 git-compat-util.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 43718da..f964e36 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -851,8 +851,9 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
  * times, and it must be assignable as an lvalue.
  */
 #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_); \
 } while (0)
 #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
 	(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
-- 
2.10.1

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

[Jeff King]

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

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

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

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

[René Scharfe]

Am 15.10.2016 um 19:13 schrieb Jeff King:
> On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:
> 
>> 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.

The only way I see is to pass the type to the macro explicitly (because
typeof is an extention), and that would make call sites ugly.

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

Right; we could drop that feature of the original macros and require
users to pass length expressions that don't have side effects -- all of
them already do that anyway.  But let's keep it in this round; it just
costs one extra line.

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

-- >8 --
Subject: [PATCH 2/1] inline xalloc_flex() into FLEXPTR_ALLOC_MEM

Allocate and copy directly in FLEXPTR_ALLOC_MEM and remove the now
unused helper function xalloc_flex().  The resulting code is shorter
and the offset arithmetic is a bit simpler.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f964e36..49ca28c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -856,7 +856,9 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
 	memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
 } while (0)
 #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
-	(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
+	size_t flex_array_len_ = (len); \
+	(x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
+	memcpy((x) + 1, (buf), flex_array_len_); \
 	(x)->ptrname = (void *)((x)+1); \
 } while(0)
 #define FLEX_ALLOC_STR(x, flexname, str) \
@@ -864,14 +866,6 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
 #define FLEXPTR_ALLOC_STR(x, ptrname, str) \
 	FLEXPTR_ALLOC_MEM((x), ptrname, (str), strlen(str))
 
-static inline void *xalloc_flex(size_t base_len, size_t offset,
-				const void *src, size_t src_len)
-{
-	unsigned char *ret = xcalloc(1, st_add3(base_len, src_len, 1));
-	memcpy(ret + offset, src, src_len);
-	return ret;
-}
-
 static inline char *xstrdup_or_null(const char *str)
 {
 	return str ? xstrdup(str) : NULL;
-- 
2.10.1

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

[Jeff King]

On Sun, Oct 16, 2016 at 12:06:02PM +0200, René Scharfe wrote:

> > 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.
> 
> The only way I see is to pass the type to the macro explicitly (because
> typeof is an extention), and that would make call sites ugly.

Yep, exactly. I really wish we could use typeof(); there are a couple
things that could be made less error-prone and ugly. But it's not even a
matter of C99; it's a GNU-ism, so I don't think we are even close to
having support for it everywhere.

> > 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.
> 
> Right; we could drop that feature of the original macros and require
> users to pass length expressions that don't have side effects -- all of
> them already do that anyway.  But let's keep it in this round; it just
> costs one extra line.

Oh, I think it's worth keeping forever, if only because it makes the
macro interface have one fewer gotcha. Spending a line of code on that
is OK with me.

> > Now that xalloc_flex() has only this one caller remaining, perhaps it
> > should just be inlined here, too, for simplicity.
> 
> -- >8 --
> Subject: [PATCH 2/1] inline xalloc_flex() into FLEXPTR_ALLOC_MEM
> 
> Allocate and copy directly in FLEXPTR_ALLOC_MEM and remove the now
> unused helper function xalloc_flex().  The resulting code is shorter
> and the offset arithmetic is a bit simpler.

Looks good. Thanks for following up.

-Peff