git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] multi-pack-index: fix potential segfault without sub-command
@ 2021-07-19 17:18 Taylor Blau
  2021-07-21  8:10 ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Taylor Blau @ 2021-07-19 17:18 UTC (permalink / raw)
  To: git; +Cc: avarab

Since cd57bc41bb (builtin/multi-pack-index.c: display usage on
unrecognized command, 2021-03-30) we have used a "usage" label to avoid
having two separate callers of usage_with_options (one when no arguments
are given, and another for unrecognized sub-commands).

But the first caller has been broken since cd57bc41bb, since it will
happily jump to usage without arguments, and then pass argv[0] to the
"unrecognized subcommand" error.

Many compilers will save us from a segfault here, but the end result is
ugly, since it mentions an unrecognized subcommand when we didn't even
pass one, and (on GCC) includes "(null)" in its output.

Move the "usage" label down past the error about unrecognized
subcommands so that it is only triggered when it should be. While we're
at it, bulk up our test coverage in this area, too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Noticed this while I was reading code in a similar area while reviewing
one of Ævar's series.

 builtin/multi-pack-index.c  | 2 +-
 t/t5319-multi-pack-index.sh | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 5d3ea445fd..8ff0dee2ec 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -176,8 +176,8 @@ int cmd_multi_pack_index(int argc, const char **argv,
 	else if (!strcmp(argv[0], "expire"))
 		return cmd_multi_pack_index_expire(argc, argv);
 	else {
-usage:
 		error(_("unrecognized subcommand: %s"), argv[0]);
+usage:
 		usage_with_options(builtin_multi_pack_index_usage,
 				   builtin_multi_pack_index_options);
 	}
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 5641d158df..dab7123b3a 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -824,4 +824,9 @@ test_expect_success 'load reverse index when missing .idx, .pack' '
 	)
 '

+test_expect_success 'usage shown without sub-command' '
+	test_expect_code 129 git multi-pack-index 2>err &&
+	! test_i18ngrep "unrecognized subcommand" err
+'
+
 test_done
--
2.31.1.163.ga65ce7f831

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

* Re: [PATCH] multi-pack-index: fix potential segfault without sub-command
  2021-07-19 17:18 [PATCH] multi-pack-index: fix potential segfault without sub-command Taylor Blau
@ 2021-07-21  8:10 ` Jeff King
  2021-07-21 16:52   ` [PATCH v2] " Taylor Blau
  0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2021-07-21  8:10 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, avarab

On Mon, Jul 19, 2021 at 01:18:49PM -0400, Taylor Blau wrote:

> Since cd57bc41bb (builtin/multi-pack-index.c: display usage on
> unrecognized command, 2021-03-30) we have used a "usage" label to avoid
> having two separate callers of usage_with_options (one when no arguments
> are given, and another for unrecognized sub-commands).
> 
> But the first caller has been broken since cd57bc41bb, since it will
> happily jump to usage without arguments, and then pass argv[0] to the
> "unrecognized subcommand" error.
> 
> Many compilers will save us from a segfault here, but the end result is
> ugly, since it mentions an unrecognized subcommand when we didn't even
> pass one, and (on GCC) includes "(null)" in its output.
> 
> Move the "usage" label down past the error about unrecognized
> subcommands so that it is only triggered when it should be. While we're
> at it, bulk up our test coverage in this area, too.

Good find. The code change seems obviously correct.

> +test_expect_success 'usage shown without sub-command' '
> +	test_expect_code 129 git multi-pack-index 2>err &&
> +	! test_i18ngrep "unrecognized subcommand" err
> +'

I think we're avoiding test_i18ngrep in new code these days.

-Peff

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

* [PATCH v2] multi-pack-index: fix potential segfault without sub-command
  2021-07-21  8:10 ` Jeff King
@ 2021-07-21 16:52   ` Taylor Blau
  2021-07-23  7:34     ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Taylor Blau @ 2021-07-21 16:52 UTC (permalink / raw)
  To: git; +Cc: peff, avarab

Since cd57bc41bb (builtin/multi-pack-index.c: display usage on
unrecognized command, 2021-03-30) we have used a "usage" label to avoid
having two separate callers of usage_with_options (one when no arguments
are given, and another for unrecognized sub-commands).

But the first caller has been broken since cd57bc41bb, since it will
happily jump to usage without arguments, and then pass argv[0] to the
"unrecognized subcommand" error.

Many compilers will save us from a segfault here, but the end result is
ugly, since it mentions an unrecognized subcommand when we didn't even
pass one, and (on GCC) includes "(null)" in its output.

Move the "usage" label down past the error about unrecognized
subcommands so that it is only triggered when it should be. While we're
at it, bulk up our test coverage in this area, too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
I didn't realize that we are veering away from test_i18ngrep in new
tests, so here's a version of this trivial patch with grep instead.

Range-diff against v1:
1:  8c0bb3e0dc ! 1:  1280413042 multi-pack-index: fix potential segfault without sub-command
    @@ t/t5319-multi-pack-index.sh: test_expect_success 'load reverse index when missin

     +test_expect_success 'usage shown without sub-command' '
     +	test_expect_code 129 git multi-pack-index 2>err &&
    -+	! test_i18ngrep "unrecognized subcommand" err
    ++	! grep "unrecognized subcommand" err
     +'
     +
      test_done

 builtin/multi-pack-index.c  | 2 +-
 t/t5319-multi-pack-index.sh | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 5d3ea445fd..8ff0dee2ec 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -176,8 +176,8 @@ int cmd_multi_pack_index(int argc, const char **argv,
 	else if (!strcmp(argv[0], "expire"))
 		return cmd_multi_pack_index_expire(argc, argv);
 	else {
-usage:
 		error(_("unrecognized subcommand: %s"), argv[0]);
+usage:
 		usage_with_options(builtin_multi_pack_index_usage,
 				   builtin_multi_pack_index_options);
 	}
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 7609f1ea64..c0ec5a6fd3 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -837,4 +837,9 @@ test_expect_success 'load reverse index when missing .idx, .pack' '
 	)
 '

+test_expect_success 'usage shown without sub-command' '
+	test_expect_code 129 git multi-pack-index 2>err &&
+	! grep "unrecognized subcommand" err
+'
+
 test_done
--
2.31.1.163.ga65ce7f831

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

* Re: [PATCH v2] multi-pack-index: fix potential segfault without sub-command
  2021-07-21 16:52   ` [PATCH v2] " Taylor Blau
@ 2021-07-23  7:34     ` Jeff King
  0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2021-07-23  7:34 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Junio C Hamano, git, avarab

On Wed, Jul 21, 2021 at 12:52:37PM -0400, Taylor Blau wrote:

> I didn't realize that we are veering away from test_i18ngrep in new
> tests, so here's a version of this trivial patch with grep instead.

This looks fine to me, but it looks like Junio may have already merged
the original into 'next' via 995cb54b5b (Merge branch 'tb/reverse-midx'
into next, 2021-07-20).

So we'd have to update it on top. I think we can just leave it be,
though. There's nothing incorrect about using test_i18ngrep for now, and
there are a ton of existing instances to handle if we choose to do a
bulk cleanup. One more isn't a big deal.

-Peff

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

end of thread, other threads:[~2021-07-23  7:34 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 17:18 [PATCH] multi-pack-index: fix potential segfault without sub-command Taylor Blau
2021-07-21  8:10 ` Jeff King
2021-07-21 16:52   ` [PATCH v2] " Taylor Blau
2021-07-23  7:34     ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

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

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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