git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] bundle: don't segfault on "git bundle <subcmd>"

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Dec 20, 2022, 13:40 UTC
Message-ID
<patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com>
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
Previous: Ævar Arnfjörð BjarmasonNext: Hubert Jasudowicz
Message 4 of 11 in “Revert "builtin/bundle.c: let parse-options parse subcommands"”
  1. Revert "builtin/bundle.c: let parse-options parse subcommands"Hubert Jasudowicz, Dec 20, 2022
  2. Derrick StoleeDec 20, 2022
  3. Ævar Arnfjörð BjarmasonDec 20, 2022
  4. bundle: don't segfault on "git bundle <subcmd>"Ævar Arnfjörð Bjarmason, Dec 20, 2022
  5. Hubert JasudowiczDec 20, 2022
  6. Junio C HamanoDec 25, 2022
  7. Junio C HamanoDec 25, 2022
  8. 0/2 builtin/bundle.c: segfault fix style & error reporting follow-upÆvar Arnfjörð Bjarmason, Dec 27, 2022
  9. 1/2 builtin/bundle.c: remove superfluous "newargc" variableÆvar Arnfjörð Bjarmason, Dec 27, 2022
  10. 2/2 bundle <cmd>: have usage_msg_opt() note the missing "<file>"Ævar Arnfjörð Bjarmason, Dec 27, 2022
  11. Junio C HamanoDec 27, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.