diff mbox series

[v2,21/21] builtin/mv: fix leaks for submodule gitfile paths

Message ID 095469193c720b76fc793c0ab00be076caf227c1.1716541556.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Various memory leak fixes | expand

Commit Message

Patrick Steinhardt May 24, 2024, 10:04 a.m. UTC
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>
---
 builtin/mv.c                              | 44 +++++++++++++----------
 t/t4059-diff-submodule-not-initialized.sh |  1 +
 t/t7001-mv.sh                             |  2 ++
 t/t7417-submodule-path-url.sh             |  1 +
 t/t7421-submodule-summary-add.sh          |  1 +
 5 files changed, 30 insertions(+), 19 deletions(-)
diff mbox series

Patch

diff --git a/builtin/mv.c b/builtin/mv.c
index e461d29ca1..81ca910de6 100644
--- 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 @@  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;
 }
diff --git a/t/t4059-diff-submodule-not-initialized.sh b/t/t4059-diff-submodule-not-initialized.sh
index d489230df8..668f526303 100755
--- 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 879a6dce60..86258f9f43 100755
--- 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 5e3051da8b..dbbb3853dc 100755
--- 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 ce64d8b137..479c8fdde1 100755
--- 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' '