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

Christian Couder <[email protected]> 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.