From 3ea35c64b0e6c86450ebadda22400295103cda64 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 22 Sep 2025 16:25:09 -0400 Subject: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings In "git stash show", we do a first pass of parsing our command line options by splitting them into revision args and stash args. These are stored in strvecs, and we pass the revision args to setup_revisions(). But setup_revisions() may modify the argv we pass it, causing us to leak some of the entries. In particular, if it sees a "--" string, that will be dropped from argv. This is the same as other cases addressed by f92dbdbc6a (revisions API: don't leak memory on argv elements that need free()-ing, 2022-08-02), and we should fix it the same way: by passing the free_removed_argv_elements option to setup_revisions(). The added test here is run only with SANITIZE=leak, without checking its output, because the behavior of stash with "--" is a little odd: 1. Running "git stash show" will show --stat output. But running "git stash show --" will show --patch. 2. I'd expect a non-option after "--" to be treated as a pathspec, so: git stash show -p 1 -- foo would look treat "1" as a stash (a synonym for stash@{1}) and restrict the resulting diff to "foo". But it doesn't. We split the revision/stash args without any regard to "--". So in the example above both "1" and "foo" are stashes. Which is an error, but also: git stash show -- foo treats "foo" as a stash, not a pathspec. These are both oddities that we may want to address (or may not, if we want to retain historical quirks). But they are well outside the scope of this patch. So for now we'll just let the tests confirm we aren't leaking without otherwise expecting any behavior. If we later address either of those points and end up with another test that covers "stash show --", we can drop this leak-only test. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/stash.c | 3 ++- t/t3903-stash.sh | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 1977e50df2..01751ce28d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -956,6 +956,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op static int show_stash(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { + struct setup_revision_opt opt = { .free_removed_argv_elements = 1 }; int i; int ret = -1; struct stash_info info = STASH_INFO_INIT; @@ -1014,7 +1015,7 @@ static int show_stash(int argc, const char **argv, const char *prefix, } } - argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL); + argc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt); if (argc > 1) goto usage; if (!rev.diffopt.output_format) { diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 0bb4648e36..daf96aa931 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1741,4 +1741,8 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes ) ' +test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' ' + git stash show -- +' + test_done From cd439487980a212f103fd28ca81a9df33a994d33 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 Sep 2025 18:45:56 -0400 Subject: [PATCH 2/6] revision: manage memory ownership of argv in setup_revisions() The setup_revisions() function takes an argc/argv pair and consumes arguments from it, returning a reduced argc count to the caller. But it may also overwrite entries within the argv array, as it shifts unknown options to the front of argv (so they can be found in the range of 0..argc-1 after we return). For a normal argc/argv coming from the operating system, this is OK. We don't need to worry about memory ownership of the strings in those entries. But some callers pass in allocated strings from a strvec, and we do need to care about those. We faced a similar issue in f92dbdbc6a (revisions API: don't leak memory on argv elements that need free()-ing, 2022-08-02), which added an option for callers to tell us that elements need to be freed. But the implementation within setup_revisions() was incomplete. It only covered the case of dropping "--", but not the movement of unknown options. When we shift argv entries around, we should free the elements we are about to overwrite, so they are not leaked. For example, in: git stash show -p --invalid we will pass this to setup_revisions(): argc = 3, argv[] = { "show", "-p", "--invalid", NULL } which will then return: argc = 2, argv[] = { "show", "--invalid", "--invalid", NULL } overwriting the "-p" entry, which is leaked unless we free it at that moment. You can see in the output above another potential problem. We now have two copies of the "--invalid" string. If the caller does not respect the new argc when free-ing the strings via strvec_clear(), we'll get a double-free. And git-stash suffers from this, and will crash with the above command. So it seems at first glance that the solution is to just assign the reduced argc to the strvec.nr field in the caller. Then it would stop after freeing only any copied entries. But that's not always right either! Remember that we are reducing "argc" to account for elements we've consumed. So if there isn't an invalid option, we'd turn: argc = 2, argv[] = { "show", "-p", NULL } into: argc = 1, argv[] = { "show", "-p", NULL } In that case strvec_clear() must keep looking past the shortened argc we return to find the original "-p" to free. It needs to use the original argc to do that. We can solve this by turning our argv writes into strict moves, not copies. When we shuffle an unknown option to the front, we'll overwrite its old position with NULL. That leaves an argv array that may have NULL "holes" in it. So in the "--invalid" example above we get: argc = 2, argv[] = { "show", "--invalid", NULL, NULL } but something like "git stash -p --invalid -p" would yield: argc = 3, argv[] = { "show", "--invalid", NULL, "-p", NULL } because we move "--invalid" to overwrite the first "-p", but the second one is quietly consumed. But strvec_clear() can handle that fine (it iterates over the "nr" field, and passing NULL to free() is OK). To ease the implementation, I've introduced a helper function. It's a little hacky because it must take a double-pointer to set the old position to NULL. Which in turn means we cannot pass "&arg", our local alias for the current entry we're parsing, but instead "&argv[i]", the pointer in the original array. And to make it even more confusing, we delegate some of this work to handle_revision_opt(), which is passed a subset of the argv array, so is always working on "&argv[0]". Likewise, because handle_revision_opt() only receives the part of argv left to parse, it receives the array to accumulate unknown options as a separate unkc/unkv pair. But we're always working on the same argv array, so our strategy works fine. I suspect this would be a bit more obvious (and avoid some pointer cleverness) if all functions saw the full argv array and worked with positions within it (and our new helper would take two positions, a src and dst). But that would involve refactoring handle_revision_opt(). I punted on that, as what's here is not too ugly and is all contained within revision.c itself. The new test demonstrates that "git stash show -p --invalid" no longer crashes with a double-free (because we move instead of copy). And it passes with SANITIZE=leak because we free "-p" before overwriting. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- revision.c | 24 +++++++++++++++++++++--- t/t3903-stash.sh | 5 +++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/revision.c b/revision.c index 18f300d455..335f77fa98 100644 --- a/revision.c +++ b/revision.c @@ -2304,6 +2304,24 @@ static timestamp_t parse_age(const char *arg) return num; } +static void overwrite_argv(int *argc, const char **argv, + const char **value, + const struct setup_revision_opt *opt) +{ + /* + * Detect the case when we are overwriting ourselves. The assignment + * itself would be a noop either way, but this lets us avoid corner + * cases around the free() and NULL operations. + */ + if (*value != argv[*argc]) { + if (opt && opt->free_removed_argv_elements) + free((char *)argv[*argc]); + argv[*argc] = *value; + *value = NULL; + } + (*argc)++; +} + static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, int *unkc, const char **unkv, const struct setup_revision_opt* opt) @@ -2325,7 +2343,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=")) { - unkv[(*unkc)++] = arg; + overwrite_argv(unkc, unkv, &argv[0], opt); return 1; } @@ -2689,7 +2707,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) - unkv[(*unkc)++] = arg; + overwrite_argv(unkc, unkv, &argv[0], opt); return opts; } @@ -3001,7 +3019,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (!strcmp(arg, "--stdin")) { if (revs->disable_stdin) { - argv[left++] = arg; + overwrite_argv(&left, argv, &argv[i], opt); continue; } if (revs->read_from_stdin++) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index daf96aa931..930c31e547 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1745,4 +1745,9 @@ test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' ' git stash show -- ' +test_expect_success 'controlled error return on unrecognized option' ' + test_expect_code 129 git stash show -p --invalid 2>usage && + grep -e "^usage: git stash show" usage +' + test_done From f93c1d86ccadd9c08969c5fd7c4906da74cd84e4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 Sep 2025 18:48:47 -0400 Subject: [PATCH 3/6] revision: add wrapper to setup_revisions() from a strvec The setup_revisions() function was designed to take the argc/argv pair from the operating system. But we sometimes construct our own argv using a strvec and pass that in. There are a few gotchas that callers need to deal with here: 1. You should always pass the free_removed_argv_elements option via setup_revision_opt. Otherwise, entries may be leaked if setup_revisions() re-shuffles options. 2. After setup_revisions() returns, the strvec state is odd. We get a reduced argc from setup_revisions() telling us how many unknown options were left in place. Entries after that in argv may be retained, or may be NULL (depending on how the reshuffling happened). But the strvec's "nr" field still represents the original value, and some of the entries it thinks it is still storing may be NULL. Callers must be careful with how they access it. Some callers deal with (1), but not all. In practice they are OK because they do not pass any options that would cause setup_revisions() to re-shuffle (namely unknown options which may be relayed from the user, and the use of the "--" separator). But it's probably a good idea to consistently pass this option anyway to future-proof ourselves against the details of setup_revisions() changing. No callers address (2), though I don't think there any visible bugs. Most of them simply call strvec_clear() and never otherwise look at the result. And in fact, if they naively set foo.nr to the argc returned by setup_revisions(), that would cause leaks! Because setup_revisions() does not free consumed options[1], we have to leave the "nr" field of the strvec at its original value to find and free them during strvec_clear(). So I don't think there are any bugs to fix here, but we can make things safer and simpler for callers. Let's introduce a helper function that sets the free_removed_argv_elements automatically and shrinks the strvec to represent the retained options afterwards (taking care to free the now-obsolete entries). We'll start by converting all of the call-sites which use the free_removed_argv_elements option. There should be no behavior change for them, except that their "shrunken" entries are cleaned up immediately, rather than waiting for a strvec_clear() call. [1] Arguably setup_revisions() should be doing this step for us if we told it to free removed options, but there are many existing callers which will be broken if it did. Introducing this helper is a possible first step towards that. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- bisect.c | 5 +---- builtin/stash.c | 5 ++--- builtin/submodule--helper.c | 10 ++-------- remote.c | 5 +---- revision.c | 19 +++++++++++++++++++ revision.h | 2 ++ 6 files changed, 27 insertions(+), 19 deletions(-) diff --git a/bisect.c b/bisect.c index f24474542e..a6dc76b15c 100644 --- a/bisect.c +++ b/bisect.c @@ -674,9 +674,6 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs, const char *bad_format, const char *good_format, int read_paths) { - struct setup_revision_opt opt = { - .free_removed_argv_elements = 1, - }; int i; repo_init_revisions(r, revs, prefix); @@ -693,7 +690,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs, if (read_paths) read_bisect_paths(rev_argv); - setup_revisions(rev_argv->nr, rev_argv->v, revs, &opt); + setup_revisions_from_strvec(rev_argv, revs, NULL); } static void bisect_common(struct rev_info *revs) diff --git a/builtin/stash.c b/builtin/stash.c index 01751ce28d..3a89d9b7f3 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -956,7 +956,6 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op static int show_stash(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { - struct setup_revision_opt opt = { .free_removed_argv_elements = 1 }; int i; int ret = -1; struct stash_info info = STASH_INFO_INIT; @@ -1015,8 +1014,8 @@ static int show_stash(int argc, const char **argv, const char *prefix, } } - argc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt); - if (argc > 1) + setup_revisions_from_strvec(&revision_args, &rev, NULL); + if (revision_args.nr > 1) goto usage; if (!rev.diffopt.output_format) { rev.diffopt.output_format = DIFF_FORMAT_PATCH; diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 07a1935cbe..fcd73abe53 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -616,9 +616,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid, struct rev_info rev = REV_INFO_INIT; struct strbuf buf = STRBUF_INIT; const char *git_dir; - struct setup_revision_opt opt = { - .free_removed_argv_elements = 1, - }; if (validate_submodule_path(path) < 0) die(NULL); @@ -655,7 +652,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid, repo_init_revisions(the_repository, &rev, NULL); rev.abbrev = 0; - setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt); + setup_revisions_from_strvec(&diff_files_args, &rev, NULL); run_diff_files(&rev, 0); if (!diff_result_code(&rev)) { @@ -1094,9 +1091,6 @@ static int compute_summary_module_list(struct object_id *head_oid, { struct strvec diff_args = STRVEC_INIT; struct rev_info rev; - struct setup_revision_opt opt = { - .free_removed_argv_elements = 1, - }; struct module_cb_list list = MODULE_CB_LIST_INIT; int ret = 0; @@ -1114,7 +1108,7 @@ static int compute_summary_module_list(struct object_id *head_oid, repo_init_revisions(the_repository, &rev, info->prefix); rev.abbrev = 0; precompose_argv_prefix(diff_args.nr, diff_args.v, NULL); - setup_revisions(diff_args.nr, diff_args.v, &rev, &opt); + setup_revisions_from_strvec(&diff_args, &rev, NULL); rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = submodule_summary_callback; rev.diffopt.format_callback_data = &list; diff --git a/remote.c b/remote.c index 88f991795b..929c6887ce 100644 --- a/remote.c +++ b/remote.c @@ -2137,9 +2137,6 @@ static int stat_branch_pair(const char *branch_name, const char *base, struct object_id oid; struct commit *ours, *theirs; struct rev_info revs; - struct setup_revision_opt opt = { - .free_removed_argv_elements = 1, - }; struct strvec argv = STRVEC_INIT; /* Cannot stat if what we used to build on no longer exists */ @@ -2174,7 +2171,7 @@ static int stat_branch_pair(const char *branch_name, const char *base, strvec_push(&argv, "--"); repo_init_revisions(the_repository, &revs, NULL); - setup_revisions(argv.nr, argv.v, &revs, &opt); + setup_revisions_from_strvec(&argv, &revs, NULL); if (prepare_revision_walk(&revs)) die(_("revision walk setup failed")); diff --git a/revision.c b/revision.c index 335f77fa98..d4788aedab 100644 --- a/revision.c +++ b/revision.c @@ -3178,6 +3178,25 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s return left; } +void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs, + struct setup_revision_opt *opt) +{ + struct setup_revision_opt fallback_opt; + int ret; + + if (!opt) { + memset(&fallback_opt, 0, sizeof(fallback_opt)); + opt = &fallback_opt; + } + opt->free_removed_argv_elements = 1; + + ret = setup_revisions(argv->nr, argv->v, revs, opt); + + for (size_t i = ret; i < argv->nr; i++) + free((char *)argv->v[i]); + argv->nr = ret; +} + static void release_revisions_cmdline(struct rev_cmdline_info *cmdline) { unsigned int i; diff --git a/revision.h b/revision.h index 21e288c5ba..a28e349044 100644 --- a/revision.h +++ b/revision.h @@ -441,6 +441,8 @@ struct setup_revision_opt { }; int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *); +void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs, + struct setup_revision_opt *); /** * Free data allocated in a "struct rev_info" after it's been From b553332f82440d68710fcfd2dd6718ec5b43f841 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 Sep 2025 18:49:07 -0400 Subject: [PATCH 4/6] treewide: use setup_revisions_from_strvec() when we have a strvec The previous commit introduced a wrapper to make using setup_revisions() with a strvec easier and safer. It converted spots that were already doing most of what the wrapper did. Let's now convert spots where we were not setting up the free_removed_argv_elements flag. As discussed in the previous commit, this probably isn't fixing any bugs or leaks (since these sites wouldn't trigger the re-shuffling of argv that causes them). This is mostly future-proofing us against setup_revisions() becoming more aggressive about its re-shuffling. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 3 ++- http-push.c | 2 +- submodule.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index d7dd8139de..c8b3081a4d 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -525,7 +525,8 @@ static void describe_blob(struct object_id oid, struct strbuf *dst) NULL); repo_init_revisions(the_repository, &revs, NULL); - if (setup_revisions(args.nr, args.v, &revs, NULL) > 1) + setup_revisions_from_strvec(&args, &revs, NULL); + if (args.nr > 1) BUG("setup_revisions could not handle all args?"); if (prepare_revision_walk(&revs)) diff --git a/http-push.c b/http-push.c index 91a5465afb..4c43ba3bc7 100644 --- a/http-push.c +++ b/http-push.c @@ -1941,7 +1941,7 @@ int cmd_main(int argc, const char **argv) strvec_pushf(&commit_argv, "^%s", oid_to_hex(&ref->old_oid)); repo_init_revisions(the_repository, &revs, setup_git_directory()); - setup_revisions(commit_argv.nr, commit_argv.v, &revs, NULL); + setup_revisions_from_strvec(&commit_argv, &revs, NULL); revs.edge_hint = 0; /* just in case */ /* Generate a list of objects that need to be pushed */ diff --git a/submodule.c b/submodule.c index fff3c75570..35c55155f7 100644 --- a/submodule.c +++ b/submodule.c @@ -900,7 +900,7 @@ static void collect_changed_submodules(struct repository *r, save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity = 0; repo_init_revisions(r, &rev, NULL); - setup_revisions(argv->nr, argv->v, &rev, &s_r_opt); + setup_revisions_from_strvec(argv, &rev, &s_r_opt); warn_on_object_refname_ambiguity = save_warning; if (prepare_revision_walk(&rev)) die(_("revision walk setup failed")); From 18068139f2d0fc2aa82f34f2c177d781e228e732 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 Sep 2025 18:50:48 -0400 Subject: [PATCH 5/6] treewide: pass strvecs around for setup_revisions_from_strvec() The previous commit converted callers of setup_revisions() with a strvec to use the safer and easier _from_strvec() variant. Let's now convert spots that don't directly have a strvec, but receive an argc/argv pair that eventually comes from one. We'll instead pass the strvec down to the point where we call setup_revisions(). That makes these functions slightly less flexible if they were to grow other callers that don't use strvecs, but this rigidity is buying us some safety. It is only safe to pass the free_removed_argv_elements option to setup_revisions() if we know the elements of argv/argc are allocated on the heap. That isn't communicated in the type system when we are passed the bare elements. But if we get a strvec, we know that the elements are allocated strings. And at any rate, each of these modified functions has only a single caller (that has a strvec), so the loss of flexibility is unlikely to ever matter. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 6 +++--- builtin/rebase.c | 3 +-- sequencer.c | 7 ++++--- sequencer.h | 4 ++-- shallow.c | 4 ++-- shallow.h | 5 +++-- upload-pack.c | 7 +++---- 7 files changed, 18 insertions(+), 18 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 53a2256250..691935a2a4 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4650,7 +4650,7 @@ static void get_object_list_path_walk(struct rev_info *revs) die(_("failed to pack objects via path-walk")); } -static void get_object_list(struct rev_info *revs, int ac, const char **av) +static void get_object_list(struct rev_info *revs, struct strvec *argv) { struct setup_revision_opt s_r_opt = { .allow_exclude_promisor_objects = 1, @@ -4660,7 +4660,7 @@ static void get_object_list(struct rev_info *revs, int ac, const char **av) int save_warning; save_commit_buffer = 0; - setup_revisions(ac, av, revs, &s_r_opt); + setup_revisions_from_strvec(argv, revs, &s_r_opt); /* make sure shallows are read */ is_repository_shallow(the_repository); @@ -5229,7 +5229,7 @@ int cmd_pack_objects(int argc, revs.include_check = is_not_in_promisor_pack; revs.include_check_obj = is_not_in_promisor_pack_obj; } - get_object_list(&revs, rp.nr, rp.v); + get_object_list(&revs, &rp); release_revisions(&revs); } cleanup_preferred_base(); diff --git a/builtin/rebase.c b/builtin/rebase.c index 3c85768d29..286df7bd24 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -299,8 +299,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) oid_to_hex(&opts->restrict_revision->object.oid)); ret = sequencer_make_script(the_repository, &todo_list.buf, - make_script_args.nr, make_script_args.v, - flags); + &make_script_args, flags); if (ret) { error(_("could not generate todo list")); goto cleanup; diff --git a/sequencer.c b/sequencer.c index aaf2e4df64..0d0fd84aec 100644 --- a/sequencer.c +++ b/sequencer.c @@ -6063,8 +6063,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, return 0; } -int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, - const char **argv, unsigned flags) +int sequencer_make_script(struct repository *r, struct strbuf *out, + struct strvec *argv, unsigned flags) { char *format = NULL; struct pretty_print_context pp = {0}; @@ -6105,7 +6105,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, pp.fmt = revs.commit_format; pp.output_encoding = get_log_output_encoding(); - if (setup_revisions(argc, argv, &revs, NULL) > 1) { + setup_revisions_from_strvec(argv, &revs, NULL); + if (argv->nr > 1) { ret = error(_("make_script: unhandled options")); goto cleanup; } diff --git a/sequencer.h b/sequencer.h index 304ba4b4d3..719684c8a9 100644 --- a/sequencer.h +++ b/sequencer.h @@ -186,8 +186,8 @@ int sequencer_remove_state(struct replay_opts *opts); #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7) #define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8) -int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, - const char **argv, unsigned flags); +int sequencer_make_script(struct repository *r, struct strbuf *out, + struct strvec *argv, unsigned flags); int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, diff --git a/shallow.c b/shallow.c index ef3adb635f..d9cd4e219c 100644 --- a/shallow.c +++ b/shallow.c @@ -213,7 +213,7 @@ static void show_commit(struct commit *commit, void *data) * are marked with shallow_flag. The list of border/shallow commits * are also returned. */ -struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av, +struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv, int shallow_flag, int not_shallow_flag) { @@ -232,7 +232,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av, repo_init_revisions(the_repository, &revs, NULL); save_commit_buffer = 0; - setup_revisions(ac, av, &revs, NULL); + setup_revisions_from_strvec(argv, &revs, NULL); if (prepare_revision_walk(&revs)) die("revision walk setup failed"); diff --git a/shallow.h b/shallow.h index 9bfeade93e..ad591bd139 100644 --- a/shallow.h +++ b/shallow.h @@ -7,6 +7,7 @@ #include "strbuf.h" struct oid_array; +struct strvec; void set_alternate_shallow_file(struct repository *r, const char *path, int override); int register_shallow(struct repository *r, const struct object_id *oid); @@ -36,8 +37,8 @@ void rollback_shallow_file(struct repository *r, struct shallow_lock *lk); struct commit_list *get_shallow_commits(struct object_array *heads, int depth, int shallow_flag, int not_shallow_flag); -struct commit_list *get_shallow_commits_by_rev_list( - int ac, const char **av, int shallow_flag, int not_shallow_flag); +struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv, + int shallow_flag, int not_shallow_flag); int write_shallow_commits(struct strbuf *out, int use_pack_protocol, const struct oid_array *extra); diff --git a/upload-pack.c b/upload-pack.c index 4f26f6afc7..9fcacb2d1a 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -914,13 +914,12 @@ static void deepen(struct upload_pack_data *data, int depth) } static void deepen_by_rev_list(struct upload_pack_data *data, - int ac, - const char **av) + struct strvec *argv) { struct commit_list *result; disable_commit_graph(the_repository); - result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW); + result = get_shallow_commits_by_rev_list(argv, SHALLOW, NOT_SHALLOW); send_shallow(data, result); free_commit_list(result); send_unshallow(data); @@ -956,7 +955,7 @@ static int send_shallow_list(struct upload_pack_data *data) struct object *o = data->want_obj.objects[i].item; strvec_push(&av, oid_to_hex(&o->oid)); } - deepen_by_rev_list(data, av.nr, av.v); + deepen_by_rev_list(data, &av); strvec_clear(&av); ret = 1; } else { From a04bc71725f27e6210602a981563511925f798b0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 Sep 2025 18:51:46 -0400 Subject: [PATCH 6/6] revision: retain argv NULL invariant in setup_revisions() In an argc/argv pair, the entry for argv[argc] is generally NULL. You can iterate by counting up to argc, or by looking for the NULL entry in argv. When we pass such a pair to setup_revisions(), it shrinks argc to account for the options we consumed and returns the result to the caller. But it doesn't touch the entries after the reduced argc. So argv[argc] will be left pointing at some arbitrary entry rather than NULL. This isn't the source of any known bugs, since all callers are aware of the limitation and act accordingly. But it's a possible gotcha that may be easy to miss. Let's set the new argv[argc] to NULL, taking care to free it if the caller asked us to do so. It is tempting to do likewise for all of the entries afterwards, too, as some of them may also need to be freed (e.g., if coming from a strvec). But doing so isn't entirely trivial, as we munge argc in the function (e.g., when we find "--" and move all of the entries after it into the prune_data list). It would be possible with some light refactoring, but it's probably not worth it. Nobody should ever look at them (they are beyond the revised argc and past the NULL argv entry) outside of strvec cleanup, and setup_revisions_from_strvec() already handles this case. There's one other interesting gotcha: many callers which do not want to provide arguments just pass 0/NULL for argc/argv. We need to check for this case before assigning the final NULL. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- revision.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/revision.c b/revision.c index d4788aedab..ba14ac3da1 100644 --- a/revision.c +++ b/revision.c @@ -3175,6 +3175,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s revs->show_notes_given = 1; } + if (argv) { + if (opt && opt->free_removed_argv_elements) + free((char *)argv[left]); + argv[left] = NULL; + } + return left; }