Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+
@ 2020-05-20 19:52 Carlo Marcelo Arenas Belón
  2020-05-20 20:20 ` Junio C Hamano
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-20 19:52 UTC (permalink / raw)
  To: git; +Cc: gitster, pranit.bauva, Carlo Marcelo Arenas Belón

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(-)

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);
-- 
2.27.0.rc0.187.gede8c892b8


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+
  2020-05-20 19:52 [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+ Carlo Marcelo Arenas Belón
@ 2020-05-20 20:20 ` Junio C Hamano
  2020-05-20 22:25 ` Eric Sunshine
  2020-05-20 23:26 ` [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*` Carlo Marcelo Arenas Belón
  2 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2020-05-20 20:20 UTC (permalink / raw)
  To: Carlo Marcelo Arenas Belón; +Cc: git, pranit.bauva

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+
  2020-05-20 19:52 [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+ Carlo Marcelo Arenas Belón
  2020-05-20 20:20 ` Junio C Hamano
@ 2020-05-20 22:25 ` Eric Sunshine
  2020-05-20 23:26 ` [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*` Carlo Marcelo Arenas Belón
  2 siblings, 0 replies; 6+ messages in thread
From: Eric Sunshine @ 2020-05-20 22:25 UTC (permalink / raw)
  To: Carlo Marcelo Arenas Belón; +Cc: Git List, Junio C Hamano, Pranit Bauva

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*`
  2020-05-20 19:52 [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+ Carlo Marcelo Arenas Belón
  2020-05-20 20:20 ` Junio C Hamano
  2020-05-20 22:25 ` Eric Sunshine
@ 2020-05-20 23:26 ` Carlo Marcelo Arenas Belón
  2020-05-22 15:49   ` Christian Couder
  2 siblings, 1 reply; 6+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-20 23:26 UTC (permalink / raw)
  To: git; +Cc: gitster, pranit.bauva, sunshine, Carlo Marcelo Arenas Belón

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(-)

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 &&
-- 
2.27.0.rc1.181.g8d5cacc8d1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*`
  2020-05-20 23:26 ` [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*` Carlo Marcelo Arenas Belón
@ 2020-05-22 15:49   ` Christian Couder
  2020-05-24 16:00     ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Couder @ 2020-05-22 15:49 UTC (permalink / raw)
  To: Carlo Marcelo Arenas Belón
  Cc: git, Junio C Hamano, Pranit Bauva, Eric Sunshine

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*`
  2020-05-22 15:49   ` Christian Couder
@ 2020-05-24 16:00     ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2020-05-24 16:00 UTC (permalink / raw)
  To: Christian Couder
  Cc: Carlo Marcelo Arenas Belón, git, Pranit Bauva, Eric Sunshine

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 19:52 [PATCH] bisect--helper: avoid segfault with bad syntax in start --term-.+ Carlo Marcelo Arenas Belón
2020-05-20 20:20 ` Junio C Hamano
2020-05-20 22:25 ` Eric Sunshine
2020-05-20 23:26 ` [PATCH v2] bisect--helper: avoid segfault with bad syntax in `start --term-*` Carlo Marcelo Arenas Belón
2020-05-22 15:49   ` Christian Couder
2020-05-24 16:00     ` Junio C Hamano

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git