From be4b578c69e0ac1d974ebc9163ff5793a018da8e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:26:09 -0400 Subject: [PATCH 01/10] t6700: mark test as leak-free This test has never leaked since it was added. Let's annotate it to make sure it stays that way (and to reduce noise when looking for other leak-free scripts after we fix some leaks). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t6700-tree-depth.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh index e410c41234..9e70a7c763 100755 --- a/t/t6700-tree-depth.sh +++ b/t/t6700-tree-depth.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='handling of deep trees in various commands' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # We'll test against two depths here: a small one that will let us check the From ec97ad120caad0c7366a19a6680d37e2b8c5107c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:26:30 -0400 Subject: [PATCH 02/10] commit-reach: free temporary list in get_octopus_merge_bases() We loop over the set of commits to merge, and for each one compute the merge base against the existing set of merge base candidates we've found. Then we replace the candidate set with a simple assignment of the list head, leaking the old list. We should free it first before assignment. This makes t5521 leak-free, so mark it as such. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-reach.c | 1 + t/t5521-pull-options.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/commit-reach.c b/commit-reach.c index 4b7c233fd4..a868a575ea 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -173,6 +173,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in) for (k = bases; k; k = k->next) end = k; } + free_commit_list(ret); ret = new_commits; } return ret; diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 264de29c35..079b2f2536 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -5,6 +5,7 @@ test_description='pull options' 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' ' From 716a6b2c3a54779e1e6f115a5950511d019f4d17 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:27:24 -0400 Subject: [PATCH 03/10] merge: free result of repo_get_merge_bases() 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 Signed-off-by: Junio C Hamano --- 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(-) 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() { From ac6d45d11f24921ead899c74569b717a7895f4a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:27:52 -0400 Subject: [PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph() When closing and freeing a commit-graph, the main entry point is close_commit_graph(), which then uses close_commit_graph_one() to recurse through the base_graph links and free each one. Commit 957ba814bf (commit-graph: when closing the graph, also release the slab, 2021-09-08) put the call to clear the slab into the recursive function, but this is pointless: there's only a single global slab variable. It works OK in practice because clearing the slab is idempotent, but it makes the code harder to reason about and refactor. Move it into the parent function so it's only called once (and there are no other direct callers of the recursive close_commit_graph_one(), so we are not hurting them). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commit-graph.c b/commit-graph.c index 5e8a3a5085..dc54ef4776 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -728,13 +728,13 @@ static void close_commit_graph_one(struct commit_graph *g) if (!g) return; - clear_commit_graph_data_slab(&commit_graph_data_slab); close_commit_graph_one(g->base_graph); free_commit_graph(g); } void close_commit_graph(struct raw_object_store *o) { + clear_commit_graph_data_slab(&commit_graph_data_slab); close_commit_graph_one(o->commit_graph); o->commit_graph = NULL; } From 09a75abba4eabab3f2cc4474e5d2db3a33a3a682 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:29:30 -0400 Subject: [PATCH 05/10] commit-graph: free all elements of graph chain When running "commit-graph verify", we call free_commit_graph(). That's sufficient for the case of a single graph file, but if we loaded a chain of split graph files, they form a linked list via the base_graph pointers. We need to free all of them, or we leak all but the first struct. We can make this work by teaching free_commit_graph() to walk the base_graph pointers and free each element. This in turn lets us simplify close_commit_graph(), which does the same thing by recursion (we cannot just use close_commit_graph() in "commit-graph verify", as the function takes a pointer to an object store, and the verify command creates a single one-off graph struct). While indenting the code in free_commit_graph() for the loop, I noticed that setting g->data to NULL is rather pointless, as we free the struct a few lines later. So I cleaned that up while we're here. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index dc54ef4776..2f75ecd9ae 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -723,19 +723,10 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r) return NULL; } -static void close_commit_graph_one(struct commit_graph *g) -{ - if (!g) - return; - - close_commit_graph_one(g->base_graph); - free_commit_graph(g); -} - void close_commit_graph(struct raw_object_store *o) { clear_commit_graph_data_slab(&commit_graph_data_slab); - close_commit_graph_one(o->commit_graph); + free_commit_graph(o->commit_graph); o->commit_graph = NULL; } @@ -2753,15 +2744,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags) void free_commit_graph(struct commit_graph *g) { - if (!g) - return; - if (g->data) { - munmap((void *)g->data, g->data_len); - g->data = NULL; + while (g) { + struct commit_graph *next = g->base_graph; + + if (g->data) + munmap((void *)g->data, g->data_len); + free(g->filename); + free(g->bloom_filter_settings); + free(g); + + g = next; } - free(g->filename); - free(g->bloom_filter_settings); - free(g); } void disable_commit_graph(struct repository *r) From 991d549f74862d07a38a077f34c0deaf65607c74 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:30:04 -0400 Subject: [PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain() When adding a graph to a chain, we do some consistency checks and then if everything looks good, set g->base_graph to add a link to the chain. But when we added a new consistency check in 209250ef38 (commit-graph.c: prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_ we've already set g->base_graph. So we might return failure, even though we actually added to the chain. This hasn't caused a bug yet, because after failing to add to the chain, we discard the failed graph struct completely, leaking it. But in order to fix that, it's important that the struct be in a consistent and predictable state after the failure. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 2f75ecd9ae..2c72a554c2 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g, cur_g = cur_g->base_graph; } - g->base_graph = chain; - if (chain) { if (unsigned_add_overflows(chain->num_commits, chain->num_commits_in_base)) { @@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g, g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base; } + g->base_graph = chain; + return 1; } From 1d94abfe1eedb3f0a9de74ba59483ef8f10b352c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:30:44 -0400 Subject: [PATCH 07/10] commit-graph: free graph struct that was not added to chain When reading the graph chain file, we open (and allocate) each individual slice it mentions and then add them to a linked-list chain. But if adding to the chain fails (e.g., because the base-graph chunk it contains didn't match what we expected), we leave the function without freeing the graph struct that caused the failure, leaking it. We can fix it by calling free_graph_commit(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/commit-graph.c b/commit-graph.c index 2c72a554c2..4aa2f294f1 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -566,6 +566,8 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r, if (add_graph_to_chain(g, graph_chain, oids, i)) { graph_chain = g; valid = 1; + } else { + free_commit_graph(g); } break; From 274bfa7f28fea96fafa114f2508ebef53735d7b6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:30:55 -0400 Subject: [PATCH 08/10] commit-graph: free write-context entries before overwriting When writing a split graph file, we replace the final element of the commit_graph_hash_after and commit_graph_filenames_after arrays. But since these are allocated strings, we need to free them before overwriting to avoid leaking the old string. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/commit-graph.c b/commit-graph.c index 4aa2f294f1..744b7eb1a3 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) free(graph_name); } + free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]); ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash)); final_graph_name = get_split_graph_filename(ctx->odb, ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]); + free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]); ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name; result = rename(ctx->graph_name, final_graph_name); From d9c84c6d67e3418bade00af7bcb5f72d7c656164 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:31:11 -0400 Subject: [PATCH 09/10] commit-graph: free write-context base_graph_name during cleanup Commit 6c622f9f0b (commit-graph: write commit-graph chains, 2019-06-18) added a base_graph_name string to the write_commit_graph_context struct. But the end-of-function cleanup forgot to free it, causing a leak. This (presumably in combination with the preceding leak-fixes) lets us mark t5328 as leak-free. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 1 + t/t5328-commit-graph-64bit-time.sh | 2 ++ 2 files changed, 3 insertions(+) diff --git a/commit-graph.c b/commit-graph.c index 744b7eb1a3..e4d09da090 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2518,6 +2518,7 @@ int write_commit_graph(struct object_directory *odb, cleanup: free(ctx->graph_name); + free(ctx->base_graph_name); free(ctx->commits.list); oid_array_clear(&ctx->oids); clear_topo_level_slab(&topo_levels); diff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh index e9c521c061..ca476e80a0 100755 --- a/t/t5328-commit-graph-64bit-time.sh +++ b/t/t5328-commit-graph-64bit-time.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='commit graph with 64-bit timestamps' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh if ! test_have_prereq TIME_IS_64BIT || ! test_have_prereq TIME_T_IS_64BIT From da09e7af68247519e2b19fc8dff113896c39ac3c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:31:30 -0400 Subject: [PATCH 10/10] commit-graph: clear oidset after finishing write In graph_write() we store commits in an oidset, but never clean it up, leaking the contents. We should clear it in the cleanup section. The oidset comes from 6830c36077 (commit-graph.h: replace 'commit_hex' with 'commits', 2020-04-13), but it was just replacing a string_list that was also leaked. Curiously, we fixed the leak of some adjacent variables in commit fa8953cb40 (builtin/commit-graph.c: extract 'read_one_commit()', 2020-05-18), but the oidset wasn't included for some reason. In combination with the preceding commits, this lets us mark t5324 as leak-free. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 1 + t/t5324-split-commit-graph.sh | 2 ++ 2 files changed, 3 insertions(+) diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index c88389df24..c527a8369e 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -311,6 +311,7 @@ cleanup: FREE_AND_NULL(options); string_list_clear(&pack_indexes, 0); strbuf_release(&buf); + oidset_clear(&commits); return result; } diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 36c4141e67..52e8a3e619 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='split commit graph' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_COMMIT_GRAPH=0