bisect--helper: avoid segfault with bad syntax in start --term-.+
diff mbox series

Message ID 20200520195214.62655-1-carenas@gmail.com
State New
Headers show
Series
  • bisect--helper: avoid segfault with bad syntax in start --term-.+
Related show

Commit Message

Carlo Marcelo Arenas Belón May 20, 2020, 7:52 p.m. UTC
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.

Detect if they are enough arguments left in the command line to use for
--term-{old,good,new,bad} and throw the same syntax error the old shell
script will show if not.

While at it, remove and unnecessary (and incomplete) check for unknown
arguments.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/bisect--helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Junio C Hamano May 20, 2020, 8:20 p.m. UTC | #1
Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 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.
>
> Detect if they are enough arguments left in the command line to use for
> --term-{old,good,new,bad} and throw the same syntax error the old shell
> script will show if not.

Thanks for a quick discovery and fix.  The bug is more than a year
old---I guess nobody uses custom bisect terms?

> While at it, remove and unnecessary (and incomplete) check for unknown
> arguments.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/bisect--helper.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c1c40b516d..37db7d2afa 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -455,6 +455,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>  			no_checkout = 1;
>  		} else if (!strcmp(arg, "--term-good") ||
>  			 !strcmp(arg, "--term-old")) {
> +			if (argc - i <= 1)
> +				return error(_("'' is not a valid term"));
>  			must_write_terms = 1;
>  			free((void *) terms->term_good);
>  			terms->term_good = xstrdup(argv[++i]);

As the variable that counts up in the loop is "i", it is easier to
make the condition about "i", not about "difference between argc and
i", e.g.

			if (argc - 1 <= i)
				return error(...)

i.e. "The variable 'i' approached from 0 toward argc, and it went
past (argc - 1) already."  I used "<=" so that "went past" is more
obvious (i.e. imagine a number-line where things on the left hand
side are smaller than things on the right hand side).

I think incrementing 'i' upfront, once we know we want to read one
more from argv[], may make it even easier to read:

		} else if (... this is about term-good or term-old ...) {
			i++;
			if (argc <= i)
				return error(... missing arg ...);
			...
			terms->term_good = xstrdup(argv[i]);

The same comment applies to the handling of bad/new.

Thanks.
Eric Sunshine May 20, 2020, 10:25 p.m. UTC | #2
On Wed, May 20, 2020 at 3:52 PM 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.
>
> Detect if they are enough arguments left in the command line to use for

s/they/there/

> --term-{old,good,new,bad} and throw the same syntax error the old shell
> script will show if not.
>
> While at it, remove and unnecessary (and incomplete) check for unknown

s/and/an/

> arguments.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

Patch
diff mbox series

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c1c40b516d..37db7d2afa 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -455,6 +455,8 @@  static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			no_checkout = 1;
 		} else if (!strcmp(arg, "--term-good") ||
 			 !strcmp(arg, "--term-old")) {
+			if (argc - i <= 1)
+				return error(_("'' is not a valid term"));
 			must_write_terms = 1;
 			free((void *) terms->term_good);
 			terms->term_good = xstrdup(argv[++i]);
@@ -465,6 +467,8 @@  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")) {
+			if (argc - i <= 1)
+				return error(_("'' is not a valid term"));
 			must_write_terms = 1;
 			free((void *) terms->term_bad);
 			terms->term_bad = xstrdup(argv[++i]);
@@ -473,8 +477,7 @@  static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			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);