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

[Patrick Steinhardt <ps@pks.im>]

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