{"thread":{"id":"52254","subject":"parse-options: avoid arithmetic on pointer that's potentially NULL","startedAt":"2019-11-12T21:41:41Z","lastAt":"2019-11-14T16:01:36Z","messageCount":5,"participants":["René Scharfe","Junio C Hamano","Martin Ågren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"386049","messageId":"39a0b622-f725-9284-ea50-19cf4078209d@web.de","threadId":"52254","inReplyTo":null,"subject":"parse-options: avoid arithmetic on pointer that's potentially NULL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-12T21:41:34Z","receivedAt":"2019-11-12T21:41:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"parse_options_dup() counts the number of elements in the given array\nwithout the end marker, allocates enough memory to hold all of them plus\nan end marker, then copies them and terminates the new array.  The\ncounting part is done by advancing a pointer through the array, and the\noriginal pointer is reconstructed using pointer subtraction before the\ncopy operation.\n\nThe function is also prepared to handle a NULL pointer passed to it.\nNone of its callers do that currently, but this feature was used by\n46e91b663b (\"checkout: split part of it to new command 'restore'\",\n2019-04-25); it seems worth keeping.\n\nIt ends up doing arithmetic on that NULL pointer, though, which is\nundefined in standard C, when it tries to calculate \"NULL - 0\".  Better\navoid doing that by remembering the originally given pointer value.\n\nThere is another issue, though.  memcpy(3) does not support NULL\npointers, even for empty arrays.  Use COPY_ARRAY instead, which does\nsupport such empty arrays.  Its call is also shorter and safer by\ninferring the element type automatically.\n\nCoccinelle and contrib/coccinelle/array.cocci did not propose to use\nCOPY_ARRAY because of the pointer subtraction and because the source is\nconst -- the semantic patch cautiously only considers pointers and array\nreferences of the same type.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nPatch formatted with --function-context for easier review.\n\n parse-options-cb.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 1240a8514e..c2062ae742 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -161,17 +161,18 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n\n struct option *parse_options_dup(const struct option *o)\n {\n+\tconst struct option *orig = o;\n \tstruct option *opts;\n \tint nr = 0;\n\n \twhile (o && o->type != OPTION_END) {\n \t\tnr++;\n \t\to++;\n \t}\n\n \tALLOC_ARRAY(opts, nr + 1);\n-\tmemcpy(opts, o - nr, sizeof(*o) * nr);\n+\tCOPY_ARRAY(opts, orig, nr);\n \tmemset(opts + nr, 0, sizeof(*opts));\n \topts[nr].type = OPTION_END;\n \treturn opts;\n }\n--\n2.24.0\n"},{"id":"386086","messageId":"xmqq5zjocx7i.fsf@gitster-ct.c.googlers.com","threadId":"52254","inReplyTo":"39a0b622-f725-9284-ea50-19cf4078209d@web.de","subject":"Re: parse-options: avoid arithmetic on pointer that's potentially NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T02:43:45Z","receivedAt":"2019-11-13T02:43:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> parse_options_dup() counts the number of elements in the given array\n> without the end marker, allocates enough memory to hold all of them plus\n> an end marker, then copies them and terminates the new array.  The\n> counting part is done by advancing a pointer through the array, and the\n> original pointer is reconstructed using pointer subtraction before the\n> copy operation.\n>\n> The function is also prepared to handle a NULL pointer passed to it.\n> None of its callers do that currently, but this feature was used by\n> 46e91b663b (\"checkout: split part of it to new command 'restore'\",\n> 2019-04-25); it seems worth keeping.\n\n... meaning it was used but since then we rewrote the user and there\nis no caller that uses it?\n\nGee, how are you finding such an instance of use?  I am quite\nimpressed.\n\n> It ends up doing arithmetic on that NULL pointer, though, which is\n> undefined in standard C, when it tries to calculate \"NULL - 0\".  Better\n> avoid doing that by remembering the originally given pointer value.\n>\n> There is another issue, though.  memcpy(3) does not support NULL\n> pointers, even for empty arrays.  Use COPY_ARRAY instead, which does\n> support such empty arrays.  Its call is also shorter and safer by\n> inferring the element type automatically.\n\nNicely explained.\n\n>  struct option *parse_options_dup(const struct option *o)\n>  {\n> +\tconst struct option *orig = o;\n\nIf I were doing this patch, I'd give the incoming parameter a more\nmeaningful name and the temporary/local variable that is used to\nscan would get/keep the shorter name.  IOW\n\n\tstruct parse_options_dup(const struct option *original)\n\t{\n\t\tconst struct option *o = original;\n\t\tstruct option *copied;\n\t\tint nr = 0;\n\n\t\t... loop to count ...\n\t\tALLOC_ARRAY(copied, nr + 1);\n\t\tCOPY_ARRAY(copied, original, nr);\n\t\t...\n\nBut your patch is a strict improvement from the current code.  Will\nqueue without any tweak.\n\nThanks.\n\n>  \tstruct option *opts;\n>  \tint nr = 0;\n>\n>  \twhile (o && o->type != OPTION_END) {\n>  \t\tnr++;\n>  \t\to++;\n>  \t}\n>\n>  \tALLOC_ARRAY(opts, nr + 1);\n> -\tmemcpy(opts, o - nr, sizeof(*o) * nr);\n> +\tCOPY_ARRAY(opts, orig, nr);\n>  \tmemset(opts + nr, 0, sizeof(*opts));\n>  \topts[nr].type = OPTION_END;\n>  \treturn opts;\n>  }\n> --\n> 2.24.0\n"},{"id":"386126","messageId":"df941c25-22ea-3523-9607-8e892dea320c@web.de","threadId":"52254","inReplyTo":"xmqq5zjocx7i.fsf@gitster-ct.c.googlers.com","subject":"Re: parse-options: avoid arithmetic on pointer that's potentially NULL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-13T16:48:47Z","receivedAt":"2019-11-13T16:49:18Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.11.19 um 03:43 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> The function is also prepared to handle a NULL pointer passed to it.\n>> None of its callers do that currently, but this feature was used by\n>> 46e91b663b (\"checkout: split part of it to new command 'restore'\",\n>> 2019-04-25); it seems worth keeping.\n>\n> ... meaning it was used but since then we rewrote the user and there\n> is no caller that uses it?\n>\n> Gee, how are you finding such an instance of use?  I am quite\n> impressed.\n\nI wondered why the function checks for NULL, noticed that both users are\nin builtin/checkout.c and pass a pointer to a struct, so I ran\n\"git log -p builtin/checkout.c\" and searched for \"= NULL\" in its output\nbecause I suspected or vaguely remembered that the options were added\none by one by a patch series.  Sloppy, but good enough in this case..\n\n>>  struct option *parse_options_dup(const struct option *o)\n>>  {\n>> +\tconst struct option *orig = o;\n>\n> If I were doing this patch, I'd give the incoming parameter a more\n> meaningful name and the temporary/local variable that is used to\n> scan would get/keep the shorter name.  IOW\n>\n> \tstruct parse_options_dup(const struct option *original)\n> \t{\n> \t\tconst struct option *o = original;\n> \t\tstruct option *copied;\n> \t\tint nr = 0;\n>\n> \t\t... loop to count ...\n> \t\tALLOC_ARRAY(copied, nr + 1);\n> \t\tCOPY_ARRAY(copied, original, nr);\n> \t\t...\n\nRight.  And I just rediscovered an unsent patch series from last summer\nthat refactors this function further.  It fixed the NULL issue as well,\nbut only by accident.  Will rethink/reintegrate these patches and send\nthem later, but here's one that's ready and fits the COPY_ARRAY theme:\n\n-- >8 --\nSubject: [PATCH] parse-options: use COPY_ARRAY in parse_options_concat()\n\nUse COPY_ARRAY to copy whole arrays instead of iterating through their\nelements.  That's shorter, simpler and bit more efficient.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options-cb.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex c2062ae742..8b443938b8 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -188,10 +188,8 @@ struct option *parse_options_concat(struct option *a, struct option *b)\n \t\tb_len++;\n\n \tALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));\n-\tfor (i = 0; i < a_len; i++)\n-\t\tret[i] = a[i];\n-\tfor (i = 0; i < b_len; i++)\n-\t\tret[a_len + i] = b[i];\n+\tCOPY_ARRAY(ret, a, a_len);\n+\tCOPY_ARRAY(ret + a_len, b, b_len);\n \tret[a_len + b_len] = b[b_len]; /* final OPTION_END */\n\n \treturn ret;\n--\n2.24.0\n"},{"id":"386127","messageId":"20191113182548.1707-1-martin.agren@gmail.com","threadId":"52254","inReplyTo":"df941c25-22ea-3523-9607-8e892dea320c@web.de","subject":"Re: [PATCH] parse-options: use COPY_ARRAY in parse_options_concat()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:25:48Z","receivedAt":"2019-11-13T18:26:10Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 13 Nov 2019 at 18:19, René Scharfe <l.s.r@web.de> wrote:\n>         ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));\n> -       for (i = 0; i < a_len; i++)\n> -               ret[i] = a[i];\n> -       for (i = 0; i < b_len; i++)\n> -               ret[a_len + i] = b[i];\n> +       COPY_ARRAY(ret, a, a_len);\n> +       COPY_ARRAY(ret + a_len, b, b_len);\n>         ret[a_len + b_len] = b[b_len]; /* final OPTION_END */\n\nMaybe include that last one in the COPY_ARRAY with something like this\non top?\n\n-       COPY_ARRAY(ret + a_len, b, b_len);\n-       ret[a_len + b_len] = b[b_len]; /* final OPTION_END */\n+       /* 1 more to include the final OPTION_END */\n+       COPY_ARRAY(ret + a_len, b, st_add(b_len, 1));\n\nOr maybe even something like the below (directly on top of your patch)?\n\n(Plus, you could drop `i` entirely, but I'm not sure that's worth\ngolfing on.)\n\nMartin\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 2bde78b726..11196cfb96 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -185,11 +185,11 @@ struct option *parse_options_concat(struct option *a, struct option *b)\n \t\ta_len++;\n \tfor (i = 0; b[i].type != OPTION_END; i++)\n \t\tb_len++;\n+\tb_len++; /* final OPTION_END */\n \n-\tALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));\n+\tALLOC_ARRAY(ret, st_add(a_len, b_len));\n \tCOPY_ARRAY(ret, a, a_len);\n \tCOPY_ARRAY(ret + a_len, b, b_len);\n-\tret[a_len + b_len] = b[b_len]; /* final OPTION_END */\n \n \treturn ret;\n }\n-- \n2.24.0\n\n"},{"id":"386185","messageId":"686d3b30-9f1e-a871-7dab-e35f1d6d1499@web.de","threadId":"52254","inReplyTo":"20191113182548.1707-1-martin.agren@gmail.com","subject":"Re: [PATCH] parse-options: use COPY_ARRAY in parse_options_concat()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-14T16:01:24Z","receivedAt":"2019-11-14T16:01:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.11.19 um 19:25 schrieb Martin Ågren:\n> On Wed, 13 Nov 2019 at 18:19, René Scharfe <l.s.r@web.de> wrote:\n>>         ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));\n>> -       for (i = 0; i < a_len; i++)\n>> -               ret[i] = a[i];\n>> -       for (i = 0; i < b_len; i++)\n>> -               ret[a_len + i] = b[i];\n>> +       COPY_ARRAY(ret, a, a_len);\n>> +       COPY_ARRAY(ret + a_len, b, b_len);\n>>         ret[a_len + b_len] = b[b_len]; /* final OPTION_END */\n>\n> Maybe include that last one in the COPY_ARRAY with something like this\n> on top?\n>\n> -       COPY_ARRAY(ret + a_len, b, b_len);\n> -       ret[a_len + b_len] = b[b_len]; /* final OPTION_END */\n> +       /* 1 more to include the final OPTION_END */\n> +       COPY_ARRAY(ret + a_len, b, st_add(b_len, 1));\n\nI'd rather move towards allowing a and b to be NULL, for consistency and\nsafety.  Will send patches eventually, but preferably only once the\noriginal patch hits master.  These follow-ups that I have in mind aren't\nterribly urgent.\n\nRené\n"}]}