From: Junio C Hamano Date: Mon, 22 Sep 2025 21:26:44 GMT Subject: Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings Message-ID: In-Reply-To: <20250922202509.GE2205919@coredump.intra.peff.net> Jeff King writes: > On Mon, Sep 22, 2025 at 12:36:22PM -0700, Junio C Hamano wrote: > >> Jeff King writes: >> >> > Yeah, I had originally written just: >> > >> > test_expect_success 'stash show -- does not leak' ' >> > git stash show -- >> > ' >> > >> > but it felt funny, since the test is doing nothing in a build without >> > SANITIZE=leak. If we are OK with that funniness, I can switch back to >> > that. >> >> We have a prerequisite for that. Very nice vehicle for >> documentation purposes, even though we do not care about a single >> "stash show" invocation for correctness or performance reasons. > > Ah, nice. It crossed my mind that we could try marking it as such, but I > forgot (or never knew) that we already had this prereq available. I > guess it was mainly used as "!SANITIZE_LEAK" before for suppressing > leaks. > >> Perhaps I can squash the following in, unless you have other changes >> in mind. > > Thanks, that looks perfect for the code change. But we probably need to > update the discussion of the test in the commit message. Here's what I > came up with (replacing patch 1): Thanks, will replace then. > -- >8 -- > Subject: [PATCH] stash: tell setup_revisions() to free our allocated strings > > In "git stash show", we do a first pass of parsing our command line > options by splitting them into revision args and stash args. These are > stored in strvecs, and we pass the revision args to setup_revisions(). > > But setup_revisions() may modify the argv we pass it, causing us to leak > some of the entries. In particular, if it sees a "--" string, that will > be dropped from argv. This is the same as other cases addressed by > f92dbdbc6a (revisions API: don't leak memory on argv elements that need > free()-ing, 2022-08-02), and we should fix it the same way: by passing > the free_removed_argv_elements option to setup_revisions(). > > The added test here is run only with SANITIZE=leak, without checking its > output, because the behavior of stash with "--" is a little odd: > > 1. Running "git stash show" will show --stat output. But running "git > stash show --" will show --patch. > > 2. I'd expect a non-option after "--" to be treated as a pathspec, so: > > git stash show -p 1 -- foo > > would look treat "1" as a stash (a synonym for stash@{1}) and > restrict the resulting diff to "foo". But it doesn't. We split the > revision/stash args without any regard to "--". So in the example > above both "1" and "foo" are stashes. Which is an error, but also: > > git stash show -- foo > > treats "foo" as a stash, not a pathspec. All good. I do regret that I didn't resist harder against the "1 is the same as stash@{1}" or insisted that the parser be written more robustly if we wanted to support such syntax, but that also is way too late to change now. > These are both oddities that we may want to address (or may not, if we > want to retain historical quirks). But they are well outside the scope > of this patch. So for now we'll just let the tests confirm we aren't > leaking without otherwise expecting any behavior. If we later address > either of those points and end up with another test that covers "stash > show --", we can drop this leak-only test. OK. By the way this also solves the original "git stash show -p --invalid"; do we want to credit the bug reporter and mention that issue as well? Thanks. > Signed-off-by: Jeff King > --- > builtin/stash.c | 3 ++- > t/t3903-stash.sh | 4 ++++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/builtin/stash.c b/builtin/stash.c > index f5ddee5c7f..e5ab3c4cf5 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -957,6 +957,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op > static int show_stash(int argc, const char **argv, const char *prefix, > struct repository *repo UNUSED) > { > + struct setup_revision_opt opt = { .free_removed_argv_elements = 1 }; > int i; > int ret = -1; > struct stash_info info = STASH_INFO_INIT; > @@ -1015,7 +1016,7 @@ static int show_stash(int argc, const char **argv, const char *prefix, > } > } > > - argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL); > + argc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt); > if (argc > 1) > goto usage; > if (!rev.diffopt.output_format) { > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 0bb4648e36..daf96aa931 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1741,4 +1741,8 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes > ) > ' > > +test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' ' > + git stash show -- > +' > + > test_done