threads / patch / 60624

patchcommit-graph: fix memory leak when not writing graph

Subject: [PATCH] commit-graph: fix memory leak when not writing graph

## tl;dr

3 messages between Dec 18, 2023 and Jan 15, 2024. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Patrick Steinhardt· Dec 18, 2023, 10:02 UTC · lore

When `write_commit_graph()` bails out writing a split commit-graph early then it may happen that we have already gathered the set of existing commit-graph file names without yet determining the new merged set of files. This can result in a memory leak though because we only clear the preimage of files when we have collected the postimage.

Fix this issue by dropping the condition altogether so that we always try to free both preimage and postimage filenames. As the context structure is zero-initialized this simplification is safe to do.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit-graph.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
Show changes to commit-graph.c +8 −11
diff --git a/commit-graph.c b/commit-graph.c
index acac9bf6e1..afe0177ab2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2617,19 +2617,16 @@ int write_commit_graph(struct object_directory *odb,
 	oid_array_clear(&ctx->oids);
 	clear_topo_level_slab(&topo_levels);
 
-	if (ctx->commit_graph_filenames_after) {
-		for (i = 0; i < ctx->num_commit_graphs_after; i++) {
-			free(ctx->commit_graph_filenames_after[i]);
-			free(ctx->commit_graph_hash_after[i]);
-		}
-
-		for (i = 0; i < ctx->num_commit_graphs_before; i++)
-			free(ctx->commit_graph_filenames_before[i]);
+	for (i = 0; i < ctx->num_commit_graphs_before; i++)
+		free(ctx->commit_graph_filenames_before[i]);
+	free(ctx->commit_graph_filenames_before);
 
-		free(ctx->commit_graph_filenames_after);
-		free(ctx->commit_graph_filenames_before);
-		free(ctx->commit_graph_hash_after);
+	for (i = 0; i < ctx->num_commit_graphs_after; i++) {
+		free(ctx->commit_graph_filenames_after[i]);
+		free(ctx->commit_graph_hash_after[i]);
 	}
+	free(ctx->commit_graph_filenames_after);
+	free(ctx->commit_graph_hash_after);
 
 	free(ctx);
 

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
-- 
2.43.GIT
Taylor Blau· Jan 5, 2024, 19:11 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] commit-graph: fix memory leak when not writing graph

On Mon, Dec 18, 2023 at 11:02:28AM +0100, Patrick Steinhardt wrote:
Show 9 quoted lines
> When `write_commit_graph()` bails out writing a split commit-graph early
> then it may happen that we have already gathered the set of existing
> commit-graph file names without yet determining the new merged set of
> files. This can result in a memory leak though because we only clear the
> preimage of files when we have collected the postimage.
>
> Fix this issue by dropping the condition altogether so that we always
> try to free both preimage and postimage filenames. As the context
> structure is zero-initialized this simplification is safe to do.
Looks obviously good to me, thanks for finding and fixing.

Thanks, Taylor

Patrick Steinhardt· Jan 15, 2024, 07:08 UTC · re: Taylor Blau · lore

Re: [PATCH] commit-graph: fix memory leak when not writing graph

On Fri, Jan 05, 2024 at 02:11:22PM -0500, Taylor Blau wrote:
Show 12 quoted lines
> On Mon, Dec 18, 2023 at 11:02:28AM +0100, Patrick Steinhardt wrote:
> > When `write_commit_graph()` bails out writing a split commit-graph early
> > then it may happen that we have already gathered the set of existing
> > commit-graph file names without yet determining the new merged set of
> > files. This can result in a memory leak though because we only clear the
> > preimage of files when we have collected the postimage.
> >
> > Fix this issue by dropping the condition altogether so that we always
> > try to free both preimage and postimage filenames. As the context
> > structure is zero-initialized this simplification is safe to do.
> 
> Looks obviously good to me, thanks for finding and fixing.

Cc'ing Junio so that this fix doesn't fall off the radar. I thought I saw the topic in "seen" once, but either I misremember or it got dropped from there.

Patrick

← back to recent threads