{"thread":{"id":"45616","subject":"[PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","startedAt":"2017-04-06T08:02:31Z","lastAt":"2017-04-17T07:59:41Z","messageCount":15,"participants":["Martin Liška","Jeff King","Johannes Schindelin","René Scharfe","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"316258","messageId":"295981e7-d2e9-d3db-e32d-8dd80ca47136@suse.cz","threadId":"45616","inReplyTo":null,"subject":"[PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Martin Liška","fromEmail":"mliska@suse.cz","sentAt":"2017-04-06T08:02:22Z","receivedAt":"2017-04-06T08:02:31Z","isPatch":true,"sender":{"key":"mliska@suse.cz","avatar":null},"body":"Hello.\n\nFollowing patch fixes issues that can be seen with -fsanitize=undefined on GCC 7.\n\nPatch was tested with make test.\n\nThanks,\nMartin\n\n\nFrom e6d2d5ee5614acdbe67b79aeb0fdc9b53cf3a828 Mon Sep 17 00:00:00 2001\nFrom: marxin <mliska@suse.cz>\nDate: Wed, 5 Apr 2017 14:31:32 +0200\nSubject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.\n\nMemory functions like memmove and memcpy should not be called\nwith an argument equal to NULL.\n\nSigned-off-by: Martin Liska <mliska@suse.cz>\n---\n apply.c            | 7 ++++---\n builtin/ls-files.c | 5 +++--\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex e6dbab26a..78a83f66b 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2802,9 +2802,10 @@ static void update_image(struct apply_state *state,\n \t\t\timg->line + applied_pos + preimage_limit,\n \t\t\t(img->nr - (applied_pos + preimage_limit)) *\n \t\t\tsizeof(*img->line));\n-\tmemcpy(img->line + applied_pos,\n-\t       postimage->line,\n-\t       postimage->nr * sizeof(*img->line));\n+\tif (postimage->nr)\n+\t\tmemcpy(img->line + applied_pos,\n+\t\t\tpostimage->line,\n+\t\t\tpostimage->nr * sizeof(*img->line));\n \tif (!state->allow_overlap)\n \t\tfor (i = 0; i < postimage->nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db..01d24314d 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -391,8 +391,9 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n \t\t}\n \t\tlast = next;\n \t}\n-\tmemmove(active_cache, active_cache + pos,\n-\t\t(last - pos) * sizeof(struct cache_entry *));\n+\tif (last - pos > 0)\n+\t\tmemmove(active_cache, active_cache + pos,\n+\t\t\t(last - pos) * sizeof(struct cache_entry *));\n \tactive_nr = last - pos;\n }\n \n-- \n2.12.2\n\n"},{"id":"316262","messageId":"20170406083425.7psdmrploxar3h6v@sigill.intra.peff.net","threadId":"45616","inReplyTo":"295981e7-d2e9-d3db-e32d-8dd80ca47136@suse.cz","subject":"Re: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-06T08:34:25Z","receivedAt":"2017-04-06T08:34:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 06, 2017 at 10:02:22AM +0200, Martin Liška wrote:\n\n> Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.\n> \n> Memory functions like memmove and memcpy should not be called\n> with an argument equal to NULL.\n\nYeah, makes sense. Your fixes are obviously correct. In other cases\nwe've added wrappers like sane_qsort() that do the size check\nautomatically. I'm not sure if we'd want to do the same here.\n\nEither way, it probably makes sense to take this as a quick fix and\nworry about refactoring as a possible patch on top.\n\nThanks.\n\n-Peff\n"},{"id":"316264","messageId":"alpine.DEB.2.20.1704061056020.4268@virtualbox","threadId":"45616","inReplyTo":"295981e7-d2e9-d3db-e32d-8dd80ca47136@suse.cz","subject":"Re: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-06T08:57:26Z","receivedAt":"2017-04-06T08:57:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Thu, 6 Apr 2017, Martin Liška wrote:\n\n> Following patch fixes issues that can be seen with -fsanitize=undefined\n> on GCC 7.\n\nThe commit message says:\n\n\tMemory functions like memmove and memcpy should not be called\n\twith an argument equal to NULL.\n\nBut the patch is not about the pointer arguments, it is about the size\nargument. So I would suggest to say\n\n\tMemory functions like memmove and memcpy should only be called\n\twith positive sizes.\n\nCiao,\nJohannes"},{"id":"316268","messageId":"998bf391-7fc5-8329-db58-ef0f24517707@suse.cz","threadId":"45616","inReplyTo":"20170406083425.7psdmrploxar3h6v@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Martin Liška","fromEmail":"mliska@suse.cz","sentAt":"2017-04-06T09:52:15Z","receivedAt":"2017-04-06T09:52:26Z","isPatch":true,"sender":{"key":"mliska@suse.cz","avatar":null},"body":"On 04/06/2017 10:34 AM, Jeff King wrote:\n> On Thu, Apr 06, 2017 at 10:02:22AM +0200, Martin Liška wrote:\n> \n>> Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.\n>>\n>> Memory functions like memmove and memcpy should not be called\n>> with an argument equal to NULL.\n> \n> Yeah, makes sense. Your fixes are obviously correct. In other cases\n> we've added wrappers like sane_qsort() that do the size check\n> automatically. I'm not sure if we'd want to do the same here.\n> \n> Either way, it probably makes sense to take this as a quick fix and\n> worry about refactoring as a possible patch on top.\n> \n> Thanks.\n> \n> -Peff\n> \n\nHello.\n\nI'm sending (v2), where I updated commit message and wrapped 2 problematic\nplaces to newly introduced macros that do the check. Follow-up patch can\nchange usages of memcpy and memove.\n\nMartin\n\n\nFrom 876cfa4f4b2e74d43b9fd93d1056b88ab2ace0cd Mon Sep 17 00:00:00 2001\nFrom: marxin <mliska@suse.cz>\nDate: Wed, 5 Apr 2017 14:31:32 +0200\nSubject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.\n\nMemory functions like memmove and memcpy should not be called\nwith an argument equal to NULL.\n\nSigned-off-by: Martin Liska <mliska@suse.cz>\n---\n apply.c            |  6 +++---\n builtin/ls-files.c |  2 +-\n git-compat-util.h  | 20 ++++++++++++++++++++\n 3 files changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex e6dbab26a..eacca29fa 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2802,9 +2802,9 @@ static void update_image(struct apply_state *state,\n \t\t\timg->line + applied_pos + preimage_limit,\n \t\t\t(img->nr - (applied_pos + preimage_limit)) *\n \t\t\tsizeof(*img->line));\n-\tmemcpy(img->line + applied_pos,\n-\t       postimage->line,\n-\t       postimage->nr * sizeof(*img->line));\n+\tMEMCPY(img->line + applied_pos,\n+\t\tpostimage->line,\n+\t\tpostimage->nr * sizeof(*img->line));\n \tif (!state->allow_overlap)\n \t\tfor (i = 0; i < postimage->nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db..7caeeb6a6 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n \t\t}\n \t\tlast = next;\n \t}\n-\tmemmove(active_cache, active_cache + pos,\n+\tMEMMOVE(active_cache, active_cache + pos,\n \t\t(last - pos) * sizeof(struct cache_entry *));\n \tactive_nr = last - pos;\n }\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8a4a3f85e..e5323f6c7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1002,6 +1002,26 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,\n \t\tdie(\"BUG: qsort_s() failed\");\t\t\t\\\n } while (0)\n \n+static inline void *sane_memcpy(void *dest, const void *src, size_t n)\n+{\n+\tif (n > 0)\n+\t\treturn memcpy(dest, src, n);\n+\telse\n+\t\treturn dest;\n+}\n+\n+#define MEMCPY(dest, src, n) sane_memcpy(dest, src, n)\n+\n+static inline void *sane_memmove(void *dest, const void *src, size_t n)\n+{\n+\tif (n > 0)\n+\t\treturn memmove(dest, src, n);\n+\telse\n+\t\treturn dest;\n+}\n+\n+#define MEMMOVE(dest, src, n) sane_memmove(dest, src, n)\n+\n #ifndef REG_STARTEND\n #error \"Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd\"\n #endif\n-- \n2.12.2\n\n"},{"id":"316271","messageId":"33c63fb9-281c-8fd2-66e7-b85f62f4f447@web.de","threadId":"45616","inReplyTo":"998bf391-7fc5-8329-db58-ef0f24517707@suse.cz","subject":"Re: [PATCH v2 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-06T12:26:55Z","receivedAt":"2017-04-06T12:27:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.04.2017 um 11:52 schrieb Martin Liška:\n> I'm sending (v2), where I updated commit message and wrapped 2 problematic\n> places to newly introduced macros that do the check. Follow-up patch can\n> change usages of memcpy and memove.\n\n> diff --git a/apply.c b/apply.c\n> index e6dbab26a..eacca29fa 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -2802,9 +2802,9 @@ static void update_image(struct apply_state *state,\n>  \t\t\timg->line + applied_pos + preimage_limit,\n>  \t\t\t(img->nr - (applied_pos + preimage_limit)) *\n>  \t\t\tsizeof(*img->line));\n> -\tmemcpy(img->line + applied_pos,\n> -\t       postimage->line,\n> -\t       postimage->nr * sizeof(*img->line));\n> +\tMEMCPY(img->line + applied_pos,\n> +\t\tpostimage->line,\n> +\t\tpostimage->nr * sizeof(*img->line));\n\nUsing the existing macro COPY_ARRAY would yield a nicer result:\n\n\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);\n\n>  \tif (!state->allow_overlap)\n>  \t\tfor (i = 0; i < postimage->nr; i++)\n>  \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index d449e46db..7caeeb6a6 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n>  \t\t}\n>  \t\tlast = next;\n>  \t}\n> -\tmemmove(active_cache, active_cache + pos,\n> +\tMEMMOVE(active_cache, active_cache + pos,\n>  \t\t(last - pos) * sizeof(struct cache_entry *));\n>  \tactive_nr = last - pos;\n>  }\n\nPerhaps we should add MOVE_ARRAY to complement COPY_ARRAY.\n\nRené\n"},{"id":"316283","messageId":"8555c61f-2617-eec8-6dbe-87c79c6ca302@suse.cz","threadId":"45616","inReplyTo":"33c63fb9-281c-8fd2-66e7-b85f62f4f447@web.de","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Martin Liška","fromEmail":"mliska@suse.cz","sentAt":"2017-04-06T15:42:34Z","receivedAt":"2017-04-06T15:42:57Z","isPatch":true,"sender":{"key":"mliska@suse.cz","avatar":null},"body":"On 04/06/2017 02:26 PM, René Scharfe wrote:\n> Am 06.04.2017 um 11:52 schrieb Martin Liška:\n>> I'm sending (v2), where I updated commit message and wrapped 2 problematic\n>> places to newly introduced macros that do the check. Follow-up patch can\n>> change usages of memcpy and memove.\n> \n>> diff --git a/apply.c b/apply.c\n>> index e6dbab26a..eacca29fa 100644\n>> --- a/apply.c\n>> +++ b/apply.c\n>> @@ -2802,9 +2802,9 @@ static void update_image(struct apply_state *state,\n>>  \t\t\timg->line + applied_pos + preimage_limit,\n>>  \t\t\t(img->nr - (applied_pos + preimage_limit)) *\n>>  \t\t\tsizeof(*img->line));\n>> -\tmemcpy(img->line + applied_pos,\n>> -\t       postimage->line,\n>> -\t       postimage->nr * sizeof(*img->line));\n>> +\tMEMCPY(img->line + applied_pos,\n>> +\t\tpostimage->line,\n>> +\t\tpostimage->nr * sizeof(*img->line));\n> \n> Using the existing macro COPY_ARRAY would yield a nicer result:\n\nYes, it's nicer. I'm sending tested version 3.\n\nMartin\n\n> \n> \tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);\n> \n>>  \tif (!state->allow_overlap)\n>>  \t\tfor (i = 0; i < postimage->nr; i++)\n>>  \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>> index d449e46db..7caeeb6a6 100644\n>> --- a/builtin/ls-files.c\n>> +++ b/builtin/ls-files.c\n>> @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n>>  \t\t}\n>>  \t\tlast = next;\n>>  \t}\n>> -\tmemmove(active_cache, active_cache + pos,\n>> +\tMEMMOVE(active_cache, active_cache + pos,\n>>  \t\t(last - pos) * sizeof(struct cache_entry *));\n>>  \tactive_nr = last - pos;\n>>  }\n> \n> Perhaps we should add MOVE_ARRAY to complement COPY_ARRAY.\n> \n> René\n> \n\n\n\nFrom 4784ff90b2c4cd0d78a25c8d8ed77f299686348c Mon Sep 17 00:00:00 2001\nFrom: marxin <mliska@suse.cz>\nDate: Wed, 5 Apr 2017 14:31:32 +0200\nSubject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.\n\nMemory functions like memmove and memcpy should only be called\nwith positive sizes. That is achieved by newly introduced macro\nMEMMOVE and usage of ARRAY_COPY.\n\nSigned-off-by: Martin Liska <mliska@suse.cz>\n---\n apply.c            |  4 +---\n builtin/ls-files.c |  2 +-\n git-compat-util.h  | 10 ++++++++++\n 3 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex e6dbab26a..121f3f414 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2802,9 +2802,7 @@ static void update_image(struct apply_state *state,\n \t\t\timg->line + applied_pos + preimage_limit,\n \t\t\t(img->nr - (applied_pos + preimage_limit)) *\n \t\t\tsizeof(*img->line));\n-\tmemcpy(img->line + applied_pos,\n-\t       postimage->line,\n-\t       postimage->nr * sizeof(*img->line));\n+\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);\n \tif (!state->allow_overlap)\n \t\tfor (i = 0; i < postimage->nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db..7caeeb6a6 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n \t\t}\n \t\tlast = next;\n \t}\n-\tmemmove(active_cache, active_cache + pos,\n+\tMEMMOVE(active_cache, active_cache + pos,\n \t\t(last - pos) * sizeof(struct cache_entry *));\n \tactive_nr = last - pos;\n }\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8a4a3f85e..b75e21cff 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1002,6 +1002,16 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,\n \t\tdie(\"BUG: qsort_s() failed\");\t\t\t\\\n } while (0)\n \n+static inline void *sane_memmove(void *dest, const void *src, size_t n)\n+{\n+\tif (n > 0)\n+\t\treturn memmove(dest, src, n);\n+\telse\n+\t\treturn dest;\n+}\n+\n+#define MEMMOVE(dest, src, n) sane_memmove(dest, src, n)\n+\n #ifndef REG_STARTEND\n #error \"Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd\"\n #endif\n-- \n2.12.2\n\n"},{"id":"316285","messageId":"587b0cb9-bd66-ddf7-5cca-023df3470883@kdbg.org","threadId":"45616","inReplyTo":"8555c61f-2617-eec8-6dbe-87c79c6ca302@suse.cz","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-06T16:33:51Z","receivedAt":"2017-04-06T16:33:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.04.2017 um 17:42 schrieb Martin Liška:\n> +static inline void *sane_memmove(void *dest, const void *src, size_t n)\n> +{\n> +\tif (n > 0)\n> +\t\treturn memmove(dest, src, n);\n> +\telse\n> +\t\treturn dest;\n> +}\n\nHuh? memmove with n == 0 is well-defined. This wrapper is pointless.\n\n-- Hannes\n\n"},{"id":"316293","messageId":"e392e05c-2815-8cfa-eed0-bd990f8ce954@web.de","threadId":"45616","inReplyTo":"587b0cb9-bd66-ddf7-5cca-023df3470883@kdbg.org","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-06T17:31:38Z","receivedAt":"2017-04-06T17:32:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.04.2017 um 18:33 schrieb Johannes Sixt:\n> Am 06.04.2017 um 17:42 schrieb Martin Liška:\n>> +static inline void *sane_memmove(void *dest, const void *src, size_t n)\n>> +{\n>> +    if (n > 0)\n>> +        return memmove(dest, src, n);\n>> +    else\n>> +        return dest;\n>> +}\n>\n> Huh? memmove with n == 0 is well-defined. This wrapper is pointless.\n\nmemmove(3) with NULL pointers is undefined.  From string.h on Debian:\n\n   extern void *memmove (void *__dest, const void *__src, size_t __n)\n        __THROW __nonnull ((1, 2));\n\nSometimes we use a NULL pointer and a size of zero to represent arrays \nwith no members.  That convention is incompatible with memmove(3), but \nthe wrapper above would support it.  Checking the size instead of the \npointer is preferable because a positive length with NULL pointers \nshould still result in a segfault instead of a silent no-op.\n\n(I'd still prefer a MOVE_ARRAY macro which also infers the element\nsize).\n\nRené\n"},{"id":"316309","messageId":"357d045a-b1c5-1a73-2256-839efb543fe3@kdbg.org","threadId":"45616","inReplyTo":"e392e05c-2815-8cfa-eed0-bd990f8ce954@web.de","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-06T20:49:17Z","receivedAt":"2017-04-06T20:49:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.04.2017 um 19:31 schrieb René Scharfe:\n> Am 06.04.2017 um 18:33 schrieb Johannes Sixt:\n>> Am 06.04.2017 um 17:42 schrieb Martin Liška:\n>>> +static inline void *sane_memmove(void *dest, const void *src, size_t n)\n>>> +{\n>>> +    if (n > 0)\n>>> +        return memmove(dest, src, n);\n>>> +    else\n>>> +        return dest;\n>>> +}\n>>\n>> Huh? memmove with n == 0 is well-defined. This wrapper is pointless.\n>\n> memmove(3) with NULL pointers is undefined.\n\nThen don't hide this helper behind a macro with a suspiciously similar \nname. Using the name sane_mmemove at the call site is preferable. \nmemmove_or_null or something similar perhaps even more so.\n\n-- Hannes\n\n"},{"id":"316354","messageId":"96beb4c6-0569-0c12-8151-462c20be6a2a@suse.cz","threadId":"45616","inReplyTo":"357d045a-b1c5-1a73-2256-839efb543fe3@kdbg.org","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Martin Liška","fromEmail":"mliska@suse.cz","sentAt":"2017-04-07T14:23:13Z","receivedAt":"2017-04-07T14:23:20Z","isPatch":true,"sender":{"key":"mliska@suse.cz","avatar":null},"body":"On 04/06/2017 10:49 PM, Johannes Sixt wrote:\n> Am 06.04.2017 um 19:31 schrieb René Scharfe:\n>> Am 06.04.2017 um 18:33 schrieb Johannes Sixt:\n>>> Am 06.04.2017 um 17:42 schrieb Martin Liška:\n>>>> +static inline void *sane_memmove(void *dest, const void *src, size_t n)\n>>>> +{\n>>>> +    if (n > 0)\n>>>> +        return memmove(dest, src, n);\n>>>> +    else\n>>>> +        return dest;\n>>>> +}\n>>>\n>>> Huh? memmove with n == 0 is well-defined. This wrapper is pointless.\n>>\n>> memmove(3) with NULL pointers is undefined.\n> \n> Then don't hide this helper behind a macro with a suspiciously similar name. Using the name sane_mmemove at the call site is preferable. memmove_or_null or something similar perhaps even more so.\n> \n> -- Hannes\n> \n\nGood. There's tested v4.\n\nMartin\n\n\nFrom 0bdf4d717d3d368dd9676d15d20f8592c4d22fde Mon Sep 17 00:00:00 2001\nFrom: marxin <mliska@suse.cz>\nDate: Wed, 5 Apr 2017 14:31:32 +0200\nSubject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.\n\nReplace call to memmove with newly introduced function memmove_or_null\nand call to memcpy with COPY_ARRAY macro.\n\nSigned-off-by: Martin Liska <mliska@suse.cz>\n---\n apply.c            | 4 +---\n builtin/ls-files.c | 2 +-\n git-compat-util.h  | 8 ++++++++\n 3 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex e6dbab26a..121f3f414 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2802,9 +2802,7 @@ static void update_image(struct apply_state *state,\n \t\t\timg->line + applied_pos + preimage_limit,\n \t\t\t(img->nr - (applied_pos + preimage_limit)) *\n \t\t\tsizeof(*img->line));\n-\tmemcpy(img->line + applied_pos,\n-\t       postimage->line,\n-\t       postimage->nr * sizeof(*img->line));\n+\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);\n \tif (!state->allow_overlap)\n \t\tfor (i = 0; i < postimage->nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db..0a6cc1e8a 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n \t\t}\n \t\tlast = next;\n \t}\n-\tmemmove(active_cache, active_cache + pos,\n+\tmemmove_or_null(active_cache, active_cache + pos,\n \t\t(last - pos) * sizeof(struct cache_entry *));\n \tactive_nr = last - pos;\n }\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8a4a3f85e..81f6e56ac 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1002,6 +1002,14 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,\n \t\tdie(\"BUG: qsort_s() failed\");\t\t\t\\\n } while (0)\n \n+static inline void *memmove_or_null(void *dest, const void *src, size_t n)\n+{\n+\tif (n > 0)\n+\t\treturn memmove(dest, src, n);\n+\telse\n+\t\treturn dest;\n+}\n+\n #ifndef REG_STARTEND\n #error \"Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd\"\n #endif\n-- \n2.12.2\n\n"},{"id":"316356","messageId":"425176fe-090e-6edf-b6ba-fa77e35b3dd1@web.de","threadId":"45616","inReplyTo":"96beb4c6-0569-0c12-8151-462c20be6a2a@suse.cz","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-07T15:25:06Z","receivedAt":"2017-04-07T15:25:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.04.2017 um 16:23 schrieb Martin Liška:\n> On 04/06/2017 10:49 PM, Johannes Sixt wrote:\n>> Am 06.04.2017 um 19:31 schrieb René Scharfe:\n>>> Am 06.04.2017 um 18:33 schrieb Johannes Sixt:\n>>>> Am 06.04.2017 um 17:42 schrieb Martin Liška:\n>>>>> +static inline void *sane_memmove(void *dest, const void *src, size_t n)\n>>>>> +{\n>>>>> +    if (n > 0)\n>>>>> +        return memmove(dest, src, n);\n>>>>> +    else\n>>>>> +        return dest;\n>>>>> +}\n>>>>\n>>>> Huh? memmove with n == 0 is well-defined. This wrapper is pointless.\n>>>\n>>> memmove(3) with NULL pointers is undefined.\n>>\n>> Then don't hide this helper behind a macro with a suspiciously similar name. Using the name sane_mmemove at the call site is preferable. memmove_or_null or something similar perhaps even more so.\n>>\n>> -- Hannes\n>>\n> \n> Good. There's tested v4.\n\nThank you!  I'd *still* prefer MOVE_ARRAY, though.  Patches to follow.\n\nRené\n\n"},{"id":"316357","messageId":"c4c60ec9-641d-fb2e-046a-91e08615c8f7@web.de","threadId":"45616","inReplyTo":"96beb4c6-0569-0c12-8151-462c20be6a2a@suse.cz","subject":"[PATCH 1/2] add MOVE_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-07T15:25:15Z","receivedAt":"2017-04-07T15:25:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add MOVE_ARRAY to complement COPY_ARRAY, which was added in 60566cbb.\nIt provides the same convenience, safety and support for NULL pointers\nas representations of empty arrays, just as a wrapper for memmove(3)\ninstead of memcpy(3).\n\nInspired-by: Martin Liska <mliska@suse.cz>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n git-compat-util.h | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8a4a3f85e7..3266a71fb4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -812,6 +812,14 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n \t\tmemcpy(dst, src, st_mult(size, n));\n }\n \n+#define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \\\n+\tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n+static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n+{\n+\tif (n)\n+\t\tmemmove(dst, src, st_mult(size, n));\n+}\n+\n /*\n  * These functions help you allocate structs with flex arrays, and copy\n  * the data directly into the array. For example, if you had:\n-- \n2.12.2\n\n"},{"id":"316358","messageId":"c4bdb724-62d5-056d-0667-09e446bb3119@web.de","threadId":"45616","inReplyTo":"96beb4c6-0569-0c12-8151-462c20be6a2a@suse.cz","subject":"[PATCH 2/2] use MOVE_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-07T15:25:25Z","receivedAt":"2017-04-07T15:25:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a semantic patch for converting certain calls of memmove(3) to\nMOVE_ARRAY() and apply that transformation to the code base.  The result\nis shorter and safer code.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n apply.c                        |  7 +++----\n builtin/ls-files.c             |  3 +--\n builtin/merge.c                |  2 +-\n builtin/pack-objects.c         |  5 ++---\n cache-tree.c                   |  5 ++---\n commit.c                       | 11 ++++-------\n contrib/coccinelle/array.cocci | 37 +++++++++++++++++++++++++++++++++++++\n diffcore-rename.c              |  8 ++++----\n dir.c                          |  4 ++--\n notes-merge.c                  |  3 +--\n parse-options.c                |  2 +-\n read-cache.c                   |  5 ++---\n reflog-walk.c                  |  7 +++----\n refs/files-backend.c           |  5 ++---\n replace_object.c               |  6 ++----\n rerere.c                       |  4 ++--\n string-list.c                  |  5 ++---\n 17 files changed, 71 insertions(+), 48 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex e6dbab26ad..4e4d13cacc 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2798,10 +2798,9 @@ static void update_image(struct apply_state *state,\n \t\timg->line_allocated = img->line;\n \t}\n \tif (preimage_limit != postimage->nr)\n-\t\tmemmove(img->line + applied_pos + postimage->nr,\n-\t\t\timg->line + applied_pos + preimage_limit,\n-\t\t\t(img->nr - (applied_pos + preimage_limit)) *\n-\t\t\tsizeof(*img->line));\n+\t\tMOVE_ARRAY(img->line + applied_pos + postimage->nr,\n+\t\t\t   img->line + applied_pos + preimage_limit,\n+\t\t\t   img->nr - (applied_pos + preimage_limit));\n \tmemcpy(img->line + applied_pos,\n \t       postimage->line,\n \t       postimage->nr * sizeof(*img->line));\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db5..1627c26466 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -391,8 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n \t\t}\n \t\tlast = next;\n \t}\n-\tmemmove(active_cache, active_cache + pos,\n-\t\t(last - pos) * sizeof(struct cache_entry *));\n+\tMOVE_ARRAY(active_cache, active_cache + pos, last - pos);\n \tactive_nr = last - pos;\n }\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 95572b1810..0c0d2e4a94 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -536,7 +536,7 @@ static void parse_branch_merge_options(char *bmo)\n \t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n \t\t    split_cmdline_strerror(argc));\n \tREALLOC_ARRAY(argv, argc + 2);\n-\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n+\tMOVE_ARRAY(argv + 1, argv, argc + 1);\n \targc++;\n \targv[0] = \"branch.*.mergeoptions\";\n \tparse_options(argc, argv, NULL, builtin_merge_options,\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 84af7c2324..5fb5d7c546 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1292,9 +1292,8 @@ static int check_pbase_path(unsigned hash)\n \t\t   done_pbase_paths_alloc);\n \tdone_pbase_paths_num++;\n \tif (pos < done_pbase_paths_num)\n-\t\tmemmove(done_pbase_paths + pos + 1,\n-\t\t\tdone_pbase_paths + pos,\n-\t\t\t(done_pbase_paths_num - pos - 1) * sizeof(unsigned));\n+\t\tMOVE_ARRAY(done_pbase_paths + pos + 1, done_pbase_paths + pos,\n+\t\t\t   done_pbase_paths_num - pos - 1);\n \tdone_pbase_paths[pos] = hash;\n \treturn 0;\n }\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 345ea35963..071164a2ec 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -131,9 +131,8 @@ static int do_invalidate_path(struct cache_tree *it, const char *path)\n \t\t\t * move 4 and 5 up one place (2 entries)\n \t\t\t * 2 = 6 - 3 - 1 = subtree_nr - pos - 1\n \t\t\t */\n-\t\t\tmemmove(it->down+pos, it->down+pos+1,\n-\t\t\t\tsizeof(struct cache_tree_sub *) *\n-\t\t\t\t(it->subtree_nr - pos - 1));\n+\t\t\tMOVE_ARRAY(it->down + pos, it->down + pos + 1,\n+\t\t\t\t   it->subtree_nr - pos - 1);\n \t\t\tit->subtree_nr--;\n \t\t}\n \t\treturn 1;\ndiff --git a/commit.c b/commit.c\nindex 73c78c2b80..62a71fee92 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -125,10 +125,8 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n \tALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);\n \tcommit_graft_nr++;\n \tif (pos < commit_graft_nr)\n-\t\tmemmove(commit_graft + pos + 1,\n-\t\t\tcommit_graft + pos,\n-\t\t\t(commit_graft_nr - pos - 1) *\n-\t\t\tsizeof(*commit_graft));\n+\t\tMOVE_ARRAY(commit_graft + pos + 1, commit_graft + pos,\n+\t\t\t   commit_graft_nr - pos - 1);\n \tcommit_graft[pos] = graft;\n \treturn 0;\n }\n@@ -222,9 +220,8 @@ int unregister_shallow(const unsigned char *sha1)\n \tif (pos < 0)\n \t\treturn -1;\n \tif (pos + 1 < commit_graft_nr)\n-\t\tmemmove(commit_graft + pos, commit_graft + pos + 1,\n-\t\t\t\tsizeof(struct commit_graft *)\n-\t\t\t\t* (commit_graft_nr - pos - 1));\n+\t\tMOVE_ARRAY(commit_graft + pos, commit_graft + pos + 1,\n+\t\t\t   commit_graft_nr - pos - 1);\n \tcommit_graft_nr--;\n \treturn 0;\n }\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex 4ba98b7eaf..a6e5ee927f 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -27,6 +27,43 @@ expression n;\n \n @@\n type T;\n+T *dst;\n+T *src;\n+expression n;\n+@@\n+(\n+- memmove(\n++ MOVE_ARRAY(\n+(\n+  dst\n+|\n+  dst + ...\n+|\n+  &dst[...]\n+)\n+- , src, (n) * sizeof(*dst)\n++ , src, n\n+  );\n+|\n+- memmove(dst,\n++ MOVE_ARRAY(dst,\n+(\n+  src\n+|\n+  src + ...\n+|\n+  &src[...]\n+)\n+- , (n) * sizeof(*src)\n++ , n\n+  );\n+|\n+- memmove(dst, src, (n) * sizeof(T));\n++ MOVE_ARRAY(dst, src, n);\n+)\n+\n+@@\n+type T;\n T *ptr;\n expression n;\n @@\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex f7444c86bd..449d5f065f 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -57,8 +57,8 @@ static int add_rename_dst(struct diff_filespec *two)\n \tALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc);\n \trename_dst_nr++;\n \tif (first < rename_dst_nr)\n-\t\tmemmove(rename_dst + first + 1, rename_dst + first,\n-\t\t\t(rename_dst_nr - first - 1) * sizeof(*rename_dst));\n+\t\tMOVE_ARRAY(rename_dst + first + 1, rename_dst + first,\n+\t\t\t   rename_dst_nr - first - 1);\n \trename_dst[first].two = alloc_filespec(two->path);\n \tfill_filespec(rename_dst[first].two, two->oid.hash, two->oid_valid,\n \t\t      two->mode);\n@@ -98,8 +98,8 @@ static struct diff_rename_src *register_rename_src(struct diff_filepair *p)\n \tALLOC_GROW(rename_src, rename_src_nr + 1, rename_src_alloc);\n \trename_src_nr++;\n \tif (first < rename_src_nr)\n-\t\tmemmove(rename_src + first + 1, rename_src + first,\n-\t\t\t(rename_src_nr - first - 1) * sizeof(*rename_src));\n+\t\tMOVE_ARRAY(rename_src + first + 1, rename_src + first,\n+\t\t\t   rename_src_nr - first - 1);\n \trename_src[first].p = p;\n \trename_src[first].score = score;\n \treturn &(rename_src[first]);\ndiff --git a/dir.c b/dir.c\nindex f451bfa48c..f5f5658b1d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -691,8 +691,8 @@ static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,\n \tFLEX_ALLOC_MEM(d, name, name, len);\n \n \tALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc);\n-\tmemmove(dir->dirs + first + 1, dir->dirs + first,\n-\t\t(dir->dirs_nr - first) * sizeof(*dir->dirs));\n+\tMOVE_ARRAY(dir->dirs + first + 1, dir->dirs + first,\n+\t\t   dir->dirs_nr - first);\n \tdir->dirs_nr++;\n \tdir->dirs[first] = d;\n \treturn d;\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 5998605acc..36d5d2f87f 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -99,8 +99,7 @@ static struct notes_merge_pair *find_notes_merge_pair_pos(\n \telse {\n \t\t*occupied = 0;\n \t\tif (insert_new && i < len) {\n-\t\t\tmemmove(list + i + 1, list + i,\n-\t\t\t\t(len - i) * sizeof(struct notes_merge_pair));\n+\t\t\tMOVE_ARRAY(list + i + 1, list + i, len - i);\n \t\t\tmemset(list + i, 0, sizeof(struct notes_merge_pair));\n \t\t}\n \t}\ndiff --git a/parse-options.c b/parse-options.c\nindex a23a1e67f0..b8f768b0d8 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -524,7 +524,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n {\n-\tmemmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));\n+\tMOVE_ARRAY(ctx->out + ctx->cpidx, ctx->argv, ctx->argc);\n \tctx->out[ctx->cpidx + ctx->argc] = NULL;\n \treturn ctx->cpidx + ctx->argc;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex e447751823..1bb5e97979 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -514,9 +514,8 @@ int remove_index_entry_at(struct index_state *istate, int pos)\n \tistate->cache_nr--;\n \tif (pos >= istate->cache_nr)\n \t\treturn 0;\n-\tmemmove(istate->cache + pos,\n-\t\tistate->cache + pos + 1,\n-\t\t(istate->cache_nr - pos) * sizeof(struct cache_entry *));\n+\tMOVE_ARRAY(istate->cache + pos, istate->cache + pos + 1,\n+\t\t   istate->cache_nr - pos);\n \treturn 1;\n }\n \ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 99679f5825..25c198b9fd 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -95,10 +95,9 @@ static struct commit_info *get_commit_info(struct commit *commit,\n \t\t\tstruct commit_info *result = &lifo->items[i];\n \t\t\tif (pop) {\n \t\t\t\tif (i + 1 < lifo->nr)\n-\t\t\t\t\tmemmove(lifo->items + i,\n-\t\t\t\t\t\tlifo->items + i + 1,\n-\t\t\t\t\t\t(lifo->nr - i) *\n-\t\t\t\t\t\tsizeof(struct commit_info));\n+\t\t\t\t\tMOVE_ARRAY(lifo->items + i,\n+\t\t\t\t\t\t   lifo->items + i + 1,\n+\t\t\t\t\t\t   lifo->nr - i);\n \t\t\t\tlifo->nr--;\n \t\t\t}\n \t\t\treturn result;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 50188e92f9..a851326445 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -437,9 +437,8 @@ static int remove_entry(struct ref_dir *dir, const char *refname)\n \t\treturn -1;\n \tentry = dir->entries[entry_index];\n \n-\tmemmove(&dir->entries[entry_index],\n-\t\t&dir->entries[entry_index + 1],\n-\t\t(dir->nr - entry_index - 1) * sizeof(*dir->entries)\n+\tMOVE_ARRAY(&dir->entries[entry_index], &dir->entries[entry_index + 1],\n+\t\tdir->nr - entry_index - 1\n \t\t);\n \tdir->nr--;\n \tif (dir->sorted > entry_index)\ndiff --git a/replace_object.c b/replace_object.c\nindex f0b39f06d5..3e49965d05 100644\n--- a/replace_object.c\n+++ b/replace_object.c\n@@ -44,10 +44,8 @@ static int register_replace_object(struct replace_object *replace,\n \tALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc);\n \treplace_object_nr++;\n \tif (pos < replace_object_nr)\n-\t\tmemmove(replace_object + pos + 1,\n-\t\t\treplace_object + pos,\n-\t\t\t(replace_object_nr - pos - 1) *\n-\t\t\tsizeof(*replace_object));\n+\t\tMOVE_ARRAY(replace_object + pos + 1, replace_object + pos,\n+\t\t\t   replace_object_nr - pos - 1);\n \treplace_object[pos] = replace;\n \treturn 0;\n }\ndiff --git a/rerere.c b/rerere.c\nindex 3bd55caf3b..7410445ba2 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -159,8 +159,8 @@ static struct rerere_dir *find_rerere_dir(const char *hex)\n \t\tALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc);\n \t\t/* ... and add it in. */\n \t\trerere_dir_nr++;\n-\t\tmemmove(rerere_dir + pos + 1, rerere_dir + pos,\n-\t\t\t(rerere_dir_nr - pos - 1) * sizeof(*rerere_dir));\n+\t\tMOVE_ARRAY(rerere_dir + pos + 1, rerere_dir + pos,\n+\t\t\t   rerere_dir_nr - pos - 1);\n \t\trerere_dir[pos] = rr_dir;\n \t\tscan_rerere_dir(rr_dir);\n \t}\ndiff --git a/string-list.c b/string-list.c\nindex 45016ad86d..df64d4c446 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -46,9 +46,8 @@ static int add_entry(int insert_at, struct string_list *list, const char *string\n \t\tREALLOC_ARRAY(list->items, list->alloc);\n \t}\n \tif (index < list->nr)\n-\t\tmemmove(list->items + index + 1, list->items + index,\n-\t\t\t\t(list->nr - index)\n-\t\t\t\t* sizeof(struct string_list_item));\n+\t\tMOVE_ARRAY(list->items + index + 1, list->items + index,\n+\t\t\t   list->nr - index);\n \tlist->items[index].string = list->strdup_strings ?\n \t\txstrdup(string) : (char *)string;\n \tlist->items[index].util = NULL;\n-- \n2.12.2\n\n"},{"id":"316979","messageId":"xmqqy3uzkdm4.fsf@gitster.mtv.corp.google.com","threadId":"45616","inReplyTo":"96beb4c6-0569-0c12-8151-462c20be6a2a@suse.cz","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-17T01:49:07Z","receivedAt":"2017-04-17T01:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Liška <mliska@suse.cz> writes:\n\n> From 0bdf4d717d3d368dd9676d15d20f8592c4d22fde Mon Sep 17 00:00:00 2001\n> From: marxin <mliska@suse.cz>\n> Date: Wed, 5 Apr 2017 14:31:32 +0200\n> Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7.\n>\n> Replace call to memmove with newly introduced function memmove_or_null\n> and call to memcpy with COPY_ARRAY macro.\n\nI didn't closely follow the discussion, but with these three lines\n(which will be the primary thing future readers of this change in\n\"git log -p\" output will rely on), it is unclear why this change was\nmade.  For that matter, it is not clear what \"nonnull errors\" are,\neither.\n\n> Signed-off-by: Martin Liska <mliska@suse.cz>\n> ---\n>  apply.c            | 4 +---\n>  builtin/ls-files.c | 2 +-\n>  git-compat-util.h  | 8 ++++++++\n>  3 files changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index e6dbab26a..121f3f414 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -2802,9 +2802,7 @@ static void update_image(struct apply_state *state,\n>  \t\t\timg->line + applied_pos + preimage_limit,\n>  \t\t\t(img->nr - (applied_pos + preimage_limit)) *\n>  \t\t\tsizeof(*img->line));\n> -\tmemcpy(img->line + applied_pos,\n> -\t       postimage->line,\n> -\t       postimage->nr * sizeof(*img->line));\n> +\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);\n\nI am suspecting that postimage->nr can be 0 and newer compliers can\ngive warning \"hey what's the point of copying 0 bytes?\" which can be\nsquelched by moving to COPY_ARRAY()?  If that is the case I like the\nchange (but as I said, it is unclear if that is what is going on\nhere).\n\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index d449e46db..0a6cc1e8a 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen)\n>  \t\t}\n>  \t\tlast = next;\n>  \t}\n> -\tmemmove(active_cache, active_cache + pos,\n> +\tmemmove_or_null(active_cache, active_cache + pos,\n>  \t\t(last - pos) * sizeof(struct cache_entry *));\n\nDoes this change come with the same or a similar motivation as the\nabove (i.e. pos could be the same as last)?\n\n\"Something or NULL\" is a name we use for a function that returns\nsomething (under normal circumstances) or returns NULL.  This\nwrapper is not about returning NULL at all, as far as I can see, and\nis misnamed.  If it is about \"avoid moving 0 bytes\", similar to how\nCOPY_ARRAY() is used in the previous hunk, perhaps MOVE_ARRAY() is a\nbetter name?\n\nThanks.\n\n>  \tactive_nr = last - pos;\n>  }\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 8a4a3f85e..81f6e56ac 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1002,6 +1002,14 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,\n>  \t\tdie(\"BUG: qsort_s() failed\");\t\t\t\\\n>  } while (0)\n>  \n> +static inline void *memmove_or_null(void *dest, const void *src, size_t n)\n> +{\n> +\tif (n > 0)\n> +\t\treturn memmove(dest, src, n);\n> +\telse\n> +\t\treturn dest;\n> +}\n> +\n>  #ifndef REG_STARTEND\n>  #error \"Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd\"\n>  #endif\n"},{"id":"316994","messageId":"930be745-944f-62f3-3d57-4f1cd6f2df66@kdbg.org","threadId":"45616","inReplyTo":"xmqqy3uzkdm4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7.","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-17T07:59:29Z","receivedAt":"2017-04-17T07:59:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.04.2017 um 03:49 schrieb Junio C Hamano:\n> \"Something or NULL\" is a name we use for a function that returns\n> something (under normal circumstances) or returns NULL.  This\n> wrapper is not about returning NULL at all, as far as I can see, and\n> is misnamed.  If it is about \"avoid moving 0 bytes\", similar to how\n> COPY_ARRAY() is used in the previous hunk, perhaps MOVE_ARRAY() is a\n> better name?\n\nIt is not about \"avoid moving 0 bytes\", but \"if we move 0 bytes, then we \nallow NULL pointers\". Plain memmove/memcpy do not allow the pointers to \nbe NULL even if the count is 0. It just so happens that the \nimplementation of memmove_or_null that permits the relaxed condition \nlooks like \"avoid moving 0 bytes\".\n\nThe name was my suggestion, but I agree that it is not the best name. \n[Sentence about two most difficult things in software engineering \nomitted for brevity.]\n\n-- Hannes\n\n"}]}