git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Hubert Jasudowicz" <hubertj@stmcyber.pl>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH] bundle: don't segfault on "git bundle <subcmd>"
Date: Tue, 20 Dec 2022 14:40:18 +0100	[thread overview]
Message-ID: <patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com> (raw)
In-Reply-To: <20221220123142.812965-1-hubertj@stmcyber.pl>

Since aef7d75e580 (builtin/bundle.c: let parse-options parse
subcommands, 2022-08-19) we've been segfaulting if no argument was
provided.

The fix is easy, as all of the "git bundle" subcommands require a
non-option argument we can check that we have arguments left after
calling parse-options().

This makes use of code added in 73c3253d75e (bundle: framework for
options before bundle file, 2019-11-10), before this change that code
has always been unreachable. In 73c3253d75e we'd never reach it as we
already checked "argc < 2" in cmd_bundle() itself.

Then when aef7d75e580 (whose segfault we're fixing here) migrated this
code to the subcommand API it removed that "argc < 2" check, but we
were still checking the wrong "argc" in parse_options_cmd_bundle(), we
need to check the "newargc". The "argc" will always be >= 1, as it
will necessarily contain at least the subcommand name
itself (e.g. "create").

As an aside, this could be safely squashed into this, but let's not do
that for this minimal segfault fix, as it's an unrelated refactoring:

	--- a/builtin/bundle.c
	+++ b/builtin/bundle.c
	@@ -55,13 +55,12 @@ static int parse_options_cmd_bundle(int argc,
	 		const char * const usagestr[],
	 		const struct option options[],
	 		char **bundle_file) {
	-	int newargc;
	-	newargc = parse_options(argc, argv, NULL, options, usagestr,
	+	argc = parse_options(argc, argv, NULL, options, usagestr,
	 			     PARSE_OPT_STOP_AT_NON_OPTION);
	-	if (!newargc)
	+	if (!argc)
	 		usage_with_options(usagestr, options);
	 	*bundle_file = prefix_filename(prefix, argv[0]);
	-	return newargc;
	+	return argc;
	 }

	 static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {

Reported-by: Hubert Jasudowicz <hubertj@stmcyber.pl>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A proposed replacement for
https://lore.kernel.org/git/20221220123142.812965-1-hubertj@stmcyber.pl/;
let's move forward & add a test rather than reverting away from the
subcommand APIe

 builtin/bundle.c       | 2 +-
 t/t6020-bundle-misc.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index c12c09f8549..61c76284768 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -58,7 +58,7 @@ static int parse_options_cmd_bundle(int argc,
 	int newargc;
 	newargc = parse_options(argc, argv, NULL, options, usagestr,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
-	if (argc < 1)
+	if (!newargc)
 		usage_with_options(usagestr, options);
 	*bundle_file = prefix_filename(prefix, argv[0]);
 	return newargc;
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 833205125ab..3a1cf30b1d7 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bundle.sh
 
+for cmd in create verify list-heads unbundle
+do
+	test_expect_success "usage: git bundle $cmd needs an argument" '
+		test_expect_code 129 git bundle $cmd
+	'
+done
+
 # Create a commit or tag and set the variable with the object ID.
 test_commit_setvar () {
 	notick=
-- 
2.39.0.1106.g08bce9674be


  parent reply	other threads:[~2022-12-20 13:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-20 12:31 [PATCH] Revert "builtin/bundle.c: let parse-options parse subcommands" Hubert Jasudowicz
2022-12-20 13:30 ` Derrick Stolee
2022-12-20 13:42   ` Ævar Arnfjörð Bjarmason
2022-12-20 13:40 ` Ævar Arnfjörð Bjarmason [this message]
2022-12-20 13:50   ` [PATCH] bundle: don't segfault on "git bundle <subcmd>" Hubert Jasudowicz
2022-12-25 11:06     ` Junio C Hamano
2022-12-25 11:05   ` Junio C Hamano
2022-12-27 18:39     ` [PATCH 0/2] builtin/bundle.c: segfault fix style & error reporting follow-up Ævar Arnfjörð Bjarmason
2022-12-27 18:39       ` [PATCH 1/2] builtin/bundle.c: remove superfluous "newargc" variable Ævar Arnfjörð Bjarmason
2022-12-27 18:39       ` [PATCH 2/2] bundle <cmd>: have usage_msg_opt() note the missing "<file>" Ævar Arnfjörð Bjarmason
2022-12-27 23:32       ` [PATCH 0/2] builtin/bundle.c: segfault fix style & error reporting follow-up Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=hubertj@stmcyber.pl \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).