From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail•com>,
Lauri Niskanen <ape@ape3000•com>,
git@vger•kernel.org, Patrick Steinhardt <ps@pks•im>
Subject: Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings
Date: Mon, 22 Sep 2025 08:45:36 -0700 [thread overview]
Message-ID: <xmqq1pnywkwv.fsf@gitster.g> (raw)
In-Reply-To: <20250919224027.GA594545@coredump.intra.peff.net> (Jeff King's message of "Fri, 19 Sep 2025 18:40:27 -0400")
Jeff King <peff@peff•net> writes:
> 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().
>
> I've added a test here which fails when built with SANITIZE=leak because
> it calls "git stash show --". This by itself is not a very
> interesting invocation, because there is nothing after the "--", and
> thus the "--" is not really doing anything.
>
> But I think the current parsing in show_stash() is a little
> questionable. It splits the arguments into revision options and stash
> options solely based on the presence of a leading dash, with no regard
> to "--" at all. So:
>
> git stash show -- foo
>
> will take "foo" as a stash option before we even pass anything to
> setup_revisions(). And something like:
>
> git stash show -- 1
>
> will show stash@{1}. But I would expect anything after the "--" to be a
> pathspec. So in this example it would show only the part of the diff
> that touched "foo". And something like:
>
> git stash show -p 1 -- foo
>
> would treat "1" as a stash and "foo" as a pathspec.
>
> That may be something we want to fix, but I want to focus here on the
> leak-fixing without changing behavior. So this test is a little odd, but
> does what we want without locking us in to any particular behavior (we
> only care that "--" by itself does not change the output nor leak).
"git stash show" gives a "git diff --stat stash~ stash" (i.e. the
worktree relative to then-current-HEAD in diffstat format), and "git
stash show --" (no other arguments) gives us "git diff -p" for the
same, it seems; this is with or without your patch.
We may care "--" by itself does not change the output, but it has
already been giving different output without your patch.
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 0bb4648e36..1c9e589bbe 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1741,4 +1741,10 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes
> )
> '
>
> +test_expect_success 'stash show handles --' '
> + git stash show >expect &&
> + git stash show -- >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
In other words, this test should not pass if the stash stores
something reasonable. The only case is when both "git stash show"
and "git stash show -p" would have been silent.
The commit object pointed at by refs/stash before this test runs has
this curious thing.
tree c5f56db0c5c4e40b68ffda4bee76b301c9cc3406
parent a2a54b9cb0873c567fbab134c4b95020b9419f6c
parent 182836fa1adbc25d81a137d4e7489cbd0bec744a
parent f93620bcecd8173d3bd3ad4dff4e69e32ba6f278
author A U Thor <author@example•com> 1112912473 -0700
committer C O Mitter <committer@example•com> 1112912473 -0700
WIP on orphan2: a2a54b9 A
The trees of these three parents reveal why this test passes.
c5f56db0c5c4e40b68ffda4bee76b301c9cc3406
c5f56db0c5c4e40b68ffda4bee76b301c9cc3406
adc18b651de078499a4acf1454fadb65352ed961
In other words, "git diff refs/stash~$n refs/stash" for n == 1 & n
== 2 are empty and this stash entry exists only to record the
untracked cruft. Even "git show -c refs/stash" would stay silent
for a "merge" like this, and both "git stash show" and "git stash
show -p" would of course be empty.
I do not think we want to drop this test (we do want the "handles
without leaking" part of the test), but we should not expect the
output from these commands match.
Unless we are aspiring to introduce a breaking change, that is [*].
Perhaps create a new stash entry that does not show anything in this
test and drop that entry with test_when_finished before leaving it?
Or just run "git stash show --" without any check on its output,
possibly with a prereq that we are running under leak checker?
I dunno.
I only discovered this while merging this and another topic that
happen to touch the same t3903 into 'seen'.
Thanks.
[Footnote]
* I personally find the traditional behaviour nonsense and it may be
coming from the crappy command line parsing we have had forever, but
I am sure people who wrote
git stash show --
git stash show --end-of-options
out of "principle" in their scripts, and assumed that the patch
output is the norm for the command even though it should have been
giving diffstat, would be unhappy if we suddenly made them behave
exactly like "git stash show" (nothing else on the command line).
next prev parent reply other threads:[~2025-09-22 15:45 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-19 10:18 [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear) Lauri Niskanen
2025-09-19 13:11 ` Kristoffer Haugsbakk
2025-09-19 16:00 ` Junio C Hamano
2025-09-19 16:48 ` Jeff King
2025-09-19 17:13 ` Junio C Hamano
2025-09-19 16:58 ` Junio C Hamano
2025-09-19 17:20 ` Jeff King
2025-09-19 18:15 ` Junio C Hamano
2025-09-19 19:56 ` Jeff King
2025-09-19 22:33 ` [PATCH 0/6] fixing double-frees and leaks via setup_revisions() Jeff King
2025-09-19 22:40 ` [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings Jeff King
2025-09-22 15:45 ` Junio C Hamano [this message]
2025-09-22 19:05 ` Jeff King
2025-09-22 19:36 ` Junio C Hamano
2025-09-22 20:25 ` Jeff King
2025-09-22 21:26 ` Junio C Hamano
2025-09-23 0:48 ` Jeff King
2025-09-19 22:45 ` [PATCH 2/6] revision: manage memory ownership of argv in setup_revisions() Jeff King
2025-09-19 22:48 ` [PATCH 3/6] revision: add wrapper to setup_revisions() from a strvec Jeff King
2025-09-20 5:10 ` Eric Sunshine
2025-09-20 5:48 ` Jeff King
2025-09-19 22:49 ` [PATCH 4/6] treewide: use setup_revisions_from_strvec() when we have " Jeff King
2025-09-19 22:50 ` [PATCH 5/6] treewide: pass strvecs around for setup_revisions_from_strvec() Jeff King
2025-09-19 23:11 ` Jeff King
2025-09-19 22:51 ` [PATCH 6/6] revision: retain argv NULL invariant in setup_revisions() Jeff King
2025-09-19 23:07 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq1pnywkwv.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=ape@ape3000$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox