[BUG] git stash show --src-prefix prints freed memory since 2.52.0

[BUG] git stash show --src-prefix prints freed memory since 2.52.0

[Nicolas Le Cam]

What did you do before the bug happened? (Steps to reproduce your issue)

    git init repo && cd repo
    printf 'one\ntwo\nthree\n' >f.txt
    git add f.txt && git commit -m init
    printf 'one\nTWO\nthree\n' >f.txt
    git stash
    git stash show --src-prefix=a/ --dst-prefix=b/

What did you expect to happen? (Expected behavior)

The first line of the patch should use the prefixes I asked for:

    diff --git a/f.txt b/f.txt

What happened instead? (Actual behavior)

The prefixes are replaced by fragments of unrelated heap data, and the
value changes between runs of the same command:

    $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1
    diff --git Uf.txt Uf.txt
    $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1
    diff --git Vf.txt Vf.txt

On other versions the garbage is recognisable as pieces of other
strings live in the process -- "ributes" (from "attributes"),
"bjectmode" (from "objectmode"), "4c/" -- which is what suggests a
use-after-free rather than an off-by-one.

What's different between what you expected and what actually happened?

Scope, from testing across released versions.

"git diff --src-prefix=a/ --dst-prefix=b/" is correct on every version
I tried. Only "stash show" is affected. First line of the patch from
"git stash show --src-prefix=a/ --dst-prefix=b/":

    2.49.1   diff --git a/f.txt b/f.txt          (correct)
    2.52.0   diff --git ributesf.txt 4c/f.txt
    2.53.0   diff --git Uf.txt Uf.txt
    2.54.0   diff --git 4c/f.txt bjectmodef.txt

The 2.53.0 output varies between invocations; the others were stable
within a single container but differ from each other.

Also unaffected: "git stash show -p" with no prefix flags, and
"git stash show -p --no-ext-diff --no-textconv".

Anything else you want to add:

Suspected cause. 3ea35c64b ("stash: tell setup_revisions() to free our
allocated strings", merged in jk/setup-revisions-freefix) added

    struct setup_revision_opt opt = { .free_removed_argv_elements = 1 };

to show_stash(). v2.51.0 does not contain that commit; v2.52.0 does,
which matches the bisect above.

--src-prefix and --dst-prefix are parsed by OPT_STRING_F in diff.c:

    OPT_STRING_F(0, "src-prefix", &options->a_prefix, N_("<prefix>"),
                 N_("show the given source prefix instead of \"a/\""),
                 PARSE_OPT_NONEG),

parse-options stores the pointer into the argv element rather than
copying it, so options->a_prefix points into the "--src-prefix=a/"
string itself. Once setup_revisions() is told it may free the argv
elements it consumes, that string is freed while a_prefix still
references it, and the dangling pointer is read later when the diff
header is emitted.

If that reading is right, the same hazard would apply to any diff
option parsed with OPT_STRING* into a struct diff_options field, not
only these two -- "stash show" is simply the caller that now opts in
to the freeing.

How I ran into it: a tool that passes --src-prefix=a/ --dst-prefix=b/
explicitly so it can parse the resulting patch without being affected
by a user's diff.noprefix or diff.mnemonicPrefix configuration. That
is a fairly common pattern for programs consuming git's diff output
(lint-staged does the same), so the corrupted paths surface as
unparseable filenames rather than as an obvious crash.

I could not find an existing report for this.

[System Info]
git version 2.53.0 (Debian). Reproduced identically on the
alpine/git 2.52.0 and 2.54.0 images; not reproducible on 2.49.1.

Thanks,
Nicolas Le Cam

[PATCH] revision: hang on to "freed" argv elements

[Jeff King]

On Sun, Aug 30, 2026 at 11:55:55PM +0200, Nicolas Le Cam wrote:

> The prefixes are replaced by fragments of unrelated heap data, and the
> value changes between runs of the same command:
> 
>     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1
>     diff --git Uf.txt Uf.txt
>     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1
>     diff --git Vf.txt Vf.txt
> 
> On other versions the garbage is recognisable as pieces of other
> strings live in the process -- "ributes" (from "attributes"),
> "bjectmode" (from "objectmode"), "4c/" -- which is what suggests a
> use-after-free rather than an off-by-one.

Thanks for a clear and thorough bug report! The cause is indeed the
related to the commits you found. The explanation (and fix) are below.

-- >8 --
Subject: revision: hang on to "freed" argv elements

In setup_revisions() we rewrite the incoming argv array, losing
references to the strings it contains. For a synthetic argv array
constructed from heap strings, that traditionally meant we leaked those
allocated strings.

We fixed the leak in cd43948798 (revision: manage memory ownership of
argv in setup_revisions(), 2025-09-19). Now callers can tell the
revision code that argv entries are allocated and should be freed, which
it will do before overwriting them.

But this introduced a new bug! The overwritten entries go away as soon
as option parsing is finished, but a few options may actually create new
references to those strings. And once we free the strings, those stale
references become use-after-free bugs. For example, running:

  git stash show --src-prefix=foo/

demonstrates the problem:

  1. The stash command generates its own synthetic argv (because it has
     to treat the stash specifiers specially) which it then passes to
     setup_revisions().

  2. Parsing will create a reference to the partial string "foo/" in
     revs.diffopt.a_prefix.

  3. When setup_revisions() finishes, we rewrite argv to throw away
     parsed strings. This frees the entry holding "--src-prefix=foo",
     at which point we have a dangling reference in revs.diffopt.

  4. We generate an actual diff, accessing garbage memory via
     revs.diffopt.a_prefix. The output is usually garbled, but ASan also
     detects this reliably.

One obvious fix here is to allocate new strings when we pull data out of
the argv array. But doing so is error prone (every string option must
remember to do it or risk a subtle bug), and creates more questions
about memory ownership (e.g., some callers assign string literals
directly to a_prefix, and we would not want to free those).

Instead we can fix this centrally by delaying the free() calls. We'll
collect any "freed" strings in a new array, hold on to it for the life
of the rev_info struct, and then release it at the end. We can easily
use a strvec for this, since it handles growth and cleanup for us.

This fixes the prefix case above (which is now tested in t3903), and
should fix any other stray cases. Though I could not find any; we use
OPT_STRING only in the prefix diff options, and very few revision opts
store strings. Those that do (like --format and --encoding) already make
a copy of the string. They do not need for us to hold on to the memory
longer, but it does not hurt them if we do.

One may note that combined with cd43948798 we have approached a simpler
solution in a roundabout way. We are still hacking up argv, but now
carefully constructing a parallel argv of old strings we've overwritten
(and will eventually free). In an alternate universe, we could instead
leave the original argv pristine and return a new reduced-size argv.
This is conceptually simpler, though it does mean that every caller must
free that new argv array itself (not the entries). That's not something
they traditionally had to do, so it would mean tweaking every caller.

So even though the combination of this cd43948798 and this patch is a
little convoluted, it should make things just work (no leaks and no
use-after-free) without modifying any callers.

Reported-by: Nicolas Le Cam <niko.lecam@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I prepared this on top of master.

The bug is in v2.52.0. I think it took a while to get noticed because it
only affects a few options, and then only when used with a command that
produces an allocated argv (like "stash show").

I think you probably _could_ produce the problem directly on cd43948798,
but with the test here I think it shows up a little later, when we start
calling setup_revisions_from_strvec(), which frees more aggressively
(plugging the actual leaks). If we are targeting 'maint' and want to
apply the fix close to the source, probably doing it on top of
4bac57bc67 (Merge branch 'jk/setup-revisions-freefix', 2025-09-29) would
be sufficient.

 revision.c       | 36 ++++++++++++++++++++++++++++--------
 revision.h       |  9 +++++++++
 t/t3903-stash.sh | 17 +++++++++++++++++
 3 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index 50dc8b1991..7aee96bd8e 100644
--- a/revision.c
+++ b/revision.c
@@ -2307,9 +2307,27 @@ static timestamp_t parse_age(const char *arg)
 	return num;
 }
 
+/*
+ * When asked to free argv strings, we should not do so immediately. Some
+ * option parsing may have stored a reference to the string (either the whole
+ * thing, or a substring inside it). We should keep it valid until the rev_info
+ * struct itself is freed.
+ *
+ * Note that we take a const str for the convenience of callers (who have the
+ * usual const argv array, even when opt->free_removed_argv_elements is set).
+ * We cast away the const on their behalf.
+ */
+static void mark_argv_for_free(struct rev_info *revs, const char *str)
+{
+	if (!str)
+		return;
+	strvec_push_nodup(&revs->argv_to_free, (char *)str);
+}
+
 static void overwrite_argv(int *argc, const char **argv,
 			   const char **value,
-			   const struct setup_revision_opt *opt)
+			   const struct setup_revision_opt *opt,
+			   struct rev_info *revs)
 {
 	/*
 	 * Detect the case when we are overwriting ourselves. The assignment
@@ -2318,7 +2336,7 @@ static void overwrite_argv(int *argc, const char **argv,
 	 */
 	if (*value != argv[*argc]) {
 		if (opt && opt->free_removed_argv_elements)
-			free((char *)argv[*argc]);
+			mark_argv_for_free(revs, argv[*argc]);
 		argv[*argc] = *value;
 		*value = NULL;
 	}
@@ -2346,7 +2364,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	    starts_with(arg, "--branches=") || starts_with(arg, "--tags=") ||
 	    starts_with(arg, "--remotes=") || starts_with(arg, "--no-walk="))
 	{
-		overwrite_argv(unkc, unkv, &argv[0], opt);
+		overwrite_argv(unkc, unkv, &argv[0], opt, revs);
 		return 1;
 	}
 
@@ -2738,7 +2756,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else {
 		int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
 		if (!opts)
-			overwrite_argv(unkc, unkv, &argv[0], opt);
+			overwrite_argv(unkc, unkv, &argv[0], opt, revs);
 		return opts;
 	}
 
@@ -3038,7 +3056,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			if (strcmp(arg, "--"))
 				continue;
 			if (opt && opt->free_removed_argv_elements)
-				free((char *)argv[i]);
+				mark_argv_for_free(revs, argv[i]);
 			argv[i] = NULL;
 			argc = i;
 			if (argv[i + 1])
@@ -3068,7 +3086,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 			if (!strcmp(arg, "--stdin")) {
 				if (revs->disable_stdin) {
-					overwrite_argv(&left, argv, &argv[i], opt);
+					overwrite_argv(&left, argv, &argv[i],
+						       opt, revs);
 					continue;
 				}
 				if (revs->read_from_stdin++)
@@ -3242,7 +3261,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	if (argv) {
 		if (opt && opt->free_removed_argv_elements)
-			free((char *)argv[left]);
+			mark_argv_for_free(revs, argv[left]);
 		argv[left] = NULL;
 	}
 
@@ -3264,7 +3283,7 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
 	ret = setup_revisions(argv->nr, argv->v, revs, opt);
 
 	for (size_t i = ret; i < argv->nr; i++)
-		free((char *)argv->v[i]);
+		mark_argv_for_free(revs, argv->v[i]);
 	argv->nr = ret;
 }
 
@@ -3326,6 +3345,7 @@ void release_revisions(struct rev_info *revs)
 	oidset_clear(&revs->missing_commits);
 	release_revisions_bloom_keyvecs(revs);
 	release_follow_pathspec_slab(revs);
+	strvec_clear(&revs->argv_to_free);
 }
 
 static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
diff --git a/revision.h b/revision.h
index acf6d06b24..e5dabd18ce 100644
--- a/revision.h
+++ b/revision.h
@@ -396,6 +396,14 @@ struct rev_info {
 
 	/* Missing commits to be tracked without failing traversal. */
 	struct oidset missing_commits;
+
+	/*
+	 * Strings whose ownership has been handed over to us, but which
+	 * we may be referencing in any of the above options (including
+	 * within the diffopt struct). These will remain valid until
+	 * release_revisions() is called.
+	 */
+	struct strvec argv_to_free;
 };
 
 /**
@@ -433,6 +441,7 @@ struct rev_info {
 	.commit_format = CMIT_FMT_DEFAULT, \
 	.expand_tabs_in_log_default = 8, \
 	.rdiff_log_arg = STRVEC_INIT, \
+	.argv_to_free = STRVEC_INIT, \
 }
 
 /**
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index da27a6599a..260c809f99 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -780,6 +780,23 @@ test_expect_success 'stash show --patience shows diff' '
 	diff_cmp expected actual
 '
 
+test_expect_success 'stash show supports prefixes' '
+	git reset --hard &&
+	echo foo >>file &&
+	git stash &&
+	cat >expected <<-\EOF &&
+	diff --git foo/file bar/file
+	index 7601807..71b52c4 100644
+	--- foo/file
+	+++ bar/file
+	@@ -1 +1,2 @@
+	 baz
+	+foo
+	EOF
+	git stash show --src-prefix=foo/ --dst-prefix=bar/ >actual &&
+	diff_cmp expected actual
+'
+
 test_expect_success 'drop: fail early if specified stash is not a stash ref' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&
-- 
2.55.0.1050.g5a46c03bac

[PATCH 2/1] revision: simplify mark_argv_for_free() callers

[Jeff King]

BTW, this is a small cleanup that I resisted putting into the earlier
commit in order to keep it focused. But maybe worth doing on top?

-- >8 --
Subject: revision: simplify mark_argv_for_free() callers

You do not want to mark an argv element for freeing unless the caller
has given us the free_removed_argv_elements flag. Originally we just
called free() in this case, so each caller checked the flag itself. Now
that we mark them via a helper function, we can push the check down into
the helper. This saves a little bit of duplicated code, but also
hopefully makes the result conceptually simpler.

Every caller but one was already checking this flag. The exception is
setup_revisions_from_strvec(), but it always sets the flag explicitly
(since its whole purpose is managing argv memory). So even though it was
not checking the flag, doing so is OK (it will always be set).

Signed-off-by: Jeff King <peff@peff.net>
---
 revision.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index 7aee96bd8e..59d6372506 100644
--- a/revision.c
+++ b/revision.c
@@ -2317,8 +2317,11 @@ static timestamp_t parse_age(const char *arg)
  * usual const argv array, even when opt->free_removed_argv_elements is set).
  * We cast away the const on their behalf.
  */
-static void mark_argv_for_free(struct rev_info *revs, const char *str)
+static void mark_argv_for_free(const struct setup_revision_opt *opt,
+			       struct rev_info *revs, const char *str)
 {
+	if (!opt || !opt->free_removed_argv_elements)
+		return;
 	if (!str)
 		return;
 	strvec_push_nodup(&revs->argv_to_free, (char *)str);
@@ -2335,8 +2338,7 @@ static void overwrite_argv(int *argc, const char **argv,
 	 * cases around the free() and NULL operations.
 	 */
 	if (*value != argv[*argc]) {
-		if (opt && opt->free_removed_argv_elements)
-			mark_argv_for_free(revs, argv[*argc]);
+		mark_argv_for_free(opt, revs, argv[*argc]);
 		argv[*argc] = *value;
 		*value = NULL;
 	}
@@ -3055,8 +3057,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			const char *arg = argv[i];
 			if (strcmp(arg, "--"))
 				continue;
-			if (opt && opt->free_removed_argv_elements)
-				mark_argv_for_free(revs, argv[i]);
+			mark_argv_for_free(opt, revs, argv[i]);
 			argv[i] = NULL;
 			argc = i;
 			if (argv[i + 1])
@@ -3260,8 +3261,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	}
 
 	if (argv) {
-		if (opt && opt->free_removed_argv_elements)
-			mark_argv_for_free(revs, argv[left]);
+		mark_argv_for_free(opt, revs, argv[left]);
 		argv[left] = NULL;
 	}
 
@@ -3283,7 +3283,7 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
 	ret = setup_revisions(argv->nr, argv->v, revs, opt);
 
 	for (size_t i = ret; i < argv->nr; i++)
-		mark_argv_for_free(revs, argv->v[i]);
+		mark_argv_for_free(opt, revs, argv->v[i]);
 	argv->nr = ret;
 }
 
-- 
2.55.0.1050.g5a46c03bac

Re: [PATCH 2/1] revision: simplify mark_argv_for_free() callers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> BTW, this is a small cleanup that I resisted putting into the earlier
> commit in order to keep it focused. But maybe worth doing on top?

I like it.  It is a tiny simplification but makes the callers easier
to read.

>
> -- >8 --
> Subject: revision: simplify mark_argv_for_free() callers
>
> You do not want to mark an argv element for freeing unless the caller
> has given us the free_removed_argv_elements flag. Originally we just
> called free() in this case, so each caller checked the flag itself. Now
> that we mark them via a helper function, we can push the check down into
> the helper. This saves a little bit of duplicated code, but also
> hopefully makes the result conceptually simpler.
>
> Every caller but one was already checking this flag. The exception is
> setup_revisions_from_strvec(), but it always sets the flag explicitly
> (since its whole purpose is managing argv memory). So even though it was
> not checking the flag, doing so is OK (it will always be set).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  revision.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 7aee96bd8e..59d6372506 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2317,8 +2317,11 @@ static timestamp_t parse_age(const char *arg)
>   * usual const argv array, even when opt->free_removed_argv_elements is set).
>   * We cast away the const on their behalf.
>   */
> -static void mark_argv_for_free(struct rev_info *revs, const char *str)
> +static void mark_argv_for_free(const struct setup_revision_opt *opt,
> +			       struct rev_info *revs, const char *str)
>  {
> +	if (!opt || !opt->free_removed_argv_elements)
> +		return;
>  	if (!str)
>  		return;
>  	strvec_push_nodup(&revs->argv_to_free, (char *)str);
> @@ -2335,8 +2338,7 @@ static void overwrite_argv(int *argc, const char **argv,
>  	 * cases around the free() and NULL operations.
>  	 */
>  	if (*value != argv[*argc]) {
> -		if (opt && opt->free_removed_argv_elements)
> -			mark_argv_for_free(revs, argv[*argc]);
> +		mark_argv_for_free(opt, revs, argv[*argc]);
>  		argv[*argc] = *value;
>  		*value = NULL;
>  	}
> @@ -3055,8 +3057,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			const char *arg = argv[i];
>  			if (strcmp(arg, "--"))
>  				continue;
> -			if (opt && opt->free_removed_argv_elements)
> -				mark_argv_for_free(revs, argv[i]);
> +			mark_argv_for_free(opt, revs, argv[i]);
>  			argv[i] = NULL;
>  			argc = i;
>  			if (argv[i + 1])
> @@ -3260,8 +3261,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  	}
>  
>  	if (argv) {
> -		if (opt && opt->free_removed_argv_elements)
> -			mark_argv_for_free(revs, argv[left]);
> +		mark_argv_for_free(opt, revs, argv[left]);
>  		argv[left] = NULL;
>  	}
>  
> @@ -3283,7 +3283,7 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
>  	ret = setup_revisions(argv->nr, argv->v, revs, opt);
>  
>  	for (size_t i = ret; i < argv->nr; i++)
> -		mark_argv_for_free(revs, argv->v[i]);
> +		mark_argv_for_free(opt, revs, argv->v[i]);
>  	argv->nr = ret;
>  }

Re: [PATCH] revision: hang on to "freed" argv elements

[Patrick Steinhardt]

On Tue, Sep 01, 2026 at 02:28:15AM -0400, Jeff King wrote:
> On Sun, Aug 30, 2026 at 11:55:55PM +0200, Nicolas Le Cam wrote:
> 
> > The prefixes are replaced by fragments of unrelated heap data, and the
> > value changes between runs of the same command:
> > 
> >     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1
> >     diff --git Uf.txt Uf.txt
> >     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1
> >     diff --git Vf.txt Vf.txt
> > 
> > On other versions the garbage is recognisable as pieces of other
> > strings live in the process -- "ributes" (from "attributes"),
> > "bjectmode" (from "objectmode"), "4c/" -- which is what suggests a
> > use-after-free rather than an off-by-one.
> 
> Thanks for a clear and thorough bug report! The cause is indeed the
> related to the commits you found. The explanation (and fix) are below.
> 
> -- >8 --
> Subject: revision: hang on to "freed" argv elements
> 
> In setup_revisions() we rewrite the incoming argv array, losing
> references to the strings it contains. For a synthetic argv array
> constructed from heap strings, that traditionally meant we leaked those
> allocated strings.
> 
> We fixed the leak in cd43948798 (revision: manage memory ownership of
> argv in setup_revisions(), 2025-09-19). Now callers can tell the
> revision code that argv entries are allocated and should be freed, which
> it will do before overwriting them.
> 
> But this introduced a new bug! The overwritten entries go away as soon
> as option parsing is finished, but a few options may actually create new
> references to those strings. And once we free the strings, those stale
> references become use-after-free bugs. For example, running:
> 
>   git stash show --src-prefix=foo/
> 
> demonstrates the problem:
> 
>   1. The stash command generates its own synthetic argv (because it has
>      to treat the stash specifiers specially) which it then passes to
>      setup_revisions().
> 
>   2. Parsing will create a reference to the partial string "foo/" in
>      revs.diffopt.a_prefix.
> 
>   3. When setup_revisions() finishes, we rewrite argv to throw away
>      parsed strings. This frees the entry holding "--src-prefix=foo",
>      at which point we have a dangling reference in revs.diffopt.
> 
>   4. We generate an actual diff, accessing garbage memory via
>      revs.diffopt.a_prefix. The output is usually garbled, but ASan also
>      detects this reliably.
> 
> One obvious fix here is to allocate new strings when we pull data out of
> the argv array. But doing so is error prone (every string option must
> remember to do it or risk a subtle bug), and creates more questions
> about memory ownership (e.g., some callers assign string literals
> directly to a_prefix, and we would not want to free those).
> 
> Instead we can fix this centrally by delaying the free() calls. We'll
> collect any "freed" strings in a new array, hold on to it for the life
> of the rev_info struct, and then release it at the end. We can easily
> use a strvec for this, since it handles growth and cleanup for us.
> 
> This fixes the prefix case above (which is now tested in t3903), and
> should fix any other stray cases. Though I could not find any; we use
> OPT_STRING only in the prefix diff options, and very few revision opts
> store strings. Those that do (like --format and --encoding) already make
> a copy of the string. They do not need for us to hold on to the memory
> longer, but it does not hurt them if we do.

So the fix could've been as trivial as you mention above, where we
simply perform a copy of the string for "--src-prefix", and everything
else works just fine?

In any case though, your approach is more defensive and makes it way
harder for such use-after-free bugs to be introduced going forward, so
I'm in line with the proposed patch.

> diff --git a/revision.c b/revision.c
> index 50dc8b1991..7aee96bd8e 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2307,9 +2307,27 @@ static timestamp_t parse_age(const char *arg)
>  	return num;
>  }
>  
> +/*
> + * When asked to free argv strings, we should not do so immediately. Some
> + * option parsing may have stored a reference to the string (either the whole
> + * thing, or a substring inside it). We should keep it valid until the rev_info
> + * struct itself is freed.
> + *
> + * Note that we take a const str for the convenience of callers (who have the
> + * usual const argv array, even when opt->free_removed_argv_elements is set).
> + * We cast away the const on their behalf.
> + */
> +static void mark_argv_for_free(struct rev_info *revs, const char *str)
> +{
> +	if (!str)
> +		return;
> +	strvec_push_nodup(&revs->argv_to_free, (char *)str);
> +}

Hm. Doesn't this mean that we take ownership of the string and then
eventually try to release it when releasing the vector? I wonder whether
this could introduce subtle lifetime issues where the caller passes a
non-heap-allocated string.

I don't think it's that bad when seeing where we use these. But I feel
like hiding this fact by marking the parameter as `const` is a bit of a
weird design choice. I'd much rather prefer we force this onto the
callers so that they are aware of this, but I haven't seen the end
result of that. So maybe it's just too ugly.

Thanks!

Patrick

Re: [PATCH] revision: hang on to "freed" argv elements

[Jeff King]

On Tue, Sep 01, 2026 at 10:51:26AM +0200, Patrick Steinhardt wrote:

> > This fixes the prefix case above (which is now tested in t3903), and
> > should fix any other stray cases. Though I could not find any; we use
> > OPT_STRING only in the prefix diff options, and very few revision opts
> > store strings. Those that do (like --format and --encoding) already make
> > a copy of the string. They do not need for us to hold on to the memory
> > longer, but it does not hurt them if we do.
> 
> So the fix could've been as trivial as you mention above, where we
> simply perform a copy of the string for "--src-prefix", and everything
> else works just fine?

Well, and --dst-prefix, and also any other cases that get added later.
And keep in mind that --src-prefix and --dst-prefix are not even in the
revision code, but in the diff code. So we'd be creating a very subtle
requirement for somewhat far-away code to adhere to.

> In any case though, your approach is more defensive and makes it way
> harder for such use-after-free bugs to be introduced going forward, so
> I'm in line with the proposed patch.

Yeah, defensive is exactly what I was going for.

> > +static void mark_argv_for_free(struct rev_info *revs, const char *str)
> > +{
> > +	if (!str)
> > +		return;
> > +	strvec_push_nodup(&revs->argv_to_free, (char *)str);
> > +}
> 
> Hm. Doesn't this mean that we take ownership of the string and then
> eventually try to release it when releasing the vector? I wonder whether
> this could introduce subtle lifetime issues where the caller passes a
> non-heap-allocated string.

Yes, that's exactly the point. We are replacing a call to free() with
one that passes ownership to a strvec which later frees it. If somebody
is passing a non-heap string along with free_removed_argv_elements, then
everything was already broken.

> I don't think it's that bad when seeing where we use these. But I feel
> like hiding this fact by marking the parameter as `const` is a bit of a
> weird design choice. I'd much rather prefer we force this onto the
> callers so that they are aware of this, but I haven't seen the end
> result of that. So maybe it's just too ugly.

You can see the effect already in the diff. In the preimage all of the
callers had to cast away const-ness in order to pass the string to
free(). We could keep doing that here, but since this function has
exactly one purpose (to free the string we pass it) it seems like a nice
syntactic convenience to push the cast in here.

Though you may want to look at the "2/1" I sent, which pushes the check
for free_removed_argv_elements into this function. And then the cast and
that check are side-by-side.

-Peff

Re: [PATCH] revision: hang on to "freed" argv elements

[Patrick Steinhardt]

On Tue, Sep 01, 2026 at 05:21:20AM -0400, Jeff King wrote:
> On Tue, Sep 01, 2026 at 10:51:26AM +0200, Patrick Steinhardt wrote:
[snip]
> > > +static void mark_argv_for_free(struct rev_info *revs, const char *str)
> > > +{
> > > +	if (!str)
> > > +		return;
> > > +	strvec_push_nodup(&revs->argv_to_free, (char *)str);
> > > +}
> > 
> > Hm. Doesn't this mean that we take ownership of the string and then
> > eventually try to release it when releasing the vector? I wonder whether
> > this could introduce subtle lifetime issues where the caller passes a
> > non-heap-allocated string.
> 
> Yes, that's exactly the point. We are replacing a call to free() with
> one that passes ownership to a strvec which later frees it. If somebody
> is passing a non-heap string along with free_removed_argv_elements, then
> everything was already broken.

Fair.

> > I don't think it's that bad when seeing where we use these. But I feel
> > like hiding this fact by marking the parameter as `const` is a bit of a
> > weird design choice. I'd much rather prefer we force this onto the
> > callers so that they are aware of this, but I haven't seen the end
> > result of that. So maybe it's just too ugly.
> 
> You can see the effect already in the diff. In the preimage all of the
> callers had to cast away const-ness in order to pass the string to
> free(). We could keep doing that here, but since this function has
> exactly one purpose (to free the string we pass it) it seems like a nice
> syntactic convenience to push the cast in here.

Okay, fair enough.

> Though you may want to look at the "2/1" I sent, which pushes the check
> for free_removed_argv_elements into this function. And then the cast and
> that check are side-by-side.

Makes sense, thanks!

Patrick