From 891cb09db6c0e6bf11b8175bc5ea5f45493afb85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 20 Dec 2022 14:40:18 +0100 Subject: [PATCH 1/3] bundle: don't segfault on "git bundle " MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Ævar Arnfjörð Bjarmason Tested-by: Hubert Jasudowicz Signed-off-by: Junio C Hamano --- 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 e80efce3a4..13309f2ea9 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -48,7 +48,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 833205125a..3a1cf30b1d 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= From e778ecbcee3a4a14e46c06d66328fdfc0af8780d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 27 Dec 2022 19:39:09 +0100 Subject: [PATCH 2/3] builtin/bundle.c: remove superfluous "newargc" variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As noted in 891cb09db6c (bundle: don't segfault on "git bundle ", 2022-12-20) the "newargc" in this function is redundant to using our own "argc". Let's refactor the function to avoid needlessly introducing another variable. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/bundle.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/bundle.c b/builtin/bundle.c index 13309f2ea9..34b4aa611b 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -45,13 +45,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) { From 6d5e9e53aa46b97227a25c85400fd00b8237411a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 27 Dec 2022 19:39:10 +0100 Subject: [PATCH 3/3] bundle : have usage_msg_opt() note the missing "" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improve the usage we emit on e.g. "git bundle create" to note why we're showing the usage, it's because the "" argument is missing. We know that'll be the case for all parse_options_cmd_bundle() users, as they're passing the "char **bundle_file" parameter, which as the context shows we're expected to populate. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/bundle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/bundle.c b/builtin/bundle.c index 34b4aa611b..4b63574c98 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -48,7 +48,7 @@ static int parse_options_cmd_bundle(int argc, argc = parse_options(argc, argv, NULL, options, usagestr, PARSE_OPT_STOP_AT_NON_OPTION); if (!argc) - usage_with_options(usagestr, options); + usage_msg_opt(_("need a argument"), usagestr, options); *bundle_file = prefix_filename(prefix, argv[0]); return argc; }