From ac6d45d11f24921ead899c74569b717a7895f4a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:27:52 -0400 Subject: 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(-) (limited to 'commit-graph.c') 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; } -- cgit v1.2.3 From 09a75abba4eabab3f2cc4474e5d2db3a33a3a682 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:29:30 -0400 Subject: 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(-) (limited to 'commit-graph.c') 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) -- cgit v1.2.3 From 991d549f74862d07a38a077f34c0deaf65607c74 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:30:04 -0400 Subject: 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(-) (limited to 'commit-graph.c') 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; } -- cgit v1.2.3 From 1d94abfe1eedb3f0a9de74ba59483ef8f10b352c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:30:44 -0400 Subject: 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(+) (limited to 'commit-graph.c') 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; -- cgit v1.2.3 From 274bfa7f28fea96fafa114f2508ebef53735d7b6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:30:55 -0400 Subject: 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(+) (limited to 'commit-graph.c') 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); -- cgit v1.2.3 From d9c84c6d67e3418bade00af7bcb5f72d7c656164 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 3 Oct 2023 16:31:11 -0400 Subject: 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 + 1 file changed, 1 insertion(+) (limited to 'commit-graph.c') 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); -- cgit v1.2.3