[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

[Christian Couder]

On Tue, Aug 11, 2026 at 9:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

[...]

> >   $ 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.

Sorry but the goal was to use similar code as other options that can
be passed a value like "--git-dir", "--namespace", "--work-tree", and
so on.

> 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.?

In "t/t1300-config.sh" there is:

test_expect_success 'git --config-env with missing value' '
        test_must_fail env ENVVAR=value git --config-env 2>error &&
        test_grep "no config key given for --config-env" error &&
        test_must_fail env ENVVAR=value git --config-env config
core.name 2>error &&
        test_grep "invalid config format: config" error
'

I couldn't find anything else.

> Just being
> curious, because (1) if there are, this addition belongs there, not
> here,

I am not sure the `git --shallow-file` test belongs to "t/t1300-config.sh".

Maybe the new test for --shallow-file with no value should be at the
same place as other tests for --shallow-file, unfortunately there are
no such tests. It looks like this is an undocumented and internal only
option which is only tested indirectly in the following files:

- t5311-pack-bitmaps-shallow.sh
- t5537-fetch-shallow.sh
- t5538-push-shallow.sh
- t5539-fetch-http-shallow.sh
- t5542-push-http-shallow.sh
- t5614-clone-submodules-shallow.sh

> 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.

I am fine with (2) or (3), but they don't seem much better to me than
the test already in this patch.

Thanks.

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

[Junio C Hamano]

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

>> 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.
>
> I am fine with (2) or (3), but they don't seem much better to me than
> the test already in this patch.

I think this is the case between (1) and (2), there is not much
coverage, and there is no coverage specific to "git potty" options.

The 't0041' test is a suitable place if we eventually aim for more
complete coverage such as (3), instead of piecemeal tests, such as
'test --config option with other config-related things in t1300' and
'test --shallow-file option with other shallow-related things in
t????'.  So I think the patch is fine as-is.  I will just leave a
'#leftoverbits' comment here to remind others to consider whether it
is worth extending the test to cover more 'git potty' options for
completeness in the future.

Thanks.

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

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

[Christian Couder]

On Wed, Aug 12, 2026 at 1:22 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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.

The goal of this patch is to fix the bug by using the same code as the
other options that can be passed a value like "--git-dir",
"--namespace", "--work-tree", and so on. Now all these options use the
same pattern for the error message:

git grep -A3 'if (\*argc < 2)' git.c
git.c:                  if (*argc < 2) {
git.c-                          fprintf(stderr, _("no directory given
for '%s' option\n" ), "--git-dir");
git.c-                          usage(git_usage_string);
git.c-                  }
--
git.c:                  if (*argc < 2) {
git.c-                          fprintf(stderr, _("no namespace given
for --namespace\n" ));
git.c-                          usage(git_usage_string);
git.c-                  }
--
git.c:                  if (*argc < 2) {
git.c-                          fprintf(stderr, _("no directory given
for '%s' option\n" ), "--work-tree");
git.c-                          usage(git_usage_string);
git.c-                  }
--
git.c:                  if (*argc < 2) {
git.c-                          fprintf(stderr, _("-c expects a
configuration string\n" ));
git.c-                          usage(git_usage_string);
git.c-                  }
--
git.c:                  if (*argc < 2) {
git.c-                          fprintf(stderr, _("no config key given
for --config-env\n" ));
git.c-                          usage(git_usage_string);
git.c-                  }
--
git.c:                  if (*argc < 2) {
git.c-                          fprintf(stderr, _("no directory given
for '%s' option\n" ), "-C");
git.c-                          usage(git_usage_string);
git.c-                  }
--
git.c:                  if (*argc < 2) {
git.c-                          fprintf(stderr, _("no attribute source
given for --attr-source\n" ));
git.c-                          usage(git_usage_string);
git.c-                  }

So I don't think it makes sense for "--shallow-file" to not be
consistent with these other options.

I could perhaps add a patch to the series to convert all of these to
something like what you suggest, but it could also be done in a
separate patch series by someone else.

Anyway thanks for reviewing this patch.