revisions API: don't leak memory on argv elements that need free()-ing

Add a "free_removed_argv_elements" member to "struct
setup_revision_opt", and use it to fix several memory leaks.

We have various memory leaks in APIs that take and munge "const
char **argv", e.g. parse_options(). Sometimes these APIs are given the
"argv" we get to the "main" function, in which case we don't leak
memory, but other times we're giving it the "v" member of a "struct
strvec" we created.

There's several potential ways to fix those sort of leaks, we could
add a "nodup" mode to "struct strvec", which would work for the cases
where we push constant strings to it. But that wouldn't work as soon
as we used strvec_pushf(), or otherwise needed to duplicate or create
a string for that "struct strvec".

Let's instead make it the responsibility of the revisions API. If it's
going to clobber elements of argv it can also free() them, which it
will now do if instructed to do so via "free_removed_argv_elements".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/bisect.c b/bisect.c
index 6afb98be7a1ab05c0fbb6bb00ab218be4a06a004..38b3891f3a6209d24b8f891ccbd274f829919c4c 100644 (file)
--- a/bisect.c
+++ b/bisect.c
@@ -653,6 +653,9 @@ 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);
@@ -669,8 +672,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, NULL);
-       /* XXX leak rev_argv, as "revs" may still be pointing to it */
+       setup_revisions(rev_argv->nr, rev_argv->v, revs, &opt);
 }
 
 static void bisect_common(struct rev_info *revs)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fac52ade5e1792dff7ea717bbc28321fd2489509..b63f420ecef3c8f53b6043b7fdacacc945bd99bb 100644 (file)
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1104,6 +1104,9 @@ 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;
 
@@ -1121,7 +1124,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
        init_revisions(&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, NULL);
+       setup_revisions(diff_args.nr, diff_args.v, &rev, &opt);
        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 1ee2b145d07f7247d8f6e7476e020174520e9faf..d66064118cb3786f6f179065a1845c33feac05b3 100644 (file)
--- a/remote.c
+++ b/remote.c
@@ -2169,6 +2169,9 @@ 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 */
@@ -2203,7 +2206,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, NULL);
+       setup_revisions(argv.nr, argv.v, &revs, &opt);
        if (prepare_revision_walk(&revs))
                die(_("revision walk setup failed"));
 

diff --git a/revision.c b/revision.c
index 0c6e26cd9c8ff8d4f6fdb6050d7df57be460256e..35d24e4fd3ef50cea6d205c45d6f15562df0f69b 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -2784,6 +2784,8 @@ 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)
+                               free((char *)argv[i]);
                        argv[i] = NULL;
                        argc = i;
                        if (argv[i + 1])

diff --git a/revision.h b/revision.h
index e576845cdd103731f3e8d3ea1a4b0eefb21e2063..bb91e7ed9148644355e03e6b3190cebf767289a1 100644 (file)
--- a/revision.h
+++ b/revision.h
@@ -375,7 +375,8 @@ struct setup_revision_opt {
        const char *def;
        void (*tweak)(struct rev_info *, struct setup_revision_opt *);
        unsigned int    assume_dashdash:1,
-                       allow_exclude_promisor_objects:1;
+                       allow_exclude_promisor_objects:1,
+                       free_removed_argv_elements:1;
        unsigned revarg_opt;
 };
 int setup_revisions(int argc, const char **argv, struct rev_info *revs,

diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index bc46713a43e24193bae9a4be204455eb5c066501..2eab6474f8d0f4b3116455c95cb71cd69f78a311 100755 (executable)
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -4,6 +4,7 @@ test_description='checkout into detached HEAD state'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_detached () {