bisect--helper: avoid segfault with bad syntax in `start --term-*`

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>
Acked-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 22e669e3b170f96c046a2aa4aa2c289a0599b6f0..63b3a8389ccd69cc59fde034d5bd523773bc1d86 100644 (file)
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -452,9 +452,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;
@@ -462,16 +465,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 55835ee4a47158e1f7aeaa9aeb393f1f3d619791..1ebf5307697cbc1e3872d37a3dc57951c344d31a 100755 (executable)
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -858,7 +858,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 &&