# [PATCH] commit-graph: fix start_delayed_progress() leak 3 messages from 2025-06-04 to 2025-06-04. Participants: Lidong Yan via GitGitGadget, Patrick Steinhardt, lidongyan. Thread: https://gitlist.dev/t/63579 ## Lidong Yan via GitGitGadget, 2025-06-04 03:11 Subject: [PATCH] commit-graph: fix start_delayed_progress() leak Message-ID: URL: https://gitlist.dev/e/pull.1986.git.git.1749006675784.gitgitgadget%40gmail.com ``` From: Lidong Yan <502024330056@smail.nju.edu.cn> In commit-graph.c:graph_write(), if read_one_commit() failed, progress allocated in start_delayed_progress() will leak. Add stop_progress() before goto cleanup. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> --- commit-graph: fix start_delayed_progress() leak In commit-graph.c:graph_write(), if read_one_commit() failed, progress allocated in start_delayed_progress() will leak. Add stop_progress() before goto cleanup. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1986%2Fbrandb97%2Ffix-graph-write-leak-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1986/brandb97/fix-graph-write-leak-v1 Pull-Request: https://github.com/git/git/pull/1986 builtin/commit-graph.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index a783a86e797..ee48980248f 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix, while (strbuf_getline(&buf, stdin) != EOF) { if (read_one_commit(&commits, progress, buf.buf)) { result = 1; + stop_progress(&progress); goto cleanup; } } base-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67 -- gitgitgadget ``` ## Patrick Steinhardt, 2025-06-04 07:42 Subject: Re: [PATCH] commit-graph: fix start_delayed_progress() leak Message-ID: URL: https://gitlist.dev/e/aD_46Qxh9oVj-P3U%40pks.im In-Reply-To: ``` On Wed, Jun 04, 2025 at 03:11:15AM +0000, Lidong Yan via GitGitGadget wrote: > From: Lidong Yan <502024330056@smail.nju.edu.cn> > > In commit-graph.c:graph_write(), if read_one_commit() failed, > progress allocated in start_delayed_progress() will leak. Add > stop_progress() before goto cleanup. > > Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> Nit: it might make sense to send multiple patches that are related, like your memory leak fixes, in the same patch series. That makes it a bit easier for reviewers to group together related reviews. > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index a783a86e797..ee48980248f 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix, > while (strbuf_getline(&buf, stdin) != EOF) { > if (read_one_commit(&commits, progress, buf.buf)) { > result = 1; > + stop_progress(&progress); This function calls `stop_progress_msg()`, which knows to exit in case `*progress` is a NULL pointer. We thus don't have to guard this line with `if (progress)`. So the patch looks good to me, thanks! Patrick ``` ## lidongyan, 2025-06-04 07:47 Subject: Re: [PATCH] commit-graph: fix start_delayed_progress() leak Message-ID: URL: https://gitlist.dev/e/CB88AA24-6D26-4BED-B430-62A453E6B9D6%40smail.nju.edu.cn In-Reply-To: ``` 2025年6月4日 15:42，Patrick Steinhardt 写道： > > On Wed, Jun 04, 2025 at 03:11:15AM +0000, Lidong Yan via GitGitGadget wrote: >> From: Lidong Yan <502024330056@smail.nju.edu.cn> >> >> In commit-graph.c:graph_write(), if read_one_commit() failed, >> progress allocated in start_delayed_progress() will leak. Add >> stop_progress() before goto cleanup. >> >> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> > > Nit: it might make sense to send multiple patches that are related, like > your memory leak fixes, in the same patch series. That makes it a bit > easier for reviewers to group together related reviews. Got it, though I think this is the last time I send leak-fix patches. > >> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c >> index a783a86e797..ee48980248f 100644 >> --- a/builtin/commit-graph.c >> +++ b/builtin/commit-graph.c >> @@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix, >> while (strbuf_getline(&buf, stdin) != EOF) { >> if (read_one_commit(&commits, progress, buf.buf)) { >> result = 1; >> + stop_progress(&progress); > > This function calls `stop_progress_msg()`, which knows to exit in case > `*progress` is a NULL pointer. We thus don't have to guard this line > with `if (progress)`. So the patch looks good to me, thanks! > > Patrick > Thanks, Lidong ```