[PATCH 04/20] checkout: clarify memory ownership in `unique_tracking_name()`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 10137 bytes --]

The function `unique_tracking_name()` returns an allocated string, but
does not clearly indicate this because its return type is `const char *`
instead of `char *`. This has led to various callsites where we never
free its returned memory at all, which causes memory leaks.

Plug those leaks and mark now-passing tests as leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/checkout.c                            | 14 +++++++------
 builtin/worktree.c                            | 20 ++++++++++---------
 checkout.c                                    |  4 ++--
 checkout.h                                    |  6 +++---
 t/t2024-checkout-dwim.sh                      |  1 +
 t/t2060-switch.sh                             |  1 +
 t/t3426-rebase-submodule.sh                   |  1 +
 t/t3512-cherry-pick-submodule.sh              |  1 +
 t/t3513-revert-submodule.sh                   |  1 +
 t/t3600-rm.sh                                 |  1 +
 t/t3906-stash-submodule.sh                    |  1 +
 t/t4137-apply-submodule.sh                    |  1 +
 t/t6041-bisect-submodule.sh                   |  1 +
 t/t6438-submodule-directory-file-conflicts.sh |  1 +
 14 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f90a4ca4b7..3cf44b4683 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1275,12 +1275,12 @@ static void setup_new_branch_info_and_source_tree(
 	}
 }
 
-static const char *parse_remote_branch(const char *arg,
-				       struct object_id *rev,
-				       int could_be_checkout_paths)
+static char *parse_remote_branch(const char *arg,
+				 struct object_id *rev,
+				 int could_be_checkout_paths)
 {
 	int num_matches = 0;
-	const char *remote = unique_tracking_name(arg, rev, &num_matches);
+	char *remote = unique_tracking_name(arg, rev, &num_matches);
 
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"
@@ -1316,6 +1316,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	const char **new_branch = &opts->new_branch;
 	int argcount = 0;
 	const char *arg;
+	char *remote = NULL;
 	int dash_dash_pos;
 	int has_dash_dash = 0;
 	int i;
@@ -1416,8 +1417,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 			recover_with_dwim = 0;
 
 		if (recover_with_dwim) {
-			const char *remote = parse_remote_branch(arg, rev,
-								 could_be_checkout_paths);
+			remote = parse_remote_branch(arg, rev,
+						     could_be_checkout_paths);
 			if (remote) {
 				*new_branch = arg;
 				arg = remote;
@@ -1459,6 +1460,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 		argc--;
 	}
 
+	free(remote);
 	return argcount;
 }
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7e0868df72..937da6c0ee 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -736,16 +736,14 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
 	return 1;
 }
 
-static const char *dwim_branch(const char *path, const char **new_branch)
+static char *dwim_branch(const char *path, char **new_branch)
 {
 	int n;
 	int branch_exists;
 	const char *s = worktree_basename(path, &n);
-	const char *branchname = xstrndup(s, n);
+	char *branchname = xstrndup(s, n);
 	struct strbuf ref = STRBUF_INIT;
 
-	UNLEAK(branchname);
-
 	branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
 			refs_ref_exists(get_main_ref_store(the_repository),
 					ref.buf);
@@ -756,8 +754,7 @@ static const char *dwim_branch(const char *path, const char **new_branch)
 	*new_branch = branchname;
 	if (guess_remote) {
 		struct object_id oid;
-		const char *remote =
-			unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
 		return remote;
 	}
 	return NULL;
@@ -769,6 +766,8 @@ static int add(int ac, const char **av, const char *prefix)
 	const char *new_branch_force = NULL;
 	char *path;
 	const char *branch;
+	char *branch_to_free = NULL;
+	char *new_branch_to_free = NULL;
 	const char *new_branch = NULL;
 	const char *opt_track = NULL;
 	const char *lock_reason = NULL;
@@ -859,16 +858,17 @@ static int add(int ac, const char **av, const char *prefix)
 		opts.orphan = dwim_orphan(&opts, !!opt_track, 0);
 	} else if (ac < 2) {
 		/* DWIM: Guess branch name from path. */
-		const char *s = dwim_branch(path, &new_branch);
+		char *s = dwim_branch(path, &new_branch_to_free);
 		if (s)
-			branch = s;
+			branch = branch_to_free = s;
+		new_branch = new_branch_to_free;
 
 		/* DWIM: Infer --orphan when repo has no refs. */
 		opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
 	} else if (ac == 2) {
 		struct object_id oid;
 		struct commit *commit;
-		const char *remote;
+		char *remote;
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
@@ -923,6 +923,8 @@ static int add(int ac, const char **av, const char *prefix)
 
 	ret = add_worktree(path, branch, &opts);
 	free(path);
+	free(branch_to_free);
+	free(new_branch_to_free);
 	return ret;
 }
 
diff --git a/checkout.c b/checkout.c
index 4256e71a7c..cfaea4bd10 100644
--- a/checkout.c
+++ b/checkout.c
@@ -45,8 +45,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 	return 0;
 }
 
-const char *unique_tracking_name(const char *name, struct object_id *oid,
-				 int *dwim_remotes_matched)
+char *unique_tracking_name(const char *name, struct object_id *oid,
+			   int *dwim_remotes_matched)
 {
 	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
 	const char *default_remote = NULL;
diff --git a/checkout.h b/checkout.h
index 3c514a5ab4..ba15a13fb3 100644
--- a/checkout.h
+++ b/checkout.h
@@ -8,8 +8,8 @@
  * tracking branch.  Return the name of the remote if such a branch
  * exists, NULL otherwise.
  */
-const char *unique_tracking_name(const char *name,
-				 struct object_id *oid,
-				 int *dwim_remotes_matched);
+char *unique_tracking_name(const char *name,
+			   struct object_id *oid,
+			   int *dwim_remotes_matched);
 
 #endif /* CHECKOUT_H */
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index a3b1449ef1..2caada3d83 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -4,6 +4,7 @@ test_description='checkout <branch>
 
 Ensures that checkout on an unborn branch does what the user expects'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Is the current branch "refs/heads/$1"?
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index c91c4db936..77b2346291 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -5,6 +5,7 @@ test_description='switch basic functionality'
 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/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index ba069dccbd..94ea88e384 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -2,6 +2,7 @@
 
 test_description='rebase can handle submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh
index f22d1ddead..9387a22a9e 100755
--- a/t/t3512-cherry-pick-submodule.sh
+++ b/t/t3512-cherry-pick-submodule.sh
@@ -5,6 +5,7 @@ test_description='cherry-pick can handle submodules'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 8bfe3ed246..e178968b40 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -2,6 +2,7 @@
 
 test_description='revert can handle submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 98259e2ada..31ac31d4bc 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -8,6 +8,7 @@ test_description='Test of the various options to git rm.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Setup some files to be removed, some with funny characters
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index 0f7348ec21..0f61f01ef4 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -2,6 +2,7 @@
 
 test_description='stash can handle submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index 07d5262537..ebd0d4ad17 100755
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -2,6 +2,7 @@
 
 test_description='git apply handling submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 82013fc903..3946e18089 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -2,6 +2,7 @@
 
 test_description='bisect can handle submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
diff --git a/t/t6438-submodule-directory-file-conflicts.sh b/t/t6438-submodule-directory-file-conflicts.sh
index 8df67a0ef9..3594190af8 100755
--- a/t/t6438-submodule-directory-file-conflicts.sh
+++ b/t/t6438-submodule-directory-file-conflicts.sh
@@ -2,6 +2,7 @@
 
 test_description='merge can handle submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
-- 
2.45.1.216.g4365c6fcf9.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]