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 <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: pks-t

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 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			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 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 			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 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 				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 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			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 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				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 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	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;
 }

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

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
 

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' '

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' '