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:58 UTC
- Message-ID
- <xmqqldma5qha.fsf@gitster.g>
- In-Reply-To
- <xmqq4isy77qr.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> 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