From 66f4856110a7577c12f97ae905c95a6f38adba9d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 1 Sep 2026 02:28:15 -0400 Subject: [PATCH 1/2] 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- 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" && From 0e96176af4261824b363df146b1f673b14a1fed5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 1 Sep 2026 02:36:45 -0400 Subject: [PATCH 2/2] 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 Signed-off-by: Junio C Hamano --- 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; }