[06/29] merge-recursive: fix leaging rename conflict info

Commit Message

Patrick Steinhardt June 3, 2024, 9:46 a.m. UTC

When computing rename conflicts in our recursive merge algorithm we set
up `struct rename_conflict_info`s to track that information. We never
free those data structures though and thus leak memory.

We need to be a bit more careful here though because the same rename
conflict info can be assigned to multiple structures. Accomodate for
this by introducing a `rename_conflict_info_owned` bit that we can use
to steer whether or not the rename conflict info shall be free'd.

Signed-off-by: Patrick Steinhardt <[e-mail]>
---
 merge-recursive.c                  | 8 +++++++-
 t/t3401-rebase-and-am-rename.sh    | 1 +
 t/t4153-am-resume-override-opts.sh | 1 +
 t/t7201-co.sh                      | 1 +
 4 files changed, 10 insertions(+), 1 deletion(-)

Comments

karthik nayak June 6, 2024, 10:07 a.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

In the subject:

s/leaging/leaking

> When computing rename conflicts in our recursive merge algorithm we set
> up `struct rename_conflict_info`s to track that information. We never
> free those data structures though and thus leak memory.
>
> We need to be a bit more careful here though because the same rename
> conflict info can be assigned to multiple structures. Accomodate for

Nit: s/Accomodate/Accommodate

[snip]

Patch

diff --git a/merge-recursive.c b/merge-recursive.c
index 8ff29ed09e..8c8e8b4868 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -239,7 +239,8 @@  enum rename_type {
 struct stage_data {
 	struct diff_filespec stages[4]; /* mostly for oid & mode; maybe path */
 	struct rename_conflict_info *rename_conflict_info;
-	unsigned processed:1;
+	unsigned processed:1,
+		 rename_conflict_info_owned:1;
 };
 
 struct rename {
@@ -308,6 +309,7 @@  static inline void setup_rename_conflict_info(enum rename_type rename_type,
 
 	ci->ren1->dst_entry->processed = 0;
 	ci->ren1->dst_entry->rename_conflict_info = ci;
+	ci->ren1->dst_entry->rename_conflict_info_owned = 1;
 	if (ren2) {
 		ci->ren2->dst_entry->rename_conflict_info = ci;
 	}
@@ -3055,6 +3057,10 @@  static void final_cleanup_rename(struct string_list *rename)
 	for (i = 0; i < rename->nr; i++) {
 		re = rename->items[i].util;
 		diff_free_filepair(re->pair);
+		if (re->src_entry->rename_conflict_info_owned)
+			FREE_AND_NULL(re->src_entry->rename_conflict_info);
+		if (re->dst_entry->rename_conflict_info_owned)
+			FREE_AND_NULL(re->dst_entry->rename_conflict_info);
 	}
 	string_list_clear(rename, 1);
 	free(rename);
diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh
index f18bae9450..328c1d3a3f 100755
--- a/t/t3401-rebase-and-am-rename.sh
+++ b/t/t3401-rebase-and-am-rename.sh
@@ -2,6 +2,7 @@ 
 
 test_description='git rebase + directory rename tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh
index 4add7c7757..6bc377b917 100755
--- a/t/t4153-am-resume-override-opts.sh
+++ b/t/t4153-am-resume-override-opts.sh
@@ -2,6 +2,7 @@ 
 
 test_description='git-am command-line options override saved options'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 42352dc0db..189d8e341b 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -23,6 +23,7 @@  Test switching across them.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_tick