[PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
    repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()
    
    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.

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(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 225cef08807..6172c8aacc9 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -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);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 30075b67be8..dfc7e85ae96 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -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
-- 
gitgitgadget

Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[Patrick Steinhardt]

On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote:
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 225cef08807..6172c8aacc9 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -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.

> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 30075b67be8..dfc7e85ae96 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -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)) {

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

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 30075b67be8..dd08bc40161 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -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);
 }

Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[lidongyan]

2025年6月4日 15:56，Patrick Steinhardt <ps@pks.im> 写道：
> 
> On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote:
>> diff --git a/builtin/replay.c b/builtin/replay.c
>> index 225cef08807..6172c8aacc9 100644
>> --- a/builtin/replay.c
>> +++ b/builtin/replay.c
>> @@ -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.
> 
>> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
>> index 30075b67be8..dfc7e85ae96 100644
>> --- a/builtin/shortlog.c
>> +++ b/builtin/shortlog.c
>> @@ -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)) {
> 
> 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
> 
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 30075b67be8..dd08bc40161 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -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);
> }
> 

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

[PATCH v2] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
    repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()
    
    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.

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(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 225cef08807..6172c8aacc9 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -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);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 30075b67be8..fe15e114973 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -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
-- 
gitgitgadget

Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[Jeff King]

On Wed, Jun 04, 2025 at 09:56:39AM +0200, Patrick Steinhardt wrote:

> On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote:
> > diff --git a/builtin/replay.c b/builtin/replay.c
> > index 225cef08807..6172c8aacc9 100644
> > --- a/builtin/replay.c
> > +++ b/builtin/replay.c
> > @@ -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.

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:

diff --git a/utf8.c b/utf8.c
index 35a0251939..d7b7d372c5 100644
--- a/utf8.c
+++ b/utf8.c
@@ -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.

> > -	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.

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

> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 30075b67be8..dd08bc40161 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -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

Re: [PATCH v2] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[Jeff King]

On Thu, Jun 05, 2025 at 06:27:26AM +0000, Lidong Yan via GitGitGadget wrote:

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> 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

Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()

[lidongyan]

2025年6月5日 15:23，Jeff King <peff@peff.net> 写道：
> 
> On Wed, Jun 04, 2025 at 09:56:39AM +0200, Patrick Steinhardt wrote:
> 
>> On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote:
>>> diff --git a/builtin/replay.c b/builtin/replay.c
>>> index 225cef08807..6172c8aacc9 100644
>>> --- a/builtin/replay.c
>>> +++ b/builtin/replay.c
>>> @@ -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.
> 
> 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. 
  
> 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.

> If we add a hack like this:
> 
> diff --git a/utf8.c b/utf8.c
> index 35a0251939..d7b7d372c5 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -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.
> 
>>> - 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.
> 
> Yeah, I think that is nicer, though...
> 
>> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
>> index 30075b67be8..dd08bc40161 100644
>> --- a/builtin/shortlog.c
>> +++ b/builtin/shortlog.c
>> @@ -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

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

Thanks,
Lidong