{"thread":{"id":"63579","subject":"[PATCH] commit-graph: fix start_delayed_progress() leak","startedAt":"2025-06-04T03:11:18Z","lastAt":"2025-06-04T07:48:23Z","messageCount":3,"participants":["Lidong Yan via GitGitGadget","Patrick Steinhardt","lidongyan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"519630","messageId":"pull.1986.git.git.1749006675784.gitgitgadget@gmail.com","threadId":"63579","inReplyTo":null,"subject":"[PATCH] commit-graph: fix start_delayed_progress() leak","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-04T03:11:15Z","receivedAt":"2025-06-04T03:11:18Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn commit-graph.c:graph_write(), if read_one_commit() failed,\nprogress allocated in start_delayed_progress() will leak. Add\nstop_progress() before goto cleanup.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    commit-graph: fix start_delayed_progress() leak\n    \n    In commit-graph.c:graph_write(), if read_one_commit() failed, progress\n    allocated in start_delayed_progress() will leak. Add stop_progress()\n    before goto cleanup.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1986%2Fbrandb97%2Ffix-graph-write-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1986/brandb97/fix-graph-write-leak-v1\nPull-Request: https://github.com/git/git/pull/1986\n\n builtin/commit-graph.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex a783a86e797..ee48980248f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix,\n \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\t\tif (read_one_commit(&commits, progress, buf.buf)) {\n \t\t\t\tresult = 1;\n+\t\t\t\tstop_progress(&progress);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n\nbase-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67\n-- \ngitgitgadget\n"},{"id":"519636","messageId":"aD_46Qxh9oVj-P3U@pks.im","threadId":"63579","inReplyTo":"pull.1986.git.git.1749006675784.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: fix start_delayed_progress() leak","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-06-04T07:42:33Z","receivedAt":"2025-06-04T07:42:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 04, 2025 at 03:11:15AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> In commit-graph.c:graph_write(), if read_one_commit() failed,\n> progress allocated in start_delayed_progress() will leak. Add\n> stop_progress() before goto cleanup.\n> \n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nNit: it might make sense to send multiple patches that are related, like\nyour memory leak fixes, in the same patch series. That makes it a bit\neasier for reviewers to group together related reviews.\n\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index a783a86e797..ee48980248f 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix,\n>  \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n>  \t\t\tif (read_one_commit(&commits, progress, buf.buf)) {\n>  \t\t\t\tresult = 1;\n> +\t\t\t\tstop_progress(&progress);\n\nThis function calls `stop_progress_msg()`, which knows to exit in case\n`*progress` is a NULL pointer. We thus don't have to guard this line\nwith `if (progress)`. So the patch looks good to me, thanks!\n\nPatrick\n"},{"id":"519637","messageId":"CB88AA24-6D26-4BED-B430-62A453E6B9D6@smail.nju.edu.cn","threadId":"63579","inReplyTo":"aD_46Qxh9oVj-P3U@pks.im","subject":"Re: [PATCH] commit-graph: fix start_delayed_progress() leak","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-04T07:47:28Z","receivedAt":"2025-06-04T07:48:23Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年6月4日 15:42，Patrick Steinhardt <ps@pks.im> 写道：\n> \n> On Wed, Jun 04, 2025 at 03:11:15AM +0000, Lidong Yan via GitGitGadget wrote:\n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> In commit-graph.c:graph_write(), if read_one_commit() failed,\n>> progress allocated in start_delayed_progress() will leak. Add\n>> stop_progress() before goto cleanup.\n>> \n>> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> Nit: it might make sense to send multiple patches that are related, like\n> your memory leak fixes, in the same patch series. That makes it a bit\n> easier for reviewers to group together related reviews.\n\nGot it, though I think this is the last time I send leak-fix patches.\n\n> \n>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n>> index a783a86e797..ee48980248f 100644\n>> --- a/builtin/commit-graph.c\n>> +++ b/builtin/commit-graph.c\n>> @@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix,\n>> while (strbuf_getline(&buf, stdin) != EOF) {\n>> if (read_one_commit(&commits, progress, buf.buf)) {\n>> result = 1;\n>> + stop_progress(&progress);\n> \n> This function calls `stop_progress_msg()`, which knows to exit in case\n> `*progress` is a NULL pointer. We thus don't have to guard this line\n> with `if (progress)`. So the patch looks good to me, thanks!\n> \n> Patrick\n> \n\nThanks,\nLidong\n\n"}]}