builtin/mv: fix leaks for submodule gitfile paths

Similar to the preceding commit, we have effectively given tracking
memory ownership of submodule gitfile paths. Refactor the code to start
tracking allocated strings in a separate `struct strvec` such that we
can easily plug those leaks. Mark now-passing tests as leak free.

Note that ideally, we wouldn't require two separate data structures to
track those paths. But we do need to store `NULL` pointers for the
gitfile paths such that we can indicate that its corresponding entries
in the other arrays do not have such a path at all. And given that
`struct strvec`s cannot store `NULL` pointers we cannot use them to
store this information.

There is another small gotcha that is easy to miss: you may be wondering
why we don't want to store `SUBMODULE_WITH_GITDIR` in the strvec. This
is because this is a mere sentinel value and not actually a string at
all.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/mv.c b/builtin/mv.c
index e461d29ca114e9873c48795007cc958efaa8303d..81ca910de6fc35f4243619e6995ddc50696fcdff 100644 (file)
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -82,21 +82,23 @@ static char *add_slash(const char *path)
 
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
-static void prepare_move_submodule(const char *src, int first,
-                                  const char **submodule_gitfile)
+static const char *submodule_gitfile_path(const char *src, int first)
 {
        struct strbuf submodule_dotgit = STRBUF_INIT;
+       const char *path;
+
        if (!S_ISGITLINK(the_repository->index->cache[first]->ce_mode))
                die(_("Directory %s is in index and no submodule?"), src);
        if (!is_staging_gitmodules_ok(the_repository->index))
                die(_("Please stage your changes to .gitmodules or stash them to proceed"));
+
        strbuf_addf(&submodule_dotgit, "%s/.git", src);
-       *submodule_gitfile = read_gitfile(submodule_dotgit.buf);
-       if (*submodule_gitfile)
-               *submodule_gitfile = xstrdup(*submodule_gitfile);
-       else
-               *submodule_gitfile = SUBMODULE_WITH_GITDIR;
+
+       path = read_gitfile(submodule_dotgit.buf);
        strbuf_release(&submodule_dotgit);
+       if (path)
+               return path;
+       return SUBMODULE_WITH_GITDIR;
 }
 
 static int index_range_of_same_dir(const char *src, int length,
@@ -170,7 +172,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
        struct strvec sources = STRVEC_INIT;
        struct strvec dest_paths = STRVEC_INIT;
        struct strvec destinations = STRVEC_INIT;
-       const char **submodule_gitfile;
+       struct strvec submodule_gitfiles_to_free = STRVEC_INIT;
+       const char **submodule_gitfiles;
        char *dst_w_slash = NULL;
        const char **src_dir = NULL;
        int src_dir_nr = 0, src_dir_alloc = 0;
@@ -208,7 +211,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
                flags = 0;
        internal_prefix_pathspec(&dest_paths, prefix, argv + argc, 1, flags);
        dst_w_slash = add_slash(dest_paths.v[0]);
-       submodule_gitfile = xcalloc(argc, sizeof(char *));
+       submodule_gitfiles = xcalloc(argc, sizeof(char *));
 
        if (dest_paths.v[0][0] == '\0')
                /* special case: "." was normalized to "" */
@@ -306,8 +309,10 @@ dir_check:
                        int first = index_name_pos(the_repository->index, src, length), last;
 
                        if (first >= 0) {
-                               prepare_move_submodule(src, first,
-                                                      submodule_gitfile + i);
+                               const char *path = submodule_gitfile_path(src, first);
+                               if (path != SUBMODULE_WITH_GITDIR)
+                                       path = strvec_push(&submodule_gitfiles_to_free, path);
+                               submodule_gitfiles[i] = path;
                                goto act_on_entry;
                        } else if (index_range_of_same_dir(src, length,
                                                           &first, &last) < 1) {
@@ -323,7 +328,7 @@ dir_check:
 
                        n = argc + last - first;
                        REALLOC_ARRAY(modes, n);
-                       REALLOC_ARRAY(submodule_gitfile, n);
+                       REALLOC_ARRAY(submodule_gitfiles, n);
 
                        dst_with_slash = add_slash(dst);
                        dst_with_slash_len = strlen(dst_with_slash);
@@ -338,7 +343,7 @@ dir_check:
 
                                memset(modes + argc + j, 0, sizeof(enum update_mode));
                                modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;
-                               submodule_gitfile[argc + j] = NULL;
+                               submodule_gitfiles[argc + j] = NULL;
 
                                free(prefixed_path);
                        }
@@ -427,8 +432,8 @@ remove_entry:
                        strvec_remove(&sources, i);
                        strvec_remove(&destinations, i);
                        MOVE_ARRAY(modes + i, modes + i + 1, n);
-                       MOVE_ARRAY(submodule_gitfile + i,
-                                  submodule_gitfile + i + 1, n);
+                       MOVE_ARRAY(submodule_gitfiles + i,
+                                  submodule_gitfiles + i + 1, n);
                        i--;
                }
        }
@@ -462,12 +467,12 @@ remove_entry:
                                continue;
                        die_errno(_("renaming '%s' failed"), src);
                }
-               if (submodule_gitfile[i]) {
+               if (submodule_gitfiles[i]) {
                        if (!update_path_in_gitmodules(src, dst))
                                gitmodules_modified = 1;
-                       if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
+                       if (submodule_gitfiles[i] != SUBMODULE_WITH_GITDIR)
                                connect_work_tree_and_git_dir(dst,
-                                                             submodule_gitfile[i],
+                                                             submodule_gitfiles[i],
                                                              1);
                }
 
@@ -573,7 +578,8 @@ out:
        strvec_clear(&sources);
        strvec_clear(&dest_paths);
        strvec_clear(&destinations);
-       free(submodule_gitfile);
+       strvec_clear(&submodule_gitfiles_to_free);
+       free(submodule_gitfiles);
        free(modes);
        return ret;
 }

diff --git a/t/t4059-diff-submodule-not-initialized.sh b/t/t4059-diff-submodule-not-initialized.sh
index d489230df89663620fda4a11b538fc740720a8aa..668f5263038374ec0a2584230041cbdfef674652 100755 (executable)
--- a/t/t4059-diff-submodule-not-initialized.sh
+++ b/t/t4059-diff-submodule-not-initialized.sh
@@ -9,6 +9,7 @@ This test tries to verify that add_submodule_odb works when the submodule was
 initialized previously but the checkout has since been removed.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Tested non-UTF-8 encoding

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 879a6dce601110ac4bf8c59a51b32654e8726dcf..86258f9f4307a34b49a22d3d217ac2a82cefc560 100755 (executable)
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git mv in subdirs'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff-data.sh
 

diff --git a/t/t7417-submodule-path-url.sh b/t/t7417-submodule-path-url.sh
index 5e3051da8bb362fe01b80f5bc20ef0d736886a22..dbbb3853dc08537abdd482eb3f92e2bd44cceb45 100755 (executable)
--- a/t/t7417-submodule-path-url.sh
+++ b/t/t7417-submodule-path-url.sh
@@ -4,6 +4,7 @@ test_description='check handling of .gitmodule path with dash'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '

diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh
index ce64d8b13721732a30d8739c72dfdba670060454..479c8fdde1180bf9d0486a2141ada4aca155d37a 100755 (executable)
--- a/t/t7421-submodule-summary-add.sh
+++ b/t/t7421-submodule-summary-add.sh
@@ -10,6 +10,7 @@ while making sure to add submodules using `git submodule add` instead of
 `git add` as done in t7401.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '