| Message ID | 20200520232627.30901-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] bisect--helper: avoid segfault with bad syntax in `start --term-*` | expand |
On Thu, May 21, 2020 at 1:31 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > > 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C, > 2019-01-02) adds a lax parser for `git bisect start` which could result > in a segfault under a bad syntax call for start with custom terms. > > Detect if there are enough arguments left in the command line to use for > --term-{old,good,new,bad} and abort with the same syntax error the original > implementation will show if not. > > While at it, remove an unnecessary (and incomplete) check for unknown > arguments and make sure to add a test to avoid regressions. This looks good to me! Thanks, Christian.
Christian Couder <christian.couder@gmail.com> writes: > On Thu, May 21, 2020 at 1:31 AM Carlo Marcelo Arenas Belón > <carenas@gmail.com> wrote: >> >> 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C, >> 2019-01-02) adds a lax parser for `git bisect start` which could result >> in a segfault under a bad syntax call for start with custom terms. >> >> Detect if there are enough arguments left in the command line to use for >> --term-{old,good,new,bad} and abort with the same syntax error the original >> implementation will show if not. >> >> While at it, remove an unnecessary (and incomplete) check for unknown >> arguments and make sure to add a test to avoid regressions. > > This looks good to me! Thanks, both.
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index c1c40b516d..ec4996282e 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -455,9 +455,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout, no_checkout = 1; } else if (!strcmp(arg, "--term-good") || !strcmp(arg, "--term-old")) { + i++; + if (argc <= i) + return error(_("'' is not a valid term")); must_write_terms = 1; free((void *) terms->term_good); - terms->term_good = xstrdup(argv[++i]); + terms->term_good = xstrdup(argv[i]); } else if (skip_prefix(arg, "--term-good=", &arg) || skip_prefix(arg, "--term-old=", &arg)) { must_write_terms = 1; @@ -465,16 +468,18 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout, terms->term_good = xstrdup(arg); } else if (!strcmp(arg, "--term-bad") || !strcmp(arg, "--term-new")) { + i++; + if (argc <= i) + return error(_("'' is not a valid term")); must_write_terms = 1; free((void *) terms->term_bad); - terms->term_bad = xstrdup(argv[++i]); + terms->term_bad = xstrdup(argv[i]); } else if (skip_prefix(arg, "--term-bad=", &arg) || skip_prefix(arg, "--term-new=", &arg)) { must_write_terms = 1; free((void *) terms->term_bad); terms->term_bad = xstrdup(arg); - } else if (starts_with(arg, "--") && - !one_of(arg, "--term-good", "--term-bad", NULL)) { + } else if (starts_with(arg, "--")) { return error(_("unrecognized option: '%s'"), arg); } else { char *commit_id = xstrfmt("%s^{commit}", arg); diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 821a0c88cf..f7ef15a384 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -859,7 +859,9 @@ test_expect_success 'bisect cannot mix terms' ' test_expect_success 'bisect terms rejects invalid terms' ' git bisect reset && + test_must_fail git bisect start --term-good && test_must_fail git bisect start --term-good invalid..term && + test_must_fail git bisect start --term-bad && test_must_fail git bisect terms --term-bad invalid..term && test_must_fail git bisect terms --term-good bad && test_must_fail git bisect terms --term-good old &&
06f5608c14 (bisect--helper: `bisect_start` shell function partially in C, 2019-01-02) adds a lax parser for `git bisect start` which could result in a segfault under a bad syntax call for start with custom terms. Detect if there are enough arguments left in the command line to use for --term-{old,good,new,bad} and abort with the same syntax error the original implementation will show if not. While at it, remove an unnecessary (and incomplete) check for unknown arguments and make sure to add a test to avoid regressions. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v2: * nicer implementation (per Junio) and description (per Eric) * add test cases builtin/bisect--helper.c | 13 +++++++++---- t/t6030-bisect-porcelain.sh | 2 ++ 2 files changed, 11 insertions(+), 4 deletions(-)