[PATCH] Revert "builtin/bundle.c: let parse-options parse subcommands"

[PATCH] Revert "builtin/bundle.c: let parse-options parse subcommands"

[Hubert Jasudowicz]

This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.

The change breaks git bundle command. Running any subcommand
results with:

$ git bundle create
Segmentation fault (core dumped)

After reverting the change, everything works correctly.
---
 builtin/bundle.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index c12c09f854..d9b46f8e03 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -206,19 +206,30 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
-	parse_opt_subcommand_fn *fn = NULL;
 	struct option options[] = {
-		OPT_SUBCOMMAND("create", &fn, cmd_bundle_create),
-		OPT_SUBCOMMAND("verify", &fn, cmd_bundle_verify),
-		OPT_SUBCOMMAND("list-heads", &fn, cmd_bundle_list_heads),
-		OPT_SUBCOMMAND("unbundle", &fn, cmd_bundle_unbundle),
 		OPT_END()
 	};
+	int result;
 
 	argc = parse_options(argc, argv, prefix, options, builtin_bundle_usage,
-			     0);
+		PARSE_OPT_STOP_AT_NON_OPTION);
 
 	packet_trace_identity("bundle");
 
-	return !!fn(argc, argv, prefix);
+	if (argc < 2)
+		usage_with_options(builtin_bundle_usage, options);
+
+	else if (!strcmp(argv[0], "create"))
+		result = cmd_bundle_create(argc, argv, prefix);
+	else if (!strcmp(argv[0], "verify"))
+		result = cmd_bundle_verify(argc, argv, prefix);
+	else if (!strcmp(argv[0], "list-heads"))
+		result = cmd_bundle_list_heads(argc, argv, prefix);
+	else if (!strcmp(argv[0], "unbundle"))
+		result = cmd_bundle_unbundle(argc, argv, prefix);
+	else {
+		error(_("Unknown subcommand: %s"), argv[0]);
+		usage_with_options(builtin_bundle_usage, options);
+	}
+	return result ? 1 : 0;
 }
-- 
2.38.1

Re: [PATCH] Revert "builtin/bundle.c: let parse-options parse subcommands"

[Derrick Stolee]

On 12/20/22 7:31 AM, Hubert Jasudowicz wrote:
> This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.
> 
> The change breaks git bundle command. Running any subcommand
> results with:
> 
> $ git bundle create
> Segmentation fault (core dumped)

Could you be more specific? We have tests that verify that
these commands work without a segfault. There must be something
different about your environment that makes the segfault occur.

One thing that I could believe is that you are running 'git
bundle create' outside of a Git repository, and I doubt we have
tests covering that scenario.

Before reverting this change that has been out in two releases,
I recommend adding a test that demonstrates your failure and
then doing a specific update to fix that scenario.

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH] Revert "builtin/bundle.c: let parse-options parse subcommands"

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 20 2022, Derrick Stolee wrote:

> On 12/20/22 7:31 AM, Hubert Jasudowicz wrote:
>> This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.
>> 
>> The change breaks git bundle command. Running any subcommand
>> results with:
>> 
>> $ git bundle create
>> Segmentation fault (core dumped)
>
> Could you be more specific?

I don't think the report can get more specific than (quoting it):

	$ git bundle create
	Segmentation fault (core dumped)

:) Did you try running it?

> We have tests that verify that
> these commands work without a segfault. There must be something
> different about your environment that makes the segfault occur.

We don't have those tests, I submitted an alternate smaller fix in
https://lore.kernel.org/git/patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com/
that adds some.

I think what you're misrecalling here is probably that we have general
tests for running "git <cmd> -h" for all built-in <cmd>, but we don't
have any such tests for running sub-commands.

And even then, that wouldn't catch this, as it's a bespoke segfault in
the bundle code, as it can't handle not getting at least one non-option
argument.

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

[Hubert Jasudowicz]

On 12/20/22 14:40, Ævar Arnfjörð Bjarmason wrote:> 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=

Seems to work. Thanks!

Tested-by: Hubert Jasudowicz <hubertj@stmcyber.pl>

Hubert Jasudowicz

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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;
> 	 }

That would actually make the intent much clearer and if the code
were written to update argc instead of introducing a separate
varilable, this bug would not have happened.

Thanks, will queue (without the clean-up at least for now).

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

[Junio C Hamano]

Hubert Jasudowicz <hubert.jasudowicz@stmcyber.pl> writes:

> Seems to work. Thanks!
>
> Tested-by: Hubert Jasudowicz <hubertj@stmcyber.pl>
>
> Hubert Jasudowicz

Thanks for reporting and testing.

[PATCH 0/2] builtin/bundle.c: segfault fix style & error reporting follow-up

[Ævar Arnfjörð Bjarmason]

On Sun, Dec 25 2022, Junio C Hamano wrote:

> That would actually make the intent much clearer and if the code
> were written to update argc instead of introducing a separate
> varilable, this bug would not have happened.

Here's a couple of general improvements on top of ab/bundle-wo-args,
we should also report to the user that they were missing the <file>
argument, as 2/2 here fixes.

Ævar Arnfjörð Bjarmason (2):
  builtin/bundle.c: remove superfluous "newargc" variable
  bundle <cmd>: have usage_msg_opt() note the missing "<file>"

 builtin/bundle.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.39.0.1106.gf45ba805d1a

[PATCH 1/2] builtin/bundle.c: remove superfluous "newargc" variable

[Ævar Arnfjörð Bjarmason]

As noted in 891cb09db6c (bundle: don't segfault on "git bundle
<subcmd>", 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 <avarab@gmail.com>
---
 builtin/bundle.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 61c76284768..3d1ad220ff8 100644
--- 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) {
-- 
2.39.0.1106.gf45ba805d1a

[PATCH 2/2] bundle <cmd>: have usage_msg_opt() note the missing "<file>"

[Ævar Arnfjörð Bjarmason]

Improve the usage we emit on e.g. "git bundle create" to note why
we're showing the usage, it's because the "<file>" 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 <avarab@gmail.com>
---
 builtin/bundle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 3d1ad220ff8..acceef62001 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -58,7 +58,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 <file> argument"), usagestr, options);
 	*bundle_file = prefix_filename(prefix, argv[0]);
 	return argc;
 }
-- 
2.39.0.1106.gf45ba805d1a

Re: [PATCH 0/2] builtin/bundle.c: segfault fix style & error reporting follow-up

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Here's a couple of general improvements on top of ab/bundle-wo-args,
> we should also report to the user that they were missing the <file>
> argument, as 2/2 here fixes.
>
> Ævar Arnfjörð Bjarmason (2):
>   builtin/bundle.c: remove superfluous "newargc" variable
>   bundle <cmd>: have usage_msg_opt() note the missing "<file>"
>
>  builtin/bundle.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

I have no strong opinions on the second one, but the first one is a
definite improvement.  Will queue both.  Thanks.