Re: [PATCH] revision: hang on to "freed" argv elements
From: Patrick Steinhardt <hidden>
Date: 2026-09-01 08:51:35
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:quoted
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.
quoted hunk ↗ jump to hunk
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