threads / patch / 63579

patchcommit-graph: fix start_delayed_progress() leak

Subject: [PATCH] commit-graph: fix start_delayed_progress() leak

## tl;dr

3 messages between Jun 4, 2025 and Jun 4, 2025. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Lidong Yan via GitGitGadget· Jun 4, 2025, 03:11 UTC · lore
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(+)
Show changes to builtin/commit-graph.c +1 −0
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· Jun 4, 2025, 07:42 UTC · re: Lidong Yan via GitGitGadget · lore

Re: [PATCH] commit-graph: fix start_delayed_progress() leak

On Wed, Jun 04, 2025 at 03:11:15AM +0000, Lidong Yan via GitGitGadget wrote:
Show 7 quoted lines
> 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.
Show 9 quoted lines
> 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· Jun 4, 2025, 07:47 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] commit-graph: fix start_delayed_progress() leak

2025年6月4日 15:42，Patrick Steinhardt <ps@pks.im> 写道：
Show 13 quoted lines
> 
> 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.
Show 17 quoted lines
> 
>> 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

← back to recent threads