{"thread":{"id":"60624","subject":"[PATCH] commit-graph: fix memory leak when not writing graph","startedAt":"2023-12-18T10:02:33Z","lastAt":"2024-01-15T07:08:58Z","messageCount":3,"participants":["Patrick Steinhardt","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485771","messageId":"0feab5e7d5bc6275e2c7671cd8f6786ea86fd610.1702891190.git.ps@pks.im","threadId":"60624","inReplyTo":null,"subject":"[PATCH] commit-graph: fix memory leak when not writing graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-18T10:02:28Z","receivedAt":"2023-12-18T10:02:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When `write_commit_graph()` bails out writing a split commit-graph early\nthen it may happen that we have already gathered the set of existing\ncommit-graph file names without yet determining the new merged set of\nfiles. This can result in a memory leak though because we only clear the\npreimage of files when we have collected the postimage.\n\nFix this issue by dropping the condition altogether so that we always\ntry to free both preimage and postimage filenames. As the context\nstructure is zero-initialized this simplification is safe to do.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n commit-graph.c | 19 ++++++++-----------\n 1 file changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex acac9bf6e1..afe0177ab2 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2617,19 +2617,16 @@ int write_commit_graph(struct object_directory *odb,\n \toid_array_clear(&ctx->oids);\n \tclear_topo_level_slab(&topo_levels);\n \n-\tif (ctx->commit_graph_filenames_after) {\n-\t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\n-\t\t\tfree(ctx->commit_graph_filenames_after[i]);\n-\t\t\tfree(ctx->commit_graph_hash_after[i]);\n-\t\t}\n-\n-\t\tfor (i = 0; i < ctx->num_commit_graphs_before; i++)\n-\t\t\tfree(ctx->commit_graph_filenames_before[i]);\n+\tfor (i = 0; i < ctx->num_commit_graphs_before; i++)\n+\t\tfree(ctx->commit_graph_filenames_before[i]);\n+\tfree(ctx->commit_graph_filenames_before);\n \n-\t\tfree(ctx->commit_graph_filenames_after);\n-\t\tfree(ctx->commit_graph_filenames_before);\n-\t\tfree(ctx->commit_graph_hash_after);\n+\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\n+\t\tfree(ctx->commit_graph_filenames_after[i]);\n+\t\tfree(ctx->commit_graph_hash_after[i]);\n \t}\n+\tfree(ctx->commit_graph_filenames_after);\n+\tfree(ctx->commit_graph_hash_after);\n \n \tfree(ctx);\n \n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \n2.43.GIT\n\n"},{"id":"486353","messageId":"ZZhUWu5pgBEYK409@nand.local","threadId":"60624","inReplyTo":"0feab5e7d5bc6275e2c7671cd8f6786ea86fd610.1702891190.git.ps@pks.im","subject":"Re: [PATCH] commit-graph: fix memory leak when not writing graph","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-05T19:11:22Z","receivedAt":"2024-01-05T19:11:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 18, 2023 at 11:02:28AM +0100, Patrick Steinhardt wrote:\n> When `write_commit_graph()` bails out writing a split commit-graph early\n> then it may happen that we have already gathered the set of existing\n> commit-graph file names without yet determining the new merged set of\n> files. This can result in a memory leak though because we only clear the\n> preimage of files when we have collected the postimage.\n>\n> Fix this issue by dropping the condition altogether so that we always\n> try to free both preimage and postimage filenames. As the context\n> structure is zero-initialized this simplification is safe to do.\n\nLooks obviously good to me, thanks for finding and fixing.\n\nThanks,\nTaylor\n"},{"id":"486790","messageId":"ZaTaBN2Xc61sVvTJ@tanuki","threadId":"60624","inReplyTo":"ZZhUWu5pgBEYK409@nand.local","subject":"Re: [PATCH] commit-graph: fix memory leak when not writing graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-15T07:08:52Z","receivedAt":"2024-01-15T07:08:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 05, 2024 at 02:11:22PM -0500, Taylor Blau wrote:\n> On Mon, Dec 18, 2023 at 11:02:28AM +0100, Patrick Steinhardt wrote:\n> > When `write_commit_graph()` bails out writing a split commit-graph early\n> > then it may happen that we have already gathered the set of existing\n> > commit-graph file names without yet determining the new merged set of\n> > files. This can result in a memory leak though because we only clear the\n> > preimage of files when we have collected the postimage.\n> >\n> > Fix this issue by dropping the condition altogether so that we always\n> > try to free both preimage and postimage filenames. As the context\n> > structure is zero-initialized this simplification is safe to do.\n> \n> Looks obviously good to me, thanks for finding and fixing.\n\nCc'ing Junio so that this fix doesn't fall off the radar. I thought I\nsaw the topic in \"seen\" once, but either I misremember or it got dropped\nfrom there.\n\nPatrick\n"}]}