Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 19, 2025, 16:00 UTC
- Message-ID
- <xmqq4isy77qr.fsf@gitster.g>
- In-Reply-To
- <1321ff39-6f09-426a-aa75-939ef4e1ad93@app.fastmail.com>
"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
Show 6 quoted lines
>> 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).