{"thread":{"id":"53514","subject":"[PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+","startedAt":"2020-05-20T19:52:30Z","lastAt":"2020-05-24T16:00:44Z","messageCount":6,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Eric Sunshine","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"398291","messageId":"20200520195214.62655-1-carenas@gmail.com","threadId":"53514","inReplyTo":null,"subject":"[PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-20T19:52:14Z","receivedAt":"2020-05-20T19:52:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"06f5608c14 (bisect--helper: `bisect_start` shell function partially in C,\n2019-01-02) adds a lax parser for `git bisect start` which could result\nin a segfault under a bad syntax call.\n\nDetect if they are enough arguments left in the command line to use for\n--term-{old,good,new,bad} and throw the same syntax error the old shell\nscript will show if not.\n\nWhile at it, remove and unnecessary (and incomplete) check for unknown\narguments.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n builtin/bisect--helper.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c1c40b516d..37db7d2afa 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -455,6 +455,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tno_checkout = 1;\n \t\t} else if (!strcmp(arg, \"--term-good\") ||\n \t\t\t !strcmp(arg, \"--term-old\")) {\n+\t\t\tif (argc - i <= 1)\n+\t\t\t\treturn error(_(\"'' is not a valid term\"));\n \t\t\tmust_write_terms = 1;\n \t\t\tfree((void *) terms->term_good);\n \t\t\tterms->term_good = xstrdup(argv[++i]);\n@@ -465,6 +467,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tterms->term_good = xstrdup(arg);\n \t\t} else if (!strcmp(arg, \"--term-bad\") ||\n \t\t\t !strcmp(arg, \"--term-new\")) {\n+\t\t\tif (argc - i <= 1)\n+\t\t\t\treturn error(_(\"'' is not a valid term\"));\n \t\t\tmust_write_terms = 1;\n \t\t\tfree((void *) terms->term_bad);\n \t\t\tterms->term_bad = xstrdup(argv[++i]);\n@@ -473,8 +477,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tmust_write_terms = 1;\n \t\t\tfree((void *) terms->term_bad);\n \t\t\tterms->term_bad = xstrdup(arg);\n-\t\t} else if (starts_with(arg, \"--\") &&\n-\t\t\t !one_of(arg, \"--term-good\", \"--term-bad\", NULL)) {\n+\t\t} else if (starts_with(arg, \"--\")) {\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n \t\t} else {\n \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-- \n2.27.0.rc0.187.gede8c892b8\n\n"},{"id":"398294","messageId":"xmqqimgquznj.fsf@gitster.c.googlers.com","threadId":"53514","inReplyTo":"20200520195214.62655-1-carenas@gmail.com","subject":"Re: [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-20T20:20:16Z","receivedAt":"2020-05-20T20:20:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C,\n> 2019-01-02) adds a lax parser for `git bisect start` which could result\n> in a segfault under a bad syntax call.\n>\n> Detect if they are enough arguments left in the command line to use for\n> --term-{old,good,new,bad} and throw the same syntax error the old shell\n> script will show if not.\n\nThanks for a quick discovery and fix.  The bug is more than a year\nold---I guess nobody uses custom bisect terms?\n\n> While at it, remove and unnecessary (and incomplete) check for unknown\n> arguments.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  builtin/bisect--helper.c | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index c1c40b516d..37db7d2afa 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -455,6 +455,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n>  \t\t\tno_checkout = 1;\n>  \t\t} else if (!strcmp(arg, \"--term-good\") ||\n>  \t\t\t !strcmp(arg, \"--term-old\")) {\n> +\t\t\tif (argc - i <= 1)\n> +\t\t\t\treturn error(_(\"'' is not a valid term\"));\n>  \t\t\tmust_write_terms = 1;\n>  \t\t\tfree((void *) terms->term_good);\n>  \t\t\tterms->term_good = xstrdup(argv[++i]);\n\nAs the variable that counts up in the loop is \"i\", it is easier to\nmake the condition about \"i\", not about \"difference between argc and\ni\", e.g.\n\n\t\t\tif (argc - 1 <= i)\n\t\t\t\treturn error(...)\n\ni.e. \"The variable 'i' approached from 0 toward argc, and it went\npast (argc - 1) already.\"  I used \"<=\" so that \"went past\" is more\nobvious (i.e. imagine a number-line where things on the left hand\nside are smaller than things on the right hand side).\n\nI think incrementing 'i' upfront, once we know we want to read one\nmore from argv[], may make it even easier to read:\n\n\t\t} else if (... this is about term-good or term-old ...) {\n\t\t\ti++;\n\t\t\tif (argc <= i)\n\t\t\t\treturn error(... missing arg ...);\n\t\t\t...\n\t\t\tterms->term_good = xstrdup(argv[i]);\n\nThe same comment applies to the handling of bad/new.\n\nThanks.\n"},{"id":"398306","messageId":"CAPig+cT4kv1D6O5XnaihTODnoGtVeDdDmsPLX2g380r4+YK51w@mail.gmail.com","threadId":"53514","inReplyTo":"20200520195214.62655-1-carenas@gmail.com","subject":"Re: [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-20T22:25:51Z","receivedAt":"2020-05-20T22:26:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 20, 2020 at 3:52 PM Carlo Marcelo Arenas Belón\n<carenas@gmail.com> wrote:\n> 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C,\n> 2019-01-02) adds a lax parser for `git bisect start` which could result\n> in a segfault under a bad syntax call.\n>\n> Detect if they are enough arguments left in the command line to use for\n\ns/they/there/\n\n> --term-{old,good,new,bad} and throw the same syntax error the old shell\n> script will show if not.\n>\n> While at it, remove and unnecessary (and incomplete) check for unknown\n\ns/and/an/\n\n> arguments.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n"},{"id":"398308","messageId":"20200520232627.30901-1-carenas@gmail.com","threadId":"53514","inReplyTo":"20200520195214.62655-1-carenas@gmail.com","subject":"[PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*`","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-20T23:26:27Z","receivedAt":"2020-05-20T23:26:43Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"06f5608c14 (bisect--helper: `bisect_start` shell function partially in C,\n2019-01-02) adds a lax parser for `git bisect start` which could result\nin a segfault under a bad syntax call for start with custom terms.\n\nDetect if there are enough arguments left in the command line to use for\n--term-{old,good,new,bad} and abort with the same syntax error the original\nimplementation will show if not.\n\nWhile at it, remove an unnecessary (and incomplete) check for unknown\narguments and make sure to add a test to avoid regressions.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nv2:\n* nicer implementation (per Junio) and description (per Eric)\n* add test cases\n\n builtin/bisect--helper.c    | 13 +++++++++----\n t/t6030-bisect-porcelain.sh |  2 ++\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c1c40b516d..ec4996282e 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -455,9 +455,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tno_checkout = 1;\n \t\t} else if (!strcmp(arg, \"--term-good\") ||\n \t\t\t !strcmp(arg, \"--term-old\")) {\n+\t\t\ti++;\n+\t\t\tif (argc <= i)\n+\t\t\t\treturn error(_(\"'' is not a valid term\"));\n \t\t\tmust_write_terms = 1;\n \t\t\tfree((void *) terms->term_good);\n-\t\t\tterms->term_good = xstrdup(argv[++i]);\n+\t\t\tterms->term_good = xstrdup(argv[i]);\n \t\t} else if (skip_prefix(arg, \"--term-good=\", &arg) ||\n \t\t\t   skip_prefix(arg, \"--term-old=\", &arg)) {\n \t\t\tmust_write_terms = 1;\n@@ -465,16 +468,18 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tterms->term_good = xstrdup(arg);\n \t\t} else if (!strcmp(arg, \"--term-bad\") ||\n \t\t\t !strcmp(arg, \"--term-new\")) {\n+\t\t\ti++;\n+\t\t\tif (argc <= i)\n+\t\t\t\treturn error(_(\"'' is not a valid term\"));\n \t\t\tmust_write_terms = 1;\n \t\t\tfree((void *) terms->term_bad);\n-\t\t\tterms->term_bad = xstrdup(argv[++i]);\n+\t\t\tterms->term_bad = xstrdup(argv[i]);\n \t\t} else if (skip_prefix(arg, \"--term-bad=\", &arg) ||\n \t\t\t   skip_prefix(arg, \"--term-new=\", &arg)) {\n \t\t\tmust_write_terms = 1;\n \t\t\tfree((void *) terms->term_bad);\n \t\t\tterms->term_bad = xstrdup(arg);\n-\t\t} else if (starts_with(arg, \"--\") &&\n-\t\t\t !one_of(arg, \"--term-good\", \"--term-bad\", NULL)) {\n+\t\t} else if (starts_with(arg, \"--\")) {\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n \t\t} else {\n \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 821a0c88cf..f7ef15a384 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -859,7 +859,9 @@ test_expect_success 'bisect cannot mix terms' '\n \n test_expect_success 'bisect terms rejects invalid terms' '\n \tgit bisect reset &&\n+\ttest_must_fail git bisect start --term-good &&\n \ttest_must_fail git bisect start --term-good invalid..term &&\n+\ttest_must_fail git bisect start --term-bad &&\n \ttest_must_fail git bisect terms --term-bad invalid..term &&\n \ttest_must_fail git bisect terms --term-good bad &&\n \ttest_must_fail git bisect terms --term-good old &&\n-- \n2.27.0.rc1.181.g8d5cacc8d1\n\n"},{"id":"398427","messageId":"CAP8UFD1Oa6g4kj8GDhDC-3ry1eFyg-aD+JVDtfK1TkJKuL8v-Q@mail.gmail.com","threadId":"53514","inReplyTo":"20200520232627.30901-1-carenas@gmail.com","subject":"Re: [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*`","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-22T15:49:08Z","receivedAt":"2020-05-22T15:49:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, May 21, 2020 at 1:31 AM Carlo Marcelo Arenas Belón\n<carenas@gmail.com> wrote:\n>\n> 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C,\n> 2019-01-02) adds a lax parser for `git bisect start` which could result\n> in a segfault under a bad syntax call for start with custom terms.\n>\n> Detect if there are enough arguments left in the command line to use for\n> --term-{old,good,new,bad} and abort with the same syntax error the original\n> implementation will show if not.\n>\n> While at it, remove an unnecessary (and incomplete) check for unknown\n> arguments and make sure to add a test to avoid regressions.\n\nThis looks good to me!\n\nThanks,\nChristian.\n"},{"id":"398468","messageId":"xmqqr1v9qq58.fsf@gitster.c.googlers.com","threadId":"53514","inReplyTo":"CAP8UFD1Oa6g4kj8GDhDC-3ry1eFyg-aD+JVDtfK1TkJKuL8v-Q@mail.gmail.com","subject":"Re: [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-24T16:00:35Z","receivedAt":"2020-05-24T16:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Thu, May 21, 2020 at 1:31 AM Carlo Marcelo Arenas Belón\n> <carenas@gmail.com> wrote:\n>>\n>> 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C,\n>> 2019-01-02) adds a lax parser for `git bisect start` which could result\n>> in a segfault under a bad syntax call for start with custom terms.\n>>\n>> Detect if there are enough arguments left in the command line to use for\n>> --term-{old,good,new,bad} and abort with the same syntax error the original\n>> implementation will show if not.\n>>\n>> While at it, remove an unnecessary (and incomplete) check for unknown\n>> arguments and make sure to add a test to avoid regressions.\n>\n> This looks good to me!\n\nThanks, both.\n"}]}