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

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

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

>> What happened instead?
>>
>> free(): double free detected in tcache 2
>
> This bisects to 748bd094 (builtin/stash: fix leak in `show_stash()`,
> 2024-06-11) for me.

Good eyes, both of you.

This comes from the fact that the program calls setup_revisions() on
the "rest of the command line args" and have it parse as much as it
understands, while leaving the remainder that it did not understand
there, with this call.

	argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);
	if (argc > 1)
		goto usage;

If you give "git stash show -p --no-such-option", revision_args
before setup_revisions() is called would have

    .v = { "show", "-p", "--no-such-option", NULL }

and upon returning from the call, we would have

    .v = { "show", "--no-such-option", "--no-such-option", NULL }

with argc == 2.  Note that .nr is only passed by value, so across
the call to setup_revisions(), it will stay .nr == 3.

This mongering of .v[] is because the callee parses out and removes
"-p" as an argv element that it understood shifting the remainder in
the array, with the expectation that the caller would use the array
it passed (i.e. .v[]) and the return value from the call (i.e. 2 in
argc) for further processing.  It is not getting the strvec and no
access to .nr to adjust, so this is close to the best it can do.

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