Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> So there are two ways to fix this.
>
> The easier, more performant, and closer to the original design
> around the revisions API is to do this:
>
> diff --git c/builtin/stash.c w/builtin/stash.c
> index f5ddee5c7f..b6312b1b70 100644
> --- c/builtin/stash.c
> +++ w/builtin/stash.c
> @@ -1016,6 +1016,8 @@ static int show_stash(int argc, const char **argv, const char *prefix,
>  	}
>  
>  	argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);
> +	for (i = argc; i < revision_args.nr; i++)
> +		revision_args.v[i] = NULL;
>  	if (argc > 1)
>  		goto usage;
>  	if (!rev.diffopt.output_format) {
>
> A less performant but may in the longer term safer alternative is to
> change the caller-callee contract around setup_revisions() so that
> the later "unused" slots in the argv array is NULLed before
> returning to the caller, i.e. instead of leaving
>
>     .v = { "show", "--no-such-option", "--no-such-option", NULL }
>
> in the revision_args.v[] array, teach setup_revisions() to leave
>
>     .v = { "show", "--no-such-option", NULL, NULL }
>
> there (again, we cannot do anything about .nr that is only available
> to the caller).

For completeness, here is how the other approach may look like, but
I have made my share of off-by-one mistakes all over the place over
the ears, so somebody else needs to lend an eyeball and check it for
sanity.

----- >8 -----
Subject: revisions: do not leave duplicated cruft from setup_revisions()

The API contract between setup_revisions() and its callers is that
the function gets <argc, argv[]> parameters, parses what it can
recognise, and leaves the remainder in argv[] (shifting them to the
head, and return the number of arguments left.  The caller then is
supposed to look at only the early part of argv[] it passed to the
function, up to the argc returned by the function.

Because by contract the caller is not supposed to touch the
remainder of the original argv[], the function left them intact.  If
the incoming parameter were

    argc = 3, argv[] = { "A", "-p", "--unknown", NULL }

and only "-p" is what the function understands, the function would
return

    argc = 2, argv[] = { "A", "--unknown", <unspecified>, <unspecified> }

to the caller, and it is fine as long as the caller would not look
beyond what it is allowed to see.  In practice, these <unspecified>
slots has the original pointer given by the caller, which meant that
the same instance of string "--unknown" appears at argv[1] as it
got copied from argv[2], and argv[2] is not cleared.

Recently however this bit us when the <argc, argv[]> pair is
prepared in a strvec instance.  When trying to clear the strvec
that was passed to setup_revisions(), because some later array
elements are duplicates of earlier elements, strvec_clear() ended up
freeing the same string twice X-<.

We could fix individual callers (in this particular case, letting
strvec_clear() touch the original array .v[] beyond the updated argc
returned by setup_revisions() is a bug in the caller), but I see no
good reason to return argv[] array after modifying it without
cleaning up the later slots (other than the fact that the callee
saves a few cycles because it does not have to bother cleaning array
slots that no caller is supposed to touch, that is).

Teach the function to assign NULLs to the slot the caller is not
supposed to see to help avoiding this class of bugs in the future.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c       | 6 ++++++
 t/t3903-stash.sh | 5 +++++
 2 files changed, 11 insertions(+)

diff --git c/revision.c w/revision.c
index 6ba8f67054..93948e7dc6 100644
--- c/revision.c
+++ w/revision.c
@@ -3174,6 +3174,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revs->show_notes_given = 1;
 	}
 
+	/*
+	 * NULL out the leftover args we did not understand, which has
+	 * shallow copies in earlier slots in the array.
+	 */
+	while (left < argc--)
+		argv[argc] = NULL;
 	return left;
 }
 
diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh
index 0bb4648e36..dd70deb3b3 100755
--- c/t/t3903-stash.sh
+++ w/t/t3903-stash.sh
@@ -69,6 +69,11 @@ test_expect_success 'stash some dirty working directory' '
 	setup_stash
 '
 
+test_expect_success 'controlled error return on unrecognized option' '
+	test_expect_code 129 git stash show -p --no-such 2>usage &&
+	grep -e "^usage: git stash show" usage
+'
+
 cat >expect <<EOF
 diff --git a/file b/file
 index 0cfbf08..00750ed 100644