[PATCH] git: avoid segfault on "git --shallow-file" without a value

[PATCH] git: avoid segfault on "git --shallow-file" without a value

[Christian Couder]

In "git.c", the other `handle_options()` options that take their value
as a separate argument, like `--git-dir`, `--namespace` or `-C`, check
that such an argument actually exists before using it, and error out
with a message and the usage string otherwise.

The `--shallow-file` option doesn't perform that check. It blindly
advances past the option and then dereferences the next element of
`argv`, which is the NULL terminator when no value was given. So
`git --shallow-file` segfaults:

  $ git --shallow-file
  Segmentation fault (core dumped)

Let's fix that by checking that a value was given, in the same way and
with a message worded like the ones the other options use.

While at it, let's also set the environment variable before advancing
past the option, instead of advancing first and using `(*argv)[0]`, so
that this option looks like the other ones.

Note that all the in-tree callers passing `--shallow-file` to a `git`
subprocess always pass a value after it, so they are not affected. In
`upload-pack.c` that value is an empty string, which is still accepted.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---

While working on modernizing `git fast-import`, I noticed that
`--shallow-file` was handled differently than the other options that
take an argument in "git.c", and found this segfault.

I have started working on a better way to handle such options not only
in "git.c" but also in other files. For now though, I think a small
localized bugfix like this is the simplest solution.

Not sure if "t0041-usage.sh" is the best place for testing this, but I
couldn't find a dedicated one.

CI tests all pass, see:

https://github.com/chriscool/git/actions/runs/31478034826

 git.c            | 10 +++++++---
 t/t0041-usage.sh |  7 +++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index e5f1811b6b..96df15b5cd 100644
--- a/git.c
+++ b/git.c
@@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--shallow-file")) {
-			(*argv)++;
-			(*argc)--;
-			setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);
+			if (*argc < 2) {
+				fprintf(stderr, _("no file given for '%s' option\n" ), "--shallow-file");
+				usage(git_usage_string);
+			}
+			setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[1], 1);
 			if (envchanged)
 				*envchanged = 1;
+			(*argv)++;
+			(*argc)--;
 		} else if (!strcmp(cmd, "-C")) {
 			if (*argc < 2) {
 				fprintf(stderr, _("no directory given for '%s' option\n" ), "-C");
diff --git a/t/t0041-usage.sh b/t/t0041-usage.sh
index 51af7cc030..2a9c5eafca 100755
--- a/t/t0041-usage.sh
+++ b/t/t0041-usage.sh
@@ -107,4 +107,11 @@ test_expect_success 'for-each-ref usage error' '
 	test_grep "usage" actual.err
 '
 
+test_expect_success 'git --shallow-file without a value' '
+	test_must_fail git --shallow-file >actual 2>actual.err &&
+	test_line_count = 0 actual &&
+	test_grep "no file given for " actual.err &&
+	test_grep "usage" actual.err
+'
+
 test_done
-- 
2.55.0.530.gdb3615d990.dirty

Re: [PATCH] git: avoid segfault on "git --shallow-file" without a value

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

A great subject line ;-)  It is the simplest reproducer of any bug.

> In "git.c", the other `handle_options()` options that take their value
> as a separate argument, like `--git-dir`, `--namespace` or `-C`, check
> that such an argument actually exists before using it, and error out
> with a message and the usage string otherwise.
>
> The `--shallow-file` option doesn't perform that check. It blindly
> advances past the option and then dereferences the next element of
> `argv`, which is the NULL terminator when no value was given. So
> `git --shallow-file` segfaults:
>
>   $ git --shallow-file
>   Segmentation fault (core dumped)
> ...
> diff --git a/git.c b/git.c
> index e5f1811b6b..96df15b5cd 100644
> --- a/git.c
> +++ b/git.c
> @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			if (envchanged)
>  				*envchanged = 1;
>  		} else if (!strcmp(cmd, "--shallow-file")) {
> -			(*argv)++;
> -			(*argc)--;
> -			setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);
> +			if (*argc < 2) {
> +				fprintf(stderr, _("no file given for '%s' option\n" ), "--shallow-file");
> +				usage(git_usage_string);
> +			}
> +			setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[1], 1);
>  			if (envchanged)
>  				*envchanged = 1;
> +			(*argv)++;
> +			(*argc)--;

It is curious that the fix needs to be so big, when the only change
necessary, as far as I can tell from your problem description, is to
insert 4 line "if (... not enough args ...) { ... barf and die ...}"
block and without anything else.  I think the culprit is this "while
at it" ...

> While at it, let's also set the environment variable before advancing
> past the option, instead of advancing first and using `(*argv)[0]`, so
> that this option looks like the other ones.

... that made the patch more confusing to read than otherwise.

But without reading the preimage of the patch, the result is just as
understandable ;-)  Let's take the patch as-is.

> +test_expect_success 'git --shallow-file without a value' '
> +	test_must_fail git --shallow-file >actual 2>actual.err &&
> +	test_line_count = 0 actual &&
> +	test_grep "no file given for " actual.err &&
> +	test_grep "usage" actual.err
> +'

Do we have similar "oops, you were supposed to give me a value" test
for other things like "--config-env=", "-C", etc.?  Just being
curious, because (1) if there are, this addition belongs there, not
here, and (2) if there aren't, this addition may not be needed, and
(3) if there aren't or if the existing coverage is incomplete,
perhaps we should give a more complete coverage while at it.

With (3), I mean something along the lines of ...

	for opt in -C -c --git-dir --work-tree --namespace --config-env
	do
		test_expect_success "git $opt without a value" '
			test_must_fail git $opt >actual 2>error &&
			test_line_count 0 actual &&
			test_grep usage error
		'
	done

I do not mean to say that (3) is my favorite among these three,
though.

Re: [PATCH] git: avoid segfault on "git --shallow-file" without a value

[Patrick Steinhardt]

On Tue, Aug 11, 2026 at 02:14:46PM +0200, Christian Couder wrote:
> diff --git a/git.c b/git.c
> index e5f1811b6b..96df15b5cd 100644
> --- a/git.c
> +++ b/git.c
> @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			if (envchanged)
>  				*envchanged = 1;
>  		} else if (!strcmp(cmd, "--shallow-file")) {
> -			(*argv)++;
> -			(*argc)--;
> -			setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);
> +			if (*argc < 2) {
> +				fprintf(stderr, _("no file given for '%s' option\n" ), "--shallow-file");
> +				usage(git_usage_string);

Should we maybe condense this into a single line?

    usage(_("no file given for '%s' option\n")), "--shallow-file")

I think that also printing the usage string is only distracting and
doesn't really give the user a lot of extra context.

Other than that this patch looks good to me, thanks!

Patrick