| Message ID | 20231003202724.GC7812@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 716a6b2c3a54779e1e6f115a5950511d019f4d17 |
| Headers | show |
| Series | some commit-graph leak fixes | expand |
On Tue, Oct 03, 2023 at 04:27:24PM -0400, Jeff King wrote: > We call repo_get_merge_bases(), which allocates a commit_list, but never > free the result, causing a leak. > > The obvious solution is to free it, but we need to look at the contents > of the first item to decide whether to leave the loop. One option is to > free it in both code paths. But since the commit that the list points to > is longer-lived than the list itself, we can just dereference it > immediately, free the list, and then continue with the existing logic. > This is about the same amount of code, but keeps the list management all > in one place. > > This lets us mark a number of merge-related test scripts as leak-free. Wow, getting 10 newly leak-free tests for half as many lines of code is terrific. Woohoo! Thanks, Taylor
diff --git a/builtin/merge.c b/builtin/merge.c index fd21c0d4f4..ac4816c14e 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1634,6 +1634,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) for (j = remoteheads; j; j = j->next) { struct commit_list *common_one; + struct commit *common_item; /* * Here we *have* to calculate the individual @@ -1643,7 +1644,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix) common_one = repo_get_merge_bases(the_repository, head_commit, j->item); - if (!oideq(&common_one->item->object.oid, &j->item->object.oid)) { + common_item = common_one->item; + free_commit_list(common_one); + if (!oideq(&common_item->object.oid, &j->item->object.oid)) { up_to_date = 0; break; } diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh index f70c46bbbf..7905597869 100755 --- a/t/t4214-log-graph-octopus.sh +++ b/t/t4214-log-graph-octopus.sh @@ -5,6 +5,7 @@ test_description='git log --graph of skewed left octopus merge.' 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-log-graph.sh diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh index 28d0779a8c..b877ac7235 100755 --- a/t/t4215-log-skewed-merges.sh +++ b/t/t4215-log-skewed-merges.sh @@ -2,6 +2,7 @@ test_description='git log --graph of skewed merges' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-log-graph.sh diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh index 5a67bbc760..ced40157ed 100755 --- a/t/t6009-rev-list-parent.sh +++ b/t/t6009-rev-list-parent.sh @@ -5,6 +5,7 @@ test_description='ancestor culling and limiting by parent number' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_revlist () { diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh index 17b54d625d..5f414abc89 100755 --- a/t/t6416-recursive-corner-cases.sh +++ b/t/t6416-recursive-corner-cases.sh @@ -5,6 +5,7 @@ test_description='recursive merge corner cases involving criss-cross merges' 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-merge.sh diff --git a/t/t6433-merge-toplevel.sh b/t/t6433-merge-toplevel.sh index b16031465f..2b42f095dc 100755 --- a/t/t6433-merge-toplevel.sh +++ b/t/t6433-merge-toplevel.sh @@ -5,6 +5,7 @@ test_description='"git merge" top-level frontend' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh t3033_reset () { diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index c9a86f2e94..daa507862c 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh diff --git a/t/t7602-merge-octopus-many.sh b/t/t7602-merge-octopus-many.sh index ff085b086c..3669d33bd5 100755 --- a/t/t7602-merge-octopus-many.sh +++ b/t/t7602-merge-octopus-many.sh @@ -4,6 +4,7 @@ test_description='git merge Testing octopus merge with more than 25 refs.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh index 4887ca705b..0e85b21ec8 100755 --- a/t/t7603-merge-reduce-heads.sh +++ b/t/t7603-merge-reduce-heads.sh @@ -4,6 +4,7 @@ test_description='git merge Testing octopus merge when reducing parents to independent branches.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # 0 - 1 diff --git a/t/t7607-merge-state.sh b/t/t7607-merge-state.sh index 89a62ac53b..9001674f2e 100755 --- a/t/t7607-merge-state.sh +++ b/t/t7607-merge-state.sh @@ -4,6 +4,7 @@ test_description="Test that merge state is as expected after failed merge" 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 'Ensure we restore original state if no merge strategy handles it' ' diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh index 0b908ab2e7..2179938c43 100755 --- a/t/t7608-merge-messages.sh +++ b/t/t7608-merge-messages.sh @@ -4,6 +4,7 @@ test_description='test auto-generated merge messages' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_oneline() {
We call repo_get_merge_bases(), which allocates a commit_list, but never free the result, causing a leak. The obvious solution is to free it, but we need to look at the contents of the first item to decide whether to leave the loop. One option is to free it in both code paths. But since the commit that the list points to is longer-lived than the list itself, we can just dereference it immediately, free the list, and then continue with the existing logic. This is about the same amount of code, but keeps the list management all in one place. This lets us mark a number of merge-related test scripts as leak-free. Signed-off-by: Jeff King <peff@peff.net> --- builtin/merge.c | 5 ++++- t/t4214-log-graph-octopus.sh | 1 + t/t4215-log-skewed-merges.sh | 1 + t/t6009-rev-list-parent.sh | 1 + t/t6416-recursive-corner-cases.sh | 1 + t/t6433-merge-toplevel.sh | 1 + t/t6437-submodule-merge.sh | 1 + t/t7602-merge-octopus-many.sh | 1 + t/t7603-merge-reduce-heads.sh | 1 + t/t7607-merge-state.sh | 1 + t/t7608-merge-messages.sh | 1 + 11 files changed, 14 insertions(+), 1 deletion(-)