Skip to contentSkip to conversation

commit-graph: fix start_delayed_progress() leak

PATCH In review Lidong Yan via GitGitGadget 3 participants 3 messages Jun 4, 2025

lore
Share & tools
lore

Lidong Yan via GitGitGadget

author
#0 lore raw

PATCH 1/1 commit-graph: fix start_delayed_progress() leak

From: Lidong Yan <[email protected]>

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 <[email protected]>

---
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(+)
builtin/commit-graph.c +1 -0
index a783a86e797..ee48980248f 100644
@@ -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

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 7 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

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 <[email protected]>

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.

builtin/commit-graph.c static int graph_write(int argc, const char **argv, const char *prefix, · line 314

Quoted 9 lines
builtin/commit-graph.c +1 -0
index a783a86e797..ee48980248f 100644
@@ -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

#2 lore raw
Quoted 8 lines · 2025年6月4日 15:42，Patrick Steinhardt <[email protected]> 写道：
Quoted 6 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Lidong Yan <[email protected]>

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.

Quoted 15 lines

builtin/commit-graph.c static int graph_write(int argc, const char **argv, const char *prefix, · line 314

Quoted 10 lines
builtin/commit-graph.c +1 -0
index a783a86e797..ee48980248f 100644
@@ -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!

Quoted 2 lines

Patrick

Thanks,
Lidong