{"thread":{"id":"44305","subject":"Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM","startedAt":"2016-10-16T00:34:17Z","lastAt":"2016-10-16T19:46:15Z","messageCount":4,"participants":["Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"304211","messageId":"20161015171325.k2jggjezfmhk3tz7@sigill.intra.peff.net","threadId":"44305","inReplyTo":"ccb15072-d949-fc84-ee45-45ba013f53c4@web.de","subject":"Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-15T17:13:25Z","receivedAt":"2016-10-16T00:34:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:\n\n> Calculating offsets involving a NULL pointer is undefined.  It works in\n> practice (for now?), but we should not rely on it.  Allocate first and\n> then simply refer to the flexible array member by its name instead of\n> performing pointer arithmetic up front.  The resulting code is slightly\n> shorter, easier to read and doesn't rely on undefined behaviour.\n\nYeah, this NULL computation is pretty nasty. I recall trying to get rid\nof it, but I think it is impossible to do so portably while still using\nthe generic xalloc_flex() helper. I'm not sure why I didn't think of\njust inlining it as you do here. It's not that many lines of code, and I\nI agree the result is conceptually simpler.\n\n>  #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \\\n> -\t(x) = NULL; /* silence -Wuninitialized for offset calculation */ \\\n> -\t(x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \\\n> +\tsize_t flex_array_len_ = (len); \\\n> +\t(x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \\\n> +\tmemcpy((void *)(x)->flexname, (buf), flex_array_len_); \\\n\nThis looks correct. I wondered at first why you bothered with\nflex_array_len, but it is to avoid evaluating the \"len\" parameter\nmultiple times.\n\n>  } while (0)\n>  #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \\\n>  \t(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \\\n\nNow that xalloc_flex() has only this one caller remaining, perhaps it\nshould just be inlined here, too, for simplicity.\n\n-Peff\n"},{"id":"304218","messageId":"ccb15072-d949-fc84-ee45-45ba013f53c4@web.de","threadId":"44305","inReplyTo":null,"subject":"[PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-15T16:23:11Z","receivedAt":"2016-10-16T00:34:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Calculating offsets involving a NULL pointer is undefined.  It works in\npractice (for now?), but we should not rely on it.  Allocate first and\nthen simply refer to the flexible array member by its name instead of\nperforming pointer arithmetic up front.  The resulting code is slightly\nshorter, easier to read and doesn't rely on undefined behaviour.\n\nNB: The cast to a (non-const) void pointer is necessary to keep support\nfor flexible array members declared as const.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nThis patch allows the test suite to largely pass (t7063 still fails)\nfor clang 3.8 with -fsanitize=undefined and -DNO_UNALIGNED_LOADS.\n\n git-compat-util.h | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 43718da..f964e36 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -851,8 +851,9 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n  * times, and it must be assignable as an lvalue.\n  */\n #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \\\n-\t(x) = NULL; /* silence -Wuninitialized for offset calculation */ \\\n-\t(x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \\\n+\tsize_t flex_array_len_ = (len); \\\n+\t(x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \\\n+\tmemcpy((void *)(x)->flexname, (buf), flex_array_len_); \\\n } while (0)\n #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \\\n \t(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \\\n-- \n2.10.1\n\n"},{"id":"304226","messageId":"c99474f2-dac3-e42a-5e4a-02464cac3982@web.de","threadId":"44305","inReplyTo":"20161015171325.k2jggjezfmhk3tz7@sigill.intra.peff.net","subject":"Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-16T10:06:02Z","receivedAt":"2016-10-16T10:06:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.10.2016 um 19:13 schrieb Jeff King:\n> On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:\n> \n>> Calculating offsets involving a NULL pointer is undefined.  It works in\n>> practice (for now?), but we should not rely on it.  Allocate first and\n>> then simply refer to the flexible array member by its name instead of\n>> performing pointer arithmetic up front.  The resulting code is slightly\n>> shorter, easier to read and doesn't rely on undefined behaviour.\n> \n> Yeah, this NULL computation is pretty nasty. I recall trying to get rid\n> of it, but I think it is impossible to do so portably while still using\n> the generic xalloc_flex() helper.\n\nThe only way I see is to pass the type to the macro explicitly (because\ntypeof is an extention), and that would make call sites ugly.\n\n>>  #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \\\n>> -\t(x) = NULL; /* silence -Wuninitialized for offset calculation */ \\\n>> -\t(x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \\\n>> +\tsize_t flex_array_len_ = (len); \\\n>> +\t(x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \\\n>> +\tmemcpy((void *)(x)->flexname, (buf), flex_array_len_); \\\n> \n> This looks correct. I wondered at first why you bothered with\n> flex_array_len, but it is to avoid evaluating the \"len\" parameter\n> multiple times.\n\nRight; we could drop that feature of the original macros and require\nusers to pass length expressions that don't have side effects -- all of\nthem already do that anyway.  But let's keep it in this round; it just\ncosts one extra line.\n\n>>  } while (0)\n>>  #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \\\n>>  \t(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \\\n> \n> Now that xalloc_flex() has only this one caller remaining, perhaps it\n> should just be inlined here, too, for simplicity.\n\n-- >8 --\nSubject: [PATCH 2/1] inline xalloc_flex() into FLEXPTR_ALLOC_MEM\n\nAllocate and copy directly in FLEXPTR_ALLOC_MEM and remove the now\nunused helper function xalloc_flex().  The resulting code is shorter\nand the offset arithmetic is a bit simpler.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n git-compat-util.h | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f964e36..49ca28c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -856,7 +856,9 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n \tmemcpy((void *)(x)->flexname, (buf), flex_array_len_); \\\n } while (0)\n #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \\\n-\t(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \\\n+\tsize_t flex_array_len_ = (len); \\\n+\t(x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \\\n+\tmemcpy((x) + 1, (buf), flex_array_len_); \\\n \t(x)->ptrname = (void *)((x)+1); \\\n } while(0)\n #define FLEX_ALLOC_STR(x, flexname, str) \\\n@@ -864,14 +866,6 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n #define FLEXPTR_ALLOC_STR(x, ptrname, str) \\\n \tFLEXPTR_ALLOC_MEM((x), ptrname, (str), strlen(str))\n \n-static inline void *xalloc_flex(size_t base_len, size_t offset,\n-\t\t\t\tconst void *src, size_t src_len)\n-{\n-\tunsigned char *ret = xcalloc(1, st_add3(base_len, src_len, 1));\n-\tmemcpy(ret + offset, src, src_len);\n-\treturn ret;\n-}\n-\n static inline char *xstrdup_or_null(const char *str)\n {\n \treturn str ? xstrdup(str) : NULL;\n-- \n2.10.1\n"},{"id":"304229","messageId":"20161016194607.a6mhmdida57rhe23@sigill.intra.peff.net","threadId":"44305","inReplyTo":"c99474f2-dac3-e42a-5e4a-02464cac3982@web.de","subject":"Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-16T19:46:08Z","receivedAt":"2016-10-16T19:46:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 16, 2016 at 12:06:02PM +0200, René Scharfe wrote:\n\n> > Yeah, this NULL computation is pretty nasty. I recall trying to get rid\n> > of it, but I think it is impossible to do so portably while still using\n> > the generic xalloc_flex() helper.\n> \n> The only way I see is to pass the type to the macro explicitly (because\n> typeof is an extention), and that would make call sites ugly.\n\nYep, exactly. I really wish we could use typeof(); there are a couple\nthings that could be made less error-prone and ugly. But it's not even a\nmatter of C99; it's a GNU-ism, so I don't think we are even close to\nhaving support for it everywhere.\n\n> > This looks correct. I wondered at first why you bothered with\n> > flex_array_len, but it is to avoid evaluating the \"len\" parameter\n> > multiple times.\n> \n> Right; we could drop that feature of the original macros and require\n> users to pass length expressions that don't have side effects -- all of\n> them already do that anyway.  But let's keep it in this round; it just\n> costs one extra line.\n\nOh, I think it's worth keeping forever, if only because it makes the\nmacro interface have one fewer gotcha. Spending a line of code on that\nis OK with me.\n\n> > Now that xalloc_flex() has only this one caller remaining, perhaps it\n> > should just be inlined here, too, for simplicity.\n> \n> -- >8 --\n> Subject: [PATCH 2/1] inline xalloc_flex() into FLEXPTR_ALLOC_MEM\n> \n> Allocate and copy directly in FLEXPTR_ALLOC_MEM and remove the now\n> unused helper function xalloc_flex().  The resulting code is shorter\n> and the offset arithmetic is a bit simpler.\n\nLooks good. Thanks for following up.\n\n-Peff\n"}]}