[PATCH] builtin/history: unuse the commit buffer after use

[PATCH] builtin/history: unuse the commit buffer after use

[Kaartic Sivaraam]

While running `git history reword` using a Git built with `SANITIZE` flag set
to `address,leak`, we could observe the following leak being reported:

-- 8< --

=================================================================
==7156==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1813 byte(s) in 1 object(s) allocated from:
    #0 0x79a3d1d2b60f in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:67
    #1 0x612e2bb8c2e9 in do_xmalloc /path/to/git/wrapper.c:55
    #2 0x612e2bb8c3f7 in do_xmallocz /path/to/git/wrapper.c:89
    #3 0x612e2bb8c48f in xmallocz_gently /path/to/git/wrapper.c:102
    #4 0x612e2b8dc28e in unpack_compressed_entry /path/to/git/packfile.c:1744
    #5 0x612e2b8dd12a in unpack_entry /path/to/git/packfile.c:1897
    #6 0x612e2b8daae2 in cache_or_unpack_entry /path/to/git/packfile.c:1535
    #7 0x612e2b8db1f6 in packed_object_info_with_index_pos /path/to/git/packfile.c:1617
    #8 0x612e2b8dc1c4 in packed_object_info /path/to/git/packfile.c:1732
    #9 0x612e2b8def05 in packfile_store_read_object_info /path/to/git/packfile.c:2228
    #10 0x612e2b889cb0 in odb_source_files_read_object_info odb/source-files.c:58
    #11 0x612e2b8805e9 in odb_source_read_object_info odb/source.h:326
    #12 0x612e2b885cf0 in do_oid_object_info_extended /path/to/git/odb.c:572
    #13 0x612e2b886fe4 in odb_read_object_info_extended /path/to/git/odb.c:710
    #14 0x612e2b887584 in odb_read_object /path/to/git/odb.c:756
    #15 0x612e2b68d6e4 in repo_get_commit_buffer /path/to/git/commit.c:399
    #16 0x612e2b91a7d4 in repo_logmsg_reencode /path/to/git/pretty.c:716
    #17 0x612e2b3de7da in commit_tree_ext builtin/history.c:127
    #18 0x612e2b3dee9f in commit_tree_with_edited_message builtin/history.c:183
    #19 0x612e2b3e2c4d in cmd_history_reword builtin/history.c:717
    #20 0x612e2b3e53b6 in cmd_history builtin/history.c:998
    #21 0x612e2b27ae97 in run_builtin /path/to/git/git.c:506
    #22 0x612e2b27b9ae in handle_builtin /path/to/git/git.c:782
    #23 0x612e2b27c240 in run_argv /path/to/git/git.c:865
    #24 0x612e2b27cd94 in cmd_main /path/to/git/git.c:986
    #25 0x612e2b5c4267 in main /path/to/git/common-main.c:9
    #26 0x79a3d182a600 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:59
    #27 0x79a3d182a717 in __libc_start_main_impl ../csu/libc-start.c:360
    #28 0x612e2b276124 in _start (/path/to.local/bin/git+0x211124) (BuildId: 8da3d640a944e21b895fc4802d7942b1505be663)

SUMMARY: AddressSanitizer: 1813 byte(s) leaked in 1 allocation(s).

-- >8 --

A deeper investigation on this reveals the following as the root cause.

As part of rewording a commit in `git history`, we get the commit message
buffer in the `commit_tree_ext` function. This in turn obtains the buffer
from `repo_logmsg_reencode`. Given how `commit_tree_ext` is invoking the
function with the last two parameters as NULL, we are clearly not expecting
a reencode to happen. In this case, the buffer that we receive from
`repo_logmsg_reencode` ends up always being obtained from a call to
`repo_get_commit_buffer`.

This buffer is expected to be released with an accompanying call to
`repo_unuse_commit_buffer` which takes care of freeing it. This call
is missing in the `commit_tree_ext` flow thus resulting in the leak.

Fix this by ensuring we call `repo_unuse_commit_buffer` on the
original_message buffer.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
I must mention that I also noticed the following comment in `commit_tree_ext`:

»       /* We retain authorship of the original commit. */
»       original_message = repo_logmsg_reencode(repo, commit_with_message, NULL, NULL);

... but I'm not quite sure why we don't unuse the buffer after its purpose is
done. Kindly englighten me in case I missed something.


 builtin/history.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/history.c b/builtin/history.c
index 091465a59e..0e9259b5d7 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,
 	free_commit_extra_headers(original_extra_headers);
 	strbuf_release(&commit_message);
 	free(original_author);
+	repo_unuse_commit_buffer(repo, commit_with_message, original_message);
 	return ret;
 }
 
-- 
2.55.0.rc0.738.g0c8ab3ebcc.dirty

Re: [PATCH] builtin/history: unuse the commit buffer after use

[Patrick Steinhardt]

On Sun, Jun 14, 2026 at 02:15:40PM +0000, Kaartic Sivaraam wrote:
> While running `git history reword` using a Git built with `SANITIZE` flag set
> to `address,leak`, we could observe the following leak being reported:

Huh, curious. That seems to hint that we're missing test coverage for
this specific scenario, as our test suite doesn't detect this leak.

[snip]
> A deeper investigation on this reveals the following as the root cause.
> 
> As part of rewording a commit in `git history`, we get the commit message
> buffer in the `commit_tree_ext` function. This in turn obtains the buffer
> from `repo_logmsg_reencode`. Given how `commit_tree_ext` is invoking the
> function with the last two parameters as NULL, we are clearly not expecting
> a reencode to happen. In this case, the buffer that we receive from
> `repo_logmsg_reencode` ends up always being obtained from a call to
> `repo_get_commit_buffer`.
> 
> This buffer is expected to be released with an accompanying call to
> `repo_unuse_commit_buffer` which takes care of freeing it. This call
> is missing in the `commit_tree_ext` flow thus resulting in the leak.

So this doesn't really read specific at all, and I would have expected
us to hit this leak. Puzzling.

> Fix this by ensuring we call `repo_unuse_commit_buffer` on the
> original_message buffer.
> 
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
> I must mention that I also noticed the following comment in `commit_tree_ext`:
> 
> »       /* We retain authorship of the original commit. */
> »       original_message = repo_logmsg_reencode(repo, commit_with_message, NULL, NULL);
> 
> ... but I'm not quite sure why we don't unuse the buffer after its purpose is
> done. Kindly englighten me in case I missed something.

Did you maybe confuse "authorship" with "ownership" while reading the
comment? The comment only mentions that we retain the original "Author"
commit metadata, it doesn't refer to ownership of the underlying
objects.

> diff --git a/builtin/history.c b/builtin/history.c
> index 091465a59e..0e9259b5d7 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,
>  	free_commit_extra_headers(original_extra_headers);
>  	strbuf_release(&commit_message);
>  	free(original_author);
> +	repo_unuse_commit_buffer(repo, commit_with_message, original_message);
>  	return ret;
>  }

Yup, this makes sense to me.

Thanks!

Patrick