diff mbox series

[v2,12/29] builtin/merge-recursive: fix leaking object ID bases

Message ID fb30a254abaf132aa3e44b1edea927cf91e1d86f.1718095906.git.ps@pks.im (mailing list archive)
State Accepted
Commit 3199b22e7d8cf8a95b7fac4e4aaf65638256b226
Headers show
Series Memory leak fixes (pt.2) | expand

Commit Message

Patrick Steinhardt June 11, 2024, 9:20 a.m. UTC 
In `cmd_merge_recursive()` we have a static array of object ID bases
that we pass to `merge_recursive_generic()`. This interface is somewhat
weird though because the latter function accepts a pointer to a pointer
of object IDs, which requires us to allocate the object IDs on the heap.
And as we never free those object IDs, the end result is a leak.

While we can easily solve this leak by just freeing the respective
object IDs, the whole calling convention is somewhat weird. Instead,
refactor `merge_recursive_generic()` to accept a plain pointer to object
IDs so that we can avoid allocating them altogether.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/am.c                              | 6 +++---
 builtin/merge-recursive.c                 | 6 ++----
 merge-recursive.c                         | 8 ++++----
 merge-recursive.h                         | 2 +-
 t/t6432-merge-recursive-space-options.sh  | 1 +
 t/t6434-merge-recursive-rename-options.sh | 1 +
 6 files changed, 12 insertions(+), 12 deletions(-)
diff mbox series

Patch

diff --git a/builtin/am.c b/builtin/am.c
index 36839029d2..4ba44e2d70 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1573,8 +1573,8 @@  static int build_fake_ancestor(const struct am_state *state, const char *index_f
  */
 static int fall_back_threeway(const struct am_state *state, const char *index_path)
 {
-	struct object_id orig_tree, their_tree, our_tree;
-	const struct object_id *bases[1] = { &orig_tree };
+	struct object_id their_tree, our_tree;
+	struct object_id bases[1] = { 0 };
 	struct merge_options o;
 	struct commit *result;
 	char *their_tree_name;
@@ -1588,7 +1588,7 @@  static int fall_back_threeway(const struct am_state *state, const char *index_pa
 	discard_index(the_repository->index);
 	read_index_from(the_repository->index, index_path, get_git_dir());
 
-	if (write_index_as_tree(&orig_tree, the_repository->index, index_path, 0, NULL))
+	if (write_index_as_tree(&bases[0], the_repository->index, index_path, 0, NULL))
 		return error(_("Repository lacks necessary blobs to fall back on 3-way merge."));
 
 	say(state, stdout, _("Using index info to reconstruct a base tree..."));
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index c2ce044a20..82bebea15b 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -23,7 +23,7 @@  static char *better_branch_name(const char *branch)
 
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)
 {
-	const struct object_id *bases[21];
+	struct object_id bases[21];
 	unsigned bases_count = 0;
 	int i, failed;
 	struct object_id h1, h2;
@@ -49,10 +49,8 @@  int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)
 			continue;
 		}
 		if (bases_count < ARRAY_SIZE(bases)-1) {
-			struct object_id *oid = xmalloc(sizeof(struct object_id));
-			if (repo_get_oid(the_repository, argv[i], oid))
+			if (repo_get_oid(the_repository, argv[i], &bases[bases_count++]))
 				die(_("could not parse object '%s'"), argv[i]);
-			bases[bases_count++] = oid;
 		}
 		else
 			warning(Q_("cannot handle more than %d base. "
diff --git a/merge-recursive.c b/merge-recursive.c
index 8c8e8b4868..eff73dac02 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3866,7 +3866,7 @@  int merge_recursive_generic(struct merge_options *opt,
 			    const struct object_id *head,
 			    const struct object_id *merge,
 			    int num_merge_bases,
-			    const struct object_id **merge_bases,
+			    const struct object_id *merge_bases,
 			    struct commit **result)
 {
 	int clean;
@@ -3879,10 +3879,10 @@  int merge_recursive_generic(struct merge_options *opt,
 		int i;
 		for (i = 0; i < num_merge_bases; ++i) {
 			struct commit *base;
-			if (!(base = get_ref(opt->repo, merge_bases[i],
-					     oid_to_hex(merge_bases[i]))))
+			if (!(base = get_ref(opt->repo, &merge_bases[i],
+					     oid_to_hex(&merge_bases[i]))))
 				return err(opt, _("Could not parse object '%s'"),
-					   oid_to_hex(merge_bases[i]));
+					   oid_to_hex(&merge_bases[i]));
 			commit_list_insert(base, &ca);
 		}
 		if (num_merge_bases == 1)
diff --git a/merge-recursive.h b/merge-recursive.h
index e67d38c303..839eb6436e 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -123,7 +123,7 @@  int merge_recursive_generic(struct merge_options *opt,
 			    const struct object_id *head,
 			    const struct object_id *merge,
 			    int num_merge_bases,
-			    const struct object_id **merge_bases,
+			    const struct object_id *merge_bases,
 			    struct commit **result);
 
 #endif
diff --git a/t/t6432-merge-recursive-space-options.sh b/t/t6432-merge-recursive-space-options.sh
index db4b77e63d..c93538b0c3 100755
--- a/t/t6432-merge-recursive-space-options.sh
+++ b/t/t6432-merge-recursive-space-options.sh
@@ -14,6 +14,7 @@  test_description='merge-recursive space options
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b
diff --git a/t/t6434-merge-recursive-rename-options.sh b/t/t6434-merge-recursive-rename-options.sh
index a11707835b..df1d0c156c 100755
--- a/t/t6434-merge-recursive-rename-options.sh
+++ b/t/t6434-merge-recursive-rename-options.sh
@@ -29,6 +29,7 @@  mentions this in a different context).
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 get_expected_stages () {