Skip to content

Commit 3ea35c6

Browse files
peffgitster
authored andcommitted
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 f92dbdb (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>
1 parent c44beea commit 3ea35c6

File tree

2 files changed

+6
-1
lines changed

2 files changed

+6
-1
lines changed

builtin/stash.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -956,6 +956,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
956956
static int show_stash(int argc, const char **argv, const char *prefix,
957957
struct repository *repo UNUSED)
958958
{
959+
struct setup_revision_opt opt = { .free_removed_argv_elements = 1 };
959960
int i;
960961
int ret = -1;
961962
struct stash_info info = STASH_INFO_INIT;
@@ -1014,7 +1015,7 @@ static int show_stash(int argc, const char **argv, const char *prefix,
10141015
}
10151016
}
10161017

1017-
argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);
1018+
argc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt);
10181019
if (argc > 1)
10191020
goto usage;
10201021
if (!rev.diffopt.output_format) {

t/t3903-stash.sh

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1741,4 +1741,8 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes
17411741
)
17421742
'
17431743

1744+
test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' '
1745+
git stash show --
1746+
'
1747+
17441748
test_done

0 commit comments

Comments
 (0)