Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings

From: Jeff King <peff@peff.net>
Date: Sep 22, 2025, 20:25 UTC
Message-ID: <20250922202509.GE2205919@coredump.intra.peff.net>
In-Reply-To: <xmqqikhath3d.fsf@gitster.g>

On Mon, Sep 22, 2025 at 12:36:22PM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> Jeff King <peff@peff.net> 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):

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

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 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

-- 
2.51.0.582.g88c6764cd5