From: Jeff King <peff@peff.net>
Date: Mon, 22 Sep 2025 20:25:09 +0000 (-0400)
Subject: stash: tell setup_revisions() to free our allocated strings
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3ea35c64b0e6c86450ebadda22400295103cda64;p=thirdparty%2Fgit.git

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

diff --git a/builtin/stash.c b/builtin/stash.c
index 1977e50df2..01751ce28d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -956,6 +956,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;
@@ -1014,7 +1015,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