Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 16, 2016, 10:06 UTC
- Message-ID
- <c99474f2-dac3-e42a-5e4a-02464cac3982@web.de>
- In-Reply-To
- <20161015171325.k2jggjezfmhk3tz7@sigill.intra.peff.net>
Am 15.10.2016 um 19:13 schrieb Jeff King:
Show 11 quoted lines
> 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.
Show 10 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.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.
Show 6 quoted lines
>> } 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