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

0 files changed, 0 insertions, 0 deletions