{"thread":{"id":"66238","subject":"[BUG] git stash show --src-prefix prints freed memory since 2.52.0","startedAt":"2026-08-30T21:56:22Z","lastAt":"2026-09-01T18:02:29Z","messageCount":7,"participants":["Nicolas Le Cam","Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"551488","messageId":"20260830215555.2660035-1-niko.lecam@gmail.com","threadId":"66238","inReplyTo":null,"subject":"[BUG] git stash show --src-prefix prints freed memory since 2.52.0","fromName":"Nicolas Le Cam","fromEmail":"niko.lecam@gmail.com","sentAt":"2026-08-30T21:55:55Z","receivedAt":"2026-08-30T21:56:22Z","isPatch":false,"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\n    git init repo && cd repo\n    printf 'one\\ntwo\\nthree\\n' >f.txt\n    git add f.txt && git commit -m init\n    printf 'one\\nTWO\\nthree\\n' >f.txt\n    git stash\n    git stash show --src-prefix=a/ --dst-prefix=b/\n\nWhat did you expect to happen? (Expected behavior)\n\nThe first line of the patch should use the prefixes I asked for:\n\n    diff --git a/f.txt b/f.txt\n\nWhat happened instead? (Actual behavior)\n\nThe prefixes are replaced by fragments of unrelated heap data, and the\nvalue changes between runs of the same command:\n\n    $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1\n    diff --git Uf.txt Uf.txt\n    $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1\n    diff --git Vf.txt Vf.txt\n\nOn other versions the garbage is recognisable as pieces of other\nstrings live in the process -- \"ributes\" (from \"attributes\"),\n\"bjectmode\" (from \"objectmode\"), \"4c/\" -- which is what suggests a\nuse-after-free rather than an off-by-one.\n\nWhat's different between what you expected and what actually happened?\n\nScope, from testing across released versions.\n\n\"git diff --src-prefix=a/ --dst-prefix=b/\" is correct on every version\nI tried. Only \"stash show\" is affected. First line of the patch from\n\"git stash show --src-prefix=a/ --dst-prefix=b/\":\n\n    2.49.1   diff --git a/f.txt b/f.txt          (correct)\n    2.52.0   diff --git ributesf.txt 4c/f.txt\n    2.53.0   diff --git Uf.txt Uf.txt\n    2.54.0   diff --git 4c/f.txt bjectmodef.txt\n\nThe 2.53.0 output varies between invocations; the others were stable\nwithin a single container but differ from each other.\n\nAlso unaffected: \"git stash show -p\" with no prefix flags, and\n\"git stash show -p --no-ext-diff --no-textconv\".\n\nAnything else you want to add:\n\nSuspected cause. 3ea35c64b (\"stash: tell setup_revisions() to free our\nallocated strings\", merged in jk/setup-revisions-freefix) added\n\n    struct setup_revision_opt opt = { .free_removed_argv_elements = 1 };\n\nto show_stash(). v2.51.0 does not contain that commit; v2.52.0 does,\nwhich matches the bisect above.\n\n--src-prefix and --dst-prefix are parsed by OPT_STRING_F in diff.c:\n\n    OPT_STRING_F(0, \"src-prefix\", &options->a_prefix, N_(\"<prefix>\"),\n                 N_(\"show the given source prefix instead of \\\"a/\\\"\"),\n                 PARSE_OPT_NONEG),\n\nparse-options stores the pointer into the argv element rather than\ncopying it, so options->a_prefix points into the \"--src-prefix=a/\"\nstring itself. Once setup_revisions() is told it may free the argv\nelements it consumes, that string is freed while a_prefix still\nreferences it, and the dangling pointer is read later when the diff\nheader is emitted.\n\nIf that reading is right, the same hazard would apply to any diff\noption parsed with OPT_STRING* into a struct diff_options field, not\nonly these two -- \"stash show\" is simply the caller that now opts in\nto the freeing.\n\nHow I ran into it: a tool that passes --src-prefix=a/ --dst-prefix=b/\nexplicitly so it can parse the resulting patch without being affected\nby a user's diff.noprefix or diff.mnemonicPrefix configuration. That\nis a fairly common pattern for programs consuming git's diff output\n(lint-staged does the same), so the corrupted paths surface as\nunparseable filenames rather than as an obvious crash.\n\nI could not find an existing report for this.\n\n[System Info]\ngit version 2.53.0 (Debian). Reproduced identically on the\nalpine/git 2.52.0 and 2.54.0 images; not reproducible on 2.49.1.\n\nThanks,\nNicolas Le Cam\n"},{"id":"551624","messageId":"20260901062815.GC1075462@coredump.intra.peff.net","threadId":"66238","inReplyTo":"20260830215555.2660035-1-niko.lecam@gmail.com","subject":"[PATCH] revision: hang on to \"freed\" argv elements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-01T06:28:15Z","receivedAt":"2026-09-01T06:28:17Z","isPatch":true,"body":"On Sun, Aug 30, 2026 at 11:55:55PM +0200, Nicolas Le Cam wrote:\n\n> The prefixes are replaced by fragments of unrelated heap data, and the\n> value changes between runs of the same command:\n> \n>     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1\n>     diff --git Uf.txt Uf.txt\n>     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1\n>     diff --git Vf.txt Vf.txt\n> \n> On other versions the garbage is recognisable as pieces of other\n> strings live in the process -- \"ributes\" (from \"attributes\"),\n> \"bjectmode\" (from \"objectmode\"), \"4c/\" -- which is what suggests a\n> use-after-free rather than an off-by-one.\n\nThanks for a clear and thorough bug report! The cause is indeed the\nrelated to the commits you found. The explanation (and fix) are below.\n\n-- >8 --\nSubject: revision: hang on to \"freed\" argv elements\n\nIn setup_revisions() we rewrite the incoming argv array, losing\nreferences to the strings it contains. For a synthetic argv array\nconstructed from heap strings, that traditionally meant we leaked those\nallocated strings.\n\nWe fixed the leak in cd43948798 (revision: manage memory ownership of\nargv in setup_revisions(), 2025-09-19). Now callers can tell the\nrevision code that argv entries are allocated and should be freed, which\nit will do before overwriting them.\n\nBut this introduced a new bug! The overwritten entries go away as soon\nas option parsing is finished, but a few options may actually create new\nreferences to those strings. And once we free the strings, those stale\nreferences become use-after-free bugs. For example, running:\n\n  git stash show --src-prefix=foo/\n\ndemonstrates the problem:\n\n  1. The stash command generates its own synthetic argv (because it has\n     to treat the stash specifiers specially) which it then passes to\n     setup_revisions().\n\n  2. Parsing will create a reference to the partial string \"foo/\" in\n     revs.diffopt.a_prefix.\n\n  3. When setup_revisions() finishes, we rewrite argv to throw away\n     parsed strings. This frees the entry holding \"--src-prefix=foo\",\n     at which point we have a dangling reference in revs.diffopt.\n\n  4. We generate an actual diff, accessing garbage memory via\n     revs.diffopt.a_prefix. The output is usually garbled, but ASan also\n     detects this reliably.\n\nOne obvious fix here is to allocate new strings when we pull data out of\nthe argv array. But doing so is error prone (every string option must\nremember to do it or risk a subtle bug), and creates more questions\nabout memory ownership (e.g., some callers assign string literals\ndirectly to a_prefix, and we would not want to free those).\n\nInstead we can fix this centrally by delaying the free() calls. We'll\ncollect any \"freed\" strings in a new array, hold on to it for the life\nof the rev_info struct, and then release it at the end. We can easily\nuse a strvec for this, since it handles growth and cleanup for us.\n\nThis fixes the prefix case above (which is now tested in t3903), and\nshould fix any other stray cases. Though I could not find any; we use\nOPT_STRING only in the prefix diff options, and very few revision opts\nstore strings. Those that do (like --format and --encoding) already make\na copy of the string. They do not need for us to hold on to the memory\nlonger, but it does not hurt them if we do.\n\nOne may note that combined with cd43948798 we have approached a simpler\nsolution in a roundabout way. We are still hacking up argv, but now\ncarefully constructing a parallel argv of old strings we've overwritten\n(and will eventually free). In an alternate universe, we could instead\nleave the original argv pristine and return a new reduced-size argv.\nThis is conceptually simpler, though it does mean that every caller must\nfree that new argv array itself (not the entries). That's not something\nthey traditionally had to do, so it would mean tweaking every caller.\n\nSo even though the combination of this cd43948798 and this patch is a\nlittle convoluted, it should make things just work (no leaks and no\nuse-after-free) without modifying any callers.\n\nReported-by: Nicolas Le Cam <niko.lecam@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI prepared this on top of master.\n\nThe bug is in v2.52.0. I think it took a while to get noticed because it\nonly affects a few options, and then only when used with a command that\nproduces an allocated argv (like \"stash show\").\n\nI think you probably _could_ produce the problem directly on cd43948798,\nbut with the test here I think it shows up a little later, when we start\ncalling setup_revisions_from_strvec(), which frees more aggressively\n(plugging the actual leaks). If we are targeting 'maint' and want to\napply the fix close to the source, probably doing it on top of\n4bac57bc67 (Merge branch 'jk/setup-revisions-freefix', 2025-09-29) would\nbe sufficient.\n\n revision.c       | 36 ++++++++++++++++++++++++++++--------\n revision.h       |  9 +++++++++\n t/t3903-stash.sh | 17 +++++++++++++++++\n 3 files changed, 54 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 50dc8b1991..7aee96bd8e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2307,9 +2307,27 @@ static timestamp_t parse_age(const char *arg)\n \treturn num;\n }\n \n+/*\n+ * When asked to free argv strings, we should not do so immediately. Some\n+ * option parsing may have stored a reference to the string (either the whole\n+ * thing, or a substring inside it). We should keep it valid until the rev_info\n+ * struct itself is freed.\n+ *\n+ * Note that we take a const str for the convenience of callers (who have the\n+ * usual const argv array, even when opt->free_removed_argv_elements is set).\n+ * We cast away the const on their behalf.\n+ */\n+static void mark_argv_for_free(struct rev_info *revs, const char *str)\n+{\n+\tif (!str)\n+\t\treturn;\n+\tstrvec_push_nodup(&revs->argv_to_free, (char *)str);\n+}\n+\n static void overwrite_argv(int *argc, const char **argv,\n \t\t\t   const char **value,\n-\t\t\t   const struct setup_revision_opt *opt)\n+\t\t\t   const struct setup_revision_opt *opt,\n+\t\t\t   struct rev_info *revs)\n {\n \t/*\n \t * Detect the case when we are overwriting ourselves. The assignment\n@@ -2318,7 +2336,7 @@ static void overwrite_argv(int *argc, const char **argv,\n \t */\n \tif (*value != argv[*argc]) {\n \t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\tfree((char *)argv[*argc]);\n+\t\t\tmark_argv_for_free(revs, argv[*argc]);\n \t\targv[*argc] = *value;\n \t\t*value = NULL;\n \t}\n@@ -2346,7 +2364,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t    starts_with(arg, \"--branches=\") || starts_with(arg, \"--tags=\") ||\n \t    starts_with(arg, \"--remotes=\") || starts_with(arg, \"--no-walk=\"))\n \t{\n-\t\toverwrite_argv(unkc, unkv, &argv[0], opt);\n+\t\toverwrite_argv(unkc, unkv, &argv[0], opt, revs);\n \t\treturn 1;\n \t}\n \n@@ -2738,7 +2756,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else {\n \t\tint opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);\n \t\tif (!opts)\n-\t\t\toverwrite_argv(unkc, unkv, &argv[0], opt);\n+\t\t\toverwrite_argv(unkc, unkv, &argv[0], opt, revs);\n \t\treturn opts;\n \t}\n \n@@ -3038,7 +3056,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tif (strcmp(arg, \"--\"))\n \t\t\t\tcontinue;\n \t\t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\t\tfree((char *)argv[i]);\n+\t\t\t\tmark_argv_for_free(revs, argv[i]);\n \t\t\targv[i] = NULL;\n \t\t\targc = i;\n \t\t\tif (argv[i + 1])\n@@ -3068,7 +3086,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\t\tif (revs->disable_stdin) {\n-\t\t\t\t\toverwrite_argv(&left, argv, &argv[i], opt);\n+\t\t\t\t\toverwrite_argv(&left, argv, &argv[i],\n+\t\t\t\t\t\t       opt, revs);\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n@@ -3242,7 +3261,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tif (argv) {\n \t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\tfree((char *)argv[left]);\n+\t\t\tmark_argv_for_free(revs, argv[left]);\n \t\targv[left] = NULL;\n \t}\n \n@@ -3264,7 +3283,7 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,\n \tret = setup_revisions(argv->nr, argv->v, revs, opt);\n \n \tfor (size_t i = ret; i < argv->nr; i++)\n-\t\tfree((char *)argv->v[i]);\n+\t\tmark_argv_for_free(revs, argv->v[i]);\n \targv->nr = ret;\n }\n \n@@ -3326,6 +3345,7 @@ void release_revisions(struct rev_info *revs)\n \toidset_clear(&revs->missing_commits);\n \trelease_revisions_bloom_keyvecs(revs);\n \trelease_follow_pathspec_slab(revs);\n+\tstrvec_clear(&revs->argv_to_free);\n }\n \n static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\ndiff --git a/revision.h b/revision.h\nindex acf6d06b24..e5dabd18ce 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -396,6 +396,14 @@ struct rev_info {\n \n \t/* Missing commits to be tracked without failing traversal. */\n \tstruct oidset missing_commits;\n+\n+\t/*\n+\t * Strings whose ownership has been handed over to us, but which\n+\t * we may be referencing in any of the above options (including\n+\t * within the diffopt struct). These will remain valid until\n+\t * release_revisions() is called.\n+\t */\n+\tstruct strvec argv_to_free;\n };\n \n /**\n@@ -433,6 +441,7 @@ struct rev_info {\n \t.commit_format = CMIT_FMT_DEFAULT, \\\n \t.expand_tabs_in_log_default = 8, \\\n \t.rdiff_log_arg = STRVEC_INIT, \\\n+\t.argv_to_free = STRVEC_INIT, \\\n }\n \n /**\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex da27a6599a..260c809f99 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -780,6 +780,23 @@ test_expect_success 'stash show --patience shows diff' '\n \tdiff_cmp expected actual\n '\n \n+test_expect_success 'stash show supports prefixes' '\n+\tgit reset --hard &&\n+\techo foo >>file &&\n+\tgit stash &&\n+\tcat >expected <<-\\EOF &&\n+\tdiff --git foo/file bar/file\n+\tindex 7601807..71b52c4 100644\n+\t--- foo/file\n+\t+++ bar/file\n+\t@@ -1 +1,2 @@\n+\t baz\n+\t+foo\n+\tEOF\n+\tgit stash show --src-prefix=foo/ --dst-prefix=bar/ >actual &&\n+\tdiff_cmp expected actual\n+'\n+\n test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n \tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n-- \n2.55.0.1050.g5a46c03bac\n\n"},{"id":"551625","messageId":"20260901063645.GA2951423@coredump.intra.peff.net","threadId":"66238","inReplyTo":"20260901062815.GC1075462@coredump.intra.peff.net","subject":"[PATCH 2/1] revision: simplify mark_argv_for_free() callers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-01T06:36:45Z","receivedAt":"2026-09-01T06:36:47Z","isPatch":true,"body":"BTW, this is a small cleanup that I resisted putting into the earlier\ncommit in order to keep it focused. But maybe worth doing on top?\n\n-- >8 --\nSubject: revision: simplify mark_argv_for_free() callers\n\nYou do not want to mark an argv element for freeing unless the caller\nhas given us the free_removed_argv_elements flag. Originally we just\ncalled free() in this case, so each caller checked the flag itself. Now\nthat we mark them via a helper function, we can push the check down into\nthe helper. This saves a little bit of duplicated code, but also\nhopefully makes the result conceptually simpler.\n\nEvery caller but one was already checking this flag. The exception is\nsetup_revisions_from_strvec(), but it always sets the flag explicitly\n(since its whole purpose is managing argv memory). So even though it was\nnot checking the flag, doing so is OK (it will always be set).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 7aee96bd8e..59d6372506 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2317,8 +2317,11 @@ static timestamp_t parse_age(const char *arg)\n  * usual const argv array, even when opt->free_removed_argv_elements is set).\n  * We cast away the const on their behalf.\n  */\n-static void mark_argv_for_free(struct rev_info *revs, const char *str)\n+static void mark_argv_for_free(const struct setup_revision_opt *opt,\n+\t\t\t       struct rev_info *revs, const char *str)\n {\n+\tif (!opt || !opt->free_removed_argv_elements)\n+\t\treturn;\n \tif (!str)\n \t\treturn;\n \tstrvec_push_nodup(&revs->argv_to_free, (char *)str);\n@@ -2335,8 +2338,7 @@ static void overwrite_argv(int *argc, const char **argv,\n \t * cases around the free() and NULL operations.\n \t */\n \tif (*value != argv[*argc]) {\n-\t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\tmark_argv_for_free(revs, argv[*argc]);\n+\t\tmark_argv_for_free(opt, revs, argv[*argc]);\n \t\targv[*argc] = *value;\n \t\t*value = NULL;\n \t}\n@@ -3055,8 +3057,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tconst char *arg = argv[i];\n \t\t\tif (strcmp(arg, \"--\"))\n \t\t\t\tcontinue;\n-\t\t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\t\tmark_argv_for_free(revs, argv[i]);\n+\t\t\tmark_argv_for_free(opt, revs, argv[i]);\n \t\t\targv[i] = NULL;\n \t\t\targc = i;\n \t\t\tif (argv[i + 1])\n@@ -3260,8 +3261,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t}\n \n \tif (argv) {\n-\t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\tmark_argv_for_free(revs, argv[left]);\n+\t\tmark_argv_for_free(opt, revs, argv[left]);\n \t\targv[left] = NULL;\n \t}\n \n@@ -3283,7 +3283,7 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,\n \tret = setup_revisions(argv->nr, argv->v, revs, opt);\n \n \tfor (size_t i = ret; i < argv->nr; i++)\n-\t\tmark_argv_for_free(revs, argv->v[i]);\n+\t\tmark_argv_for_free(opt, revs, argv->v[i]);\n \targv->nr = ret;\n }\n \n-- \n2.55.0.1050.g5a46c03bac\n"},{"id":"551630","messageId":"apaSDqIEyc82Q_zE@pks.im","threadId":"66238","inReplyTo":"20260901062815.GC1075462@coredump.intra.peff.net","subject":"Re: [PATCH] revision: hang on to \"freed\" argv elements","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-01T08:51:26Z","receivedAt":"2026-09-01T08:51:35Z","isPatch":true,"body":"On Tue, Sep 01, 2026 at 02:28:15AM -0400, Jeff King wrote:\n> On Sun, Aug 30, 2026 at 11:55:55PM +0200, Nicolas Le Cam wrote:\n> \n> > The prefixes are replaced by fragments of unrelated heap data, and the\n> > value changes between runs of the same command:\n> > \n> >     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1\n> >     diff --git Uf.txt Uf.txt\n> >     $ git stash show --src-prefix=a/ --dst-prefix=b/ | head -1\n> >     diff --git Vf.txt Vf.txt\n> > \n> > On other versions the garbage is recognisable as pieces of other\n> > strings live in the process -- \"ributes\" (from \"attributes\"),\n> > \"bjectmode\" (from \"objectmode\"), \"4c/\" -- which is what suggests a\n> > use-after-free rather than an off-by-one.\n> \n> Thanks for a clear and thorough bug report! The cause is indeed the\n> related to the commits you found. The explanation (and fix) are below.\n> \n> -- >8 --\n> Subject: revision: hang on to \"freed\" argv elements\n> \n> In setup_revisions() we rewrite the incoming argv array, losing\n> references to the strings it contains. For a synthetic argv array\n> constructed from heap strings, that traditionally meant we leaked those\n> allocated strings.\n> \n> We fixed the leak in cd43948798 (revision: manage memory ownership of\n> argv in setup_revisions(), 2025-09-19). Now callers can tell the\n> revision code that argv entries are allocated and should be freed, which\n> it will do before overwriting them.\n> \n> But this introduced a new bug! The overwritten entries go away as soon\n> as option parsing is finished, but a few options may actually create new\n> references to those strings. And once we free the strings, those stale\n> references become use-after-free bugs. For example, running:\n> \n>   git stash show --src-prefix=foo/\n> \n> demonstrates the problem:\n> \n>   1. The stash command generates its own synthetic argv (because it has\n>      to treat the stash specifiers specially) which it then passes to\n>      setup_revisions().\n> \n>   2. Parsing will create a reference to the partial string \"foo/\" in\n>      revs.diffopt.a_prefix.\n> \n>   3. When setup_revisions() finishes, we rewrite argv to throw away\n>      parsed strings. This frees the entry holding \"--src-prefix=foo\",\n>      at which point we have a dangling reference in revs.diffopt.\n> \n>   4. We generate an actual diff, accessing garbage memory via\n>      revs.diffopt.a_prefix. The output is usually garbled, but ASan also\n>      detects this reliably.\n> \n> One obvious fix here is to allocate new strings when we pull data out of\n> the argv array. But doing so is error prone (every string option must\n> remember to do it or risk a subtle bug), and creates more questions\n> about memory ownership (e.g., some callers assign string literals\n> directly to a_prefix, and we would not want to free those).\n> \n> Instead we can fix this centrally by delaying the free() calls. We'll\n> collect any \"freed\" strings in a new array, hold on to it for the life\n> of the rev_info struct, and then release it at the end. We can easily\n> use a strvec for this, since it handles growth and cleanup for us.\n> \n> This fixes the prefix case above (which is now tested in t3903), and\n> should fix any other stray cases. Though I could not find any; we use\n> OPT_STRING only in the prefix diff options, and very few revision opts\n> store strings. Those that do (like --format and --encoding) already make\n> a copy of the string. They do not need for us to hold on to the memory\n> longer, but it does not hurt them if we do.\n\nSo the fix could've been as trivial as you mention above, where we\nsimply perform a copy of the string for \"--src-prefix\", and everything\nelse works just fine?\n\nIn any case though, your approach is more defensive and makes it way\nharder for such use-after-free bugs to be introduced going forward, so\nI'm in line with the proposed patch.\n\n> diff --git a/revision.c b/revision.c\n> index 50dc8b1991..7aee96bd8e 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2307,9 +2307,27 @@ static timestamp_t parse_age(const char *arg)\n>  \treturn num;\n>  }\n>  \n> +/*\n> + * When asked to free argv strings, we should not do so immediately. Some\n> + * option parsing may have stored a reference to the string (either the whole\n> + * thing, or a substring inside it). We should keep it valid until the rev_info\n> + * struct itself is freed.\n> + *\n> + * Note that we take a const str for the convenience of callers (who have the\n> + * usual const argv array, even when opt->free_removed_argv_elements is set).\n> + * We cast away the const on their behalf.\n> + */\n> +static void mark_argv_for_free(struct rev_info *revs, const char *str)\n> +{\n> +\tif (!str)\n> +\t\treturn;\n> +\tstrvec_push_nodup(&revs->argv_to_free, (char *)str);\n> +}\n\nHm. Doesn't this mean that we take ownership of the string and then\neventually try to release it when releasing the vector? I wonder whether\nthis could introduce subtle lifetime issues where the caller passes a\nnon-heap-allocated string.\n\nI don't think it's that bad when seeing where we use these. But I feel\nlike hiding this fact by marking the parameter as `const` is a bit of a\nweird design choice. I'd much rather prefer we force this onto the\ncallers so that they are aware of this, but I haven't seen the end\nresult of that. So maybe it's just too ugly.\n\nThanks!\n\nPatrick\n"},{"id":"551633","messageId":"20260901092120.GA2979683@coredump.intra.peff.net","threadId":"66238","inReplyTo":"apaSDqIEyc82Q_zE@pks.im","subject":"Re: [PATCH] revision: hang on to \"freed\" argv elements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-01T09:21:20Z","receivedAt":"2026-09-01T09:21:22Z","isPatch":true,"body":"On Tue, Sep 01, 2026 at 10:51:26AM +0200, Patrick Steinhardt wrote:\n\n> > This fixes the prefix case above (which is now tested in t3903), and\n> > should fix any other stray cases. Though I could not find any; we use\n> > OPT_STRING only in the prefix diff options, and very few revision opts\n> > store strings. Those that do (like --format and --encoding) already make\n> > a copy of the string. They do not need for us to hold on to the memory\n> > longer, but it does not hurt them if we do.\n> \n> So the fix could've been as trivial as you mention above, where we\n> simply perform a copy of the string for \"--src-prefix\", and everything\n> else works just fine?\n\nWell, and --dst-prefix, and also any other cases that get added later.\nAnd keep in mind that --src-prefix and --dst-prefix are not even in the\nrevision code, but in the diff code. So we'd be creating a very subtle\nrequirement for somewhat far-away code to adhere to.\n\n> In any case though, your approach is more defensive and makes it way\n> harder for such use-after-free bugs to be introduced going forward, so\n> I'm in line with the proposed patch.\n\nYeah, defensive is exactly what I was going for.\n\n> > +static void mark_argv_for_free(struct rev_info *revs, const char *str)\n> > +{\n> > +\tif (!str)\n> > +\t\treturn;\n> > +\tstrvec_push_nodup(&revs->argv_to_free, (char *)str);\n> > +}\n> \n> Hm. Doesn't this mean that we take ownership of the string and then\n> eventually try to release it when releasing the vector? I wonder whether\n> this could introduce subtle lifetime issues where the caller passes a\n> non-heap-allocated string.\n\nYes, that's exactly the point. We are replacing a call to free() with\none that passes ownership to a strvec which later frees it. If somebody\nis passing a non-heap string along with free_removed_argv_elements, then\neverything was already broken.\n\n> I don't think it's that bad when seeing where we use these. But I feel\n> like hiding this fact by marking the parameter as `const` is a bit of a\n> weird design choice. I'd much rather prefer we force this onto the\n> callers so that they are aware of this, but I haven't seen the end\n> result of that. So maybe it's just too ugly.\n\nYou can see the effect already in the diff. In the preimage all of the\ncallers had to cast away const-ness in order to pass the string to\nfree(). We could keep doing that here, but since this function has\nexactly one purpose (to free the string we pass it) it seems like a nice\nsyntactic convenience to push the cast in here.\n\nThough you may want to look at the \"2/1\" I sent, which pushes the check\nfor free_removed_argv_elements into this function. And then the cast and\nthat check are side-by-side.\n\n-Peff\n"},{"id":"551644","messageId":"apayIuf9kXQcQPvS@pks.im","threadId":"66238","inReplyTo":"20260901092120.GA2979683@coredump.intra.peff.net","subject":"Re: [PATCH] revision: hang on to \"freed\" argv elements","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-01T11:08:18Z","receivedAt":"2026-09-01T11:08:27Z","isPatch":true,"body":"On Tue, Sep 01, 2026 at 05:21:20AM -0400, Jeff King wrote:\n> On Tue, Sep 01, 2026 at 10:51:26AM +0200, Patrick Steinhardt wrote:\n[snip]\n> > > +static void mark_argv_for_free(struct rev_info *revs, const char *str)\n> > > +{\n> > > +\tif (!str)\n> > > +\t\treturn;\n> > > +\tstrvec_push_nodup(&revs->argv_to_free, (char *)str);\n> > > +}\n> > \n> > Hm. Doesn't this mean that we take ownership of the string and then\n> > eventually try to release it when releasing the vector? I wonder whether\n> > this could introduce subtle lifetime issues where the caller passes a\n> > non-heap-allocated string.\n> \n> Yes, that's exactly the point. We are replacing a call to free() with\n> one that passes ownership to a strvec which later frees it. If somebody\n> is passing a non-heap string along with free_removed_argv_elements, then\n> everything was already broken.\n\nFair.\n\n> > I don't think it's that bad when seeing where we use these. But I feel\n> > like hiding this fact by marking the parameter as `const` is a bit of a\n> > weird design choice. I'd much rather prefer we force this onto the\n> > callers so that they are aware of this, but I haven't seen the end\n> > result of that. So maybe it's just too ugly.\n> \n> You can see the effect already in the diff. In the preimage all of the\n> callers had to cast away const-ness in order to pass the string to\n> free(). We could keep doing that here, but since this function has\n> exactly one purpose (to free the string we pass it) it seems like a nice\n> syntactic convenience to push the cast in here.\n\nOkay, fair enough.\n\n> Though you may want to look at the \"2/1\" I sent, which pushes the check\n> for free_removed_argv_elements into this function. And then the cast and\n> that check are side-by-side.\n\nMakes sense, thanks!\n\nPatrick\n"},{"id":"551695","messageId":"xmqq8q5ksvd8.fsf@gitster.g","threadId":"66238","inReplyTo":"20260901063645.GA2951423@coredump.intra.peff.net","subject":"Re: [PATCH 2/1] revision: simplify mark_argv_for_free() callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-01T18:02:27Z","receivedAt":"2026-09-01T18:02:29Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> BTW, this is a small cleanup that I resisted putting into the earlier\n> commit in order to keep it focused. But maybe worth doing on top?\n\nI like it.  It is a tiny simplification but makes the callers easier\nto read.\n\n>\n> -- >8 --\n> Subject: revision: simplify mark_argv_for_free() callers\n>\n> You do not want to mark an argv element for freeing unless the caller\n> has given us the free_removed_argv_elements flag. Originally we just\n> called free() in this case, so each caller checked the flag itself. Now\n> that we mark them via a helper function, we can push the check down into\n> the helper. This saves a little bit of duplicated code, but also\n> hopefully makes the result conceptually simpler.\n>\n> Every caller but one was already checking this flag. The exception is\n> setup_revisions_from_strvec(), but it always sets the flag explicitly\n> (since its whole purpose is managing argv memory). So even though it was\n> not checking the flag, doing so is OK (it will always be set).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  revision.c | 16 ++++++++--------\n>  1 file changed, 8 insertions(+), 8 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 7aee96bd8e..59d6372506 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2317,8 +2317,11 @@ static timestamp_t parse_age(const char *arg)\n>   * usual const argv array, even when opt->free_removed_argv_elements is set).\n>   * We cast away the const on their behalf.\n>   */\n> -static void mark_argv_for_free(struct rev_info *revs, const char *str)\n> +static void mark_argv_for_free(const struct setup_revision_opt *opt,\n> +\t\t\t       struct rev_info *revs, const char *str)\n>  {\n> +\tif (!opt || !opt->free_removed_argv_elements)\n> +\t\treturn;\n>  \tif (!str)\n>  \t\treturn;\n>  \tstrvec_push_nodup(&revs->argv_to_free, (char *)str);\n> @@ -2335,8 +2338,7 @@ static void overwrite_argv(int *argc, const char **argv,\n>  \t * cases around the free() and NULL operations.\n>  \t */\n>  \tif (*value != argv[*argc]) {\n> -\t\tif (opt && opt->free_removed_argv_elements)\n> -\t\t\tmark_argv_for_free(revs, argv[*argc]);\n> +\t\tmark_argv_for_free(opt, revs, argv[*argc]);\n>  \t\targv[*argc] = *value;\n>  \t\t*value = NULL;\n>  \t}\n> @@ -3055,8 +3057,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\t\tconst char *arg = argv[i];\n>  \t\t\tif (strcmp(arg, \"--\"))\n>  \t\t\t\tcontinue;\n> -\t\t\tif (opt && opt->free_removed_argv_elements)\n> -\t\t\t\tmark_argv_for_free(revs, argv[i]);\n> +\t\t\tmark_argv_for_free(opt, revs, argv[i]);\n>  \t\t\targv[i] = NULL;\n>  \t\t\targc = i;\n>  \t\t\tif (argv[i + 1])\n> @@ -3260,8 +3261,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t}\n>  \n>  \tif (argv) {\n> -\t\tif (opt && opt->free_removed_argv_elements)\n> -\t\t\tmark_argv_for_free(revs, argv[left]);\n> +\t\tmark_argv_for_free(opt, revs, argv[left]);\n>  \t\targv[left] = NULL;\n>  \t}\n>  \n> @@ -3283,7 +3283,7 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,\n>  \tret = setup_revisions(argv->nr, argv->v, revs, opt);\n>  \n>  \tfor (size_t i = ret; i < argv->nr; i++)\n> -\t\tmark_argv_for_free(revs, argv->v[i]);\n> +\t\tmark_argv_for_free(opt, revs, argv->v[i]);\n>  \targv->nr = ret;\n>  }\n"}]}