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

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 4, 2025, 07:56 UTC
Message-ID: <aD_8NxMi6Dk7CmSl@pks.im>
In-Reply-To: <pull.1988.git.git.1749006607791.gitgitgadget@gmail.com>

On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote:

Show 12 quoted lines

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

Show 16 quoted lines

> 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);
 }