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, 17:13 UTC
Message-ID: <xmqqfrci5prq.fsf@gitster.g>
In-Reply-To: <20250919164820.GA24939@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> I think we'll have leaked the string holding "-p" in this instance,
> though. We probably need to pass in a setup_revision_opt struct with its
> free_removed_argv_elements flag set.
>
> That's true even without your patch, too, of course.

Yeah, while I was preparing the "alternative", I noticed that option being paid attention to by the code, but you are right. Anybody who passes strvec (which owns its contents, unlike the traditional "we got this argv[] from the operating system" callers) needs to flip that bit set, or they would leak.

Show 8 quoted lines

> I'm mildly
> surprised that the test suite doesn't hit this in leak-checking mode,
> since it is a problem any time we rearrange argv. E.g., I think:
>
>   git stash show -p --
>
> leaks (I was surprised that "stash show -p --stat" didn't leak, but it
> doesn't seem to rearrange?).

Yeah.

Show 10 quoted lines

> I wonder if the best solution is a setup_revisions() wrapper for strvecs
> that will:
>
>   - turn on the free_removed_argv_elements option automatically
>
>   - collect the return value of setup_revisions() and use it to fix
>     the .nr field of the strvec
>
>   - restore the NULL invariant at the end of the array (though I would
>     also be happy if setup_revisions() just did this itself)

That would be nice. I only did the third one in my "alternative" patch I sent earlier.