repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

PATCH Superseded Lidong Yan via GitGitGadget 4 participants 7 messages Jun 4, 2025 – Jun 6, 2025

lore

Share & tools

lore

Lidong Yan via GitGitGadget

author

2025-06-04 03:10 UTC #0 lore raw

PATCH 1/1 repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

From: Lidong Yan <[email protected]>

pretty.c:repo_logmsg_reencode() allocated memory should be freed with repo_unuse_commit_buffer(). Callers sometimes forgot free it at exit point. Add repo_unuse_commit_buffer() in insert_records_from_trailers at builtin/shortlog.c and create_commit at builtin/replay.c

Signed-off-by Lidong Yan <[email protected]>

---
repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1988%2Fbrandb97%2Ffix-reencode-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1988/brandb97/fix-reencode-leak-v1
Pull-Request: https://github.com/git/git/pull/1988

 builtin/replay.c   | 1 +
 builtin/shortlog.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

builtin/replay.c +1 -0

index 225cef08807..6172c8aacc9 100644
@@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo,
 	obj = parse_object(repo, &ret);
 
 out:
+	repo_unuse_commit_buffer(the_repository, based_on, message);
 	free_commit_extra_headers(extra);
 	free_commit_list(parents);
 	strbuf_release(&msg);

builtin/shortlog.c +3 -1

index 30075b67be8..dfc7e85ae96 100644
@@ -186,8 +186,10 @@ static void insert_records_from_trailers(struct shortlog *log,
 	commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL,
 					     ctx->output_encoding);
 	body = strstr(commit_buffer, "\n\n");
-	if (!body)
+	if (!body) {
+		repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
 		return;
+	}
 
 	trailer_iterator_init(&iter, body);
 	while (trailer_iterator_advance(&iter)) {

base-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2025-06-04 07:56 UTC #1 lore raw

builtin/replay.c static struct commit *create_commit(struct repository *repo, · line 90

Quoted 12 lines · Lidong Yan via GitGitGadget

builtin/replay.c +1 -0

index 225cef08807..6172c8aacc9 100644
@@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo,
 	obj = parse_object(repo, &ret);
 
 out:
+	repo_unuse_commit_buffer(the_repository, based_on, message);
 	free_commit_extra_headers(extra);
 	free_commit_list(parents);
 	strbuf_release(&msg);

Makes sense. This one _looks_ like a leak that I'd expect to hit in our test suite as it's not part of an error path.

Quoted 6 lines

builtin/shortlog.c +0 -0

index 30075b67be8..dfc7e85ae96 100644
@@ -186,8 +186,10 @@ static void insert_records_from_trailers(struct shortlog *log,
 	commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL,

builtin/shortlog.c static void insert_records_from_trailers(struct shortlog *log, · line 195

Quoted 10 lines

builtin/shortlog.c +3 -1

 					     ctx->output_encoding);
 	body = strstr(commit_buffer, "\n\n");
-	if (!body)
+	if (!body) {
+		repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
 		return;
+	}
 
 	trailer_iterator_init(&iter, body);
 	while (trailer_iterator_advance(&iter)) {

Should this one maybe be converted into a goto out so that we can release resources in a single location, only? Something like the below patch.

Patrick

builtin/shortlog.c +3 -2

index 30075b67be8..dd08bc40161 100644
@@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 	struct strbuf ident = STRBUF_INIT;
 
 	if (!log->trailers.nr)
-		return;
+		goto out;
 
 	/*
 	 * Using repo_format_commit_message("%B") would be simpler here, but

@@ -187,7 +187,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 					     ctx->output_encoding);
 	body = strstr(commit_buffer, "\n\n");
 	if (!body)
-		return;
+		goto out;
 
 	trailer_iterator_init(&iter, body);
 	while (trailer_iterator_advance(&iter)) {

@@ -206,6 +206,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 	}
 	trailer_iterator_release(&iter);
 
+out:
 	strbuf_release(&ident);
 	repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
 }

lidongyan

2025-06-04 11:00 UTC #2 lore raw

Quoted 65 lines · 2025年6月4日 15:56，Patrick Steinhardt <[email protected]> 写道：

Quoted 7 lines · Lidong Yan via GitGitGadget
builtin/replay.c +0 -0
index 225cef08807..6172c8aacc9 100644
@@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo,
 obj = parse_object(repo, &ret);
 
Quoted 5 lines
out:
+ repo_unuse_commit_buffer(the_repository, based_on, message); free_commit_extra_headers(extra); free_commit_list(parents);
strbuf_release(&msg);
Makes sense. This one _looks_ like a leak that I'd expect to hit in our test suite as it's not part of an error path.
Quoted 6 lines
builtin/shortlog.c +0 -0
index 30075b67be8..dfc7e85ae96 100644
@@ -186,8 +186,10 @@ static void insert_records_from_trailers(struct shortlog *log,
 commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL,
builtin/shortlog.c static void insert_records_from_trailers(struct shortlog *log, · line 195
Quoted 10 lines
builtin/shortlog.c +3 -1
    ctx->output_encoding);
 body = strstr(commit_buffer, "\n\n");
- if (!body)
+ if (!body) {
+ repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
 return;
+ }
 
 trailer_iterator_init(&iter, body);
 while (trailer_iterator_advance(&iter)) {
Should this one maybe be converted into a goto out so that we can release resources in a single location, only? Something like the below patch.
Patrick
builtin/shortlog.c +2 -2
index 30075b67be8..dd08bc40161 100644
@@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 struct strbuf ident = STRBUF_INIT;
 
 if (!log->trailers.nr)
- return;
+ goto out;
 
 /*
 * Using repo_format_commit_message("%B") would be simpler here, but
@@ -187,7 +187,7 @@ static void insert_records_from_trailers(struct shortlog *log,
    ctx->output_encoding);
 body = strstr(commit_buffer, "\n\n");
 if (!body)
- return;
+ goto out;
 
 trailer_iterator_init(&iter, body);
 while (trailer_iterator_advance(&iter)) {

builtin/shortlog.c static void insert_records_from_trailers(struct shortlog *log, · line 212

Quoted 7 lines

builtin/shortlog.c +1 -0

@@ -206,6 +206,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 }
 trailer_iterator_release(&iter);
 
+out:
 strbuf_release(&ident);
 repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
 }

Replace return with goto out do looks better. Ident initialized to STRBUF_INIT means ident->alloc = 0, so release on it is also safe.

Lidong Yan via GitGitGadget

author

2025-06-05 06:27 UTC #5 lore raw

PATCH v2 1/1 repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

In reply to Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

Signed-off-by Lidong Yan <[email protected]>

---
repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1988%2Fbrandb97%2Ffix-reencode-leak-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1988/brandb97/fix-reencode-leak-v2
Pull-Request: https://github.com/git/git/pull/1988

Range-diff vs v1:

1: a414074f167 ! 1: f5165d6a102 repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()
@@ builtin/replay.c: static struct commit *create_commit(struct repository *repo,

## builtin/shortlog.c ##
@@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *log,
- commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL,
ctx->output_encoding);
body = strstr(commit_buffer, "\n\n");
-- if (!body)
-+ if (!body) {
-+ repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
- return;
-+ }
+ if (!body)
+- return;
++ goto out;

trailer_iterator_init(&iter, body);
while (trailer_iterator_advance(&iter)) {
+@@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *log,
+ }
+ trailer_iterator_release(&iter);
+
++out:
+ strbuf_release(&ident);
+ repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
+ }

 builtin/replay.c   | 1 +
 builtin/shortlog.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

builtin/replay.c +1 -0

index 225cef08807..6172c8aacc9 100644
@@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo,
 	obj = parse_object(repo, &ret);
 
 out:
+	repo_unuse_commit_buffer(the_repository, based_on, message);
 	free_commit_extra_headers(extra);
 	free_commit_list(parents);
 	strbuf_release(&msg);

builtin/shortlog.c +2 -1

index 30075b67be8..fe15e114973 100644
@@ -187,7 +187,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 					     ctx->output_encoding);
 	body = strstr(commit_buffer, "\n\n");
 	if (!body)
-		return;
+		goto out;
 
 	trailer_iterator_init(&iter, body);
 	while (trailer_iterator_advance(&iter)) {

@@ -206,6 +206,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 	}
 	trailer_iterator_release(&iter);
 
+out:
 	strbuf_release(&ident);
 	repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
 }

base-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67

Signature · 1 line

gitgitgadget

Jeff King

2025-06-05 07:23 UTC #3 lore raw

In reply to Patrick Steinhardt

Quoted 8 lines · Patrick Steinhardt

Quoted 7 lines · Lidong Yan via GitGitGadget

builtin/replay.c +0 -0

index 225cef08807..6172c8aacc9 100644
@@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo,
 	obj = parse_object(repo, &ret);

Quoted 8 lines

builtin/replay.c static struct commit *create_commit(struct repository *repo, · line 90
Quoted 5 lines
builtin/replay.c +1 -0
 out:
+	repo_unuse_commit_buffer(the_repository, based_on, message);
 	free_commit_extra_headers(extra);
 	free_commit_list(parents);
 	strbuf_release(&msg);
Makes sense. This one _looks_ like a leak that I'd expect to hit in our test suite as it's not part of an error path.

We'll usually never flag a leak for commit buffers, because they are stored in (and owned by) a commit-slab. So the memory is not leaked exactly, but we may hold on to it longer than we need to. This mostly only becomes obvious when we do it for every commit in a code path that touches a lot of commits (e.g., "git log" or something).

The exception is if we actually had re-encode, which requires a mismatch between the commit and output encodings (which both default to UTF-8). And then it really is a leak.

If we add a hack like this:

utf8.c +7 -0

index 35a0251939..d7b7d372c5 100644
@@ -3,6 +3,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "utf8.h"
+#include "parse.h"
 
 /* This code is originally from https://www.cl.cam.ac.uk/~mgk25/ucs/ */

@@ -442,6 +443,12 @@ int is_encoding_utf8(const char *name)
 int same_encoding(const char *src, const char *dst)
 {
 	static const char utf8[] = "UTF-8";
+	static int always_reencode = -1;
+
+	if (always_reencode < 0)
+		always_reencode = git_env_bool("GIT_TEST_ALWAYS_REENCODE", 0);
+	if (always_reencode)
+		return 0;
 
 	if (!src)
 		src = utf8;

then running:

GIT_TEST_ALWAYS_REENCODE=1 make SANITIZE=leak test

turns up this leak via t3650-replay-basics.sh (as well as in t6429).

It's probably a bit too specialized to carry around as a permanent test mode, though. I thought it might find other cases, but it doesn't. The other one in this patch only triggers when the commit message has no header separator, which is not very likely.

Quoted 6 lines

builtin/replay.c +3 -1

-	if (!body)
+	if (!body) {
+		repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
 		return;
+	}

Quoted 6 lines

builtin/replay.c
Quoted 2 lines
builtin/replay.c +0 -0
 	trailer_iterator_init(&iter, body);
 	while (trailer_iterator_advance(&iter)) {
Should this one maybe be converted into a goto out so that we can release resources in a single location, only? Something like the below patch.

Yeah, I think that is nicer, though...

builtin/shortlog.c static void insert_records_from_trailers(struct shortlog *log, · line 180

Quoted 10 lines

builtin/shortlog.c +1 -1

index 30075b67be8..dd08bc40161 100644
@@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 	struct strbuf ident = STRBUF_INIT;
 
 	if (!log->trailers.nr)
-		return;
+		goto out;

If you convert this hunk, then we'd look at the uninitialized commit_buffer variable after we jump to the out label. I think the v2 just posted is OK, though (it touches only the one conditional that needs the goto).

-Peff

Jeff King

2025-06-05 07:53 UTC #6 lore raw

In reply to Lidong Yan via GitGitGadget

Quoted 6 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>
pretty.c:repo_logmsg_reencode() allocated memory should be freed with repo_unuse_commit_buffer(). Callers sometimes forgot free it at exit point. Add repo_unuse_commit_buffer() in insert_records_from_trailers at builtin/shortlog.c and create_commit at builtin/replay.c

The patch here looks fine, and I can confirm with the hacky test-patch I showed elsewhere in the thread that the case in replay.c is fixed.

I don't think the shortlog one is triggered by the test suite, and it's probably not worth adding a specific test for a commit with no header separator.

-Peff

lidongyan

2025-06-06 12:23 UTC #4 lore raw

In reply to Jeff King

Quoted 20 lines · 2025年6月5日 15:23，Jeff King <[email protected]> 写道：

Quoted 8 lines · Patrick Steinhardt
Quoted 7 lines · Lidong Yan via GitGitGadget
builtin/replay.c +0 -0
index 225cef08807..6172c8aacc9 100644
@@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo,
 obj = parse_object(repo, &ret);
 
Quoted 8 lines
Quoted 5 lines
out:
+ repo_unuse_commit_buffer(the_repository, based_on, message); free_commit_extra_headers(extra); free_commit_list(parents);
strbuf_release(&msg);
Makes sense. This one _looks_ like a leak that I'd expect to hit in our test suite as it's not part of an error path.

Quoted 5 lines

We'll usually never flag a leak for commit buffers, because they are stored in (and owned by) a commit-slab. So the memory is not leaked exactly, but we may hold on to it longer than we need to. This mostly only becomes obvious when we do it for every commit in a code path that touches a lot of commits (e.g., "git log" or something).

I understand. The static analysis tool which I used to test git find repo_logmsg_reencode() might allocates memory through xstrdup() or reencode_string(), then it report a leak. And I find that xstrdup() is actually dead code. So only reencode_string() may cause leaks.

Quoted 4 lines

The exception is if we actually had re-encode, which requires a mismatch between the commit and output encodings (which both default to UTF-8). And then it really is a leak.

Agreed.

Quoted 70 lines

If we add a hack like this:
utf8.c +7 -0
index 35a0251939..d7b7d372c5 100644
@@ -3,6 +3,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "utf8.h"
+#include "parse.h"
 
 /* This code is originally from https://www.cl.cam.ac.uk/~mgk25/ucs/ */
 
@@ -442,6 +443,12 @@ int is_encoding_utf8(const char *name)
 int same_encoding(const char *src, const char *dst)
 {
 static const char utf8[] = "UTF-8";
+ static int always_reencode = -1;
+
+ if (always_reencode < 0)
+ always_reencode = git_env_bool("GIT_TEST_ALWAYS_REENCODE", 0);
+ if (always_reencode)
+ return 0;
 
 if (!src)
 src = utf8;
then running:
GIT_TEST_ALWAYS_REENCODE=1 make SANITIZE=leak test
turns up this leak via t3650-replay-basics.sh (as well as in t6429).
It's probably a bit too specialized to carry around as a permanent test mode, though. I thought it might find other cases, but it doesn't. The other one in this patch only triggers when the commit message has no header separator, which is not very likely.
Quoted 6 lines
Quoted 6 lines
- if (!body)
+ if (!body) {
+ repo_unuse_commit_buffer(the_repository, commit, commit_buffer); return;
+ }
Quoted 6 lines
Quoted 2 lines
trailer_iterator_init(&iter, body);
while (trailer_iterator_advance(&iter)) {
Should this one maybe be converted into a goto out so that we can release resources in a single location, only? Something like the below patch.
Yeah, I think that is nicer, though...
builtin/shortlog.c static void insert_records_from_trailers(struct shortlog *log, · line 180
Quoted 10 lines
builtin/shortlog.c +1 -1
index 30075b67be8..dd08bc40161 100644
@@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 struct strbuf ident = STRBUF_INIT;
 
 if (!log->trailers.nr)
- return;
+ goto out;
If you convert this hunk, then we'd look at the uninitialized commit_buffer variable after we jump to the out label. I think the v2 just posted is OK, though (it touches only the one conditional that needs the goto).

Quoted 1 line

-Peff

I actually learn from your hack that GIT_TEST_ is something like GIT_TRACE, both aids to test and debug.

Thanks,
Lidong