Re: [PATCH v2] builtin/history: unuse the commit buffer after use
From: Junio C Hamano <hidden>
Date: 2026-09-10 13:36:11
Kaartic Sivaraam [off-list ref] writes:
While running `git history reword` on a commit with `SANITIZE` flag set
to `address,leak`, we could observe the following leak being reported:
-- 8< --
=================================================================
==122337==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 263 byte(s) in 1 object(s) allocated from:
#0 0x7002c14fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x5cdd008ec077 in do_xmalloc /me/git/wrapper.c:55
#2 0x5cdd008ec185 in do_xmallocz /me/git/wrapper.c:89
#3 0x5cdd008ec1fa in xmallocz /me/git/wrapper.c:97
#4 0x5cdd005b99d8 in unpack_loose_rest /me/git/object-file.c:216
#5 0x5cdd005e45f4 in read_object_info_from_path odb/source-loose.c:174
#6 0x5cdd005e4ba0 in odb_source_loose_read_object_info odb/source-loose.c:235
#7 0x5cdd005d9f83 in odb_source_read_object_info odb/source.h:413
#8 0x5cdd005daaed in odb_source_files_read_object_info odb/source-files.c:93
#9 0x5cdd005d1c8c in odb_source_read_object_info odb/source.h:413
#10 0x5cdd005d5bdd in do_oid_object_info_extended /me/git/odb.c:592
#11 0x5cdd005d7080 in odb_read_object_info_extended /me/git/odb.c:747
#12 0x5cdd005d75d8 in odb_read_object /me/git/odb.c:793
#13 0x5cdd003d9af7 in repo_get_commit_buffer /me/git/commit.c:399
#14 0x5cdd006739ed in repo_logmsg_reencode /me/git/pretty.c:716
#15 0x5cdd0012287a in commit_tree_ext builtin/history.c:134
#16 0x5cdd00122f33 in commit_tree_with_edited_message builtin/history.c:190
#17 0x5cdd00126e44 in cmd_history_reword builtin/history.c:748
#18 0x5cdd0012b051 in cmd_history builtin/history.c:1209
#19 0x5cdcfffb8faf in run_builtin /me/git/git.c:510
#20 0x5cdcfffb9ac6 in handle_builtin /me/git/git.c:786
#21 0x5cdcfffba358 in run_argv /me/git/git.c:869
#22 0x5cdcfffbaea9 in cmd_main /me/git/git.c:990
#23 0x5cdd0030f27f in main /me/git/common-main.c:9
#24 0x7002c102a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#25 0x7002c102a28a in __libc_start_main_impl ../csu/libc-start.c:360
#26 0x5cdcfffb4134 in _start (/home/sivaraam/.local/bin/git+0x217134) (BuildId: 549c1036ab1f9f4fd55546e5bf31c7bd81b008fd)
-- >8 --
A deeper investigation on this reveals the following as the root cause.I am not sure if you are going to explain the root cause in such a way that is understandable by human readers, you would want to scare them away with a stack trace.
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`. 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`. The buffer that `repo_get_commit_buffer` ends up to be one that is not cached in the commit slab but a fresh buffer that is returned from `odb_read_object`. This could be confirmed confirmed by the stacktrace in the leak. A plausible reason for us receiving an uncached buffer might be because the commit comes from the commit-graph. In any case, this uncached buffer is expected to be released with an accompanying call to `repo_unuse_commit_buffer` which takes care of free-ing 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. For those who are curious, the following is a minimal way to reproduce the leak. I'm including this here as the leak does not happen when we get a cached commit obtained from the commit slab:
-- 8< -- $ git init scratch Initialized empty Git repository in /me/test-repos/scratch/.git/ $ cd scratch/ $ touch one && git add one && git commit -m "Commit one" [main (root-commit) 2182f9c] Commit one 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 one $ touch two && git add two && git commit -m "Commit two" [main 5550f33] Commit two 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 two $ git commit-graph write --reachable $ git history reword HEAD --dry-run update refs/heads/main eaded0872b14b3937605c77c0042429ca1e3bbe1 fd19e3776c75b8da9555c7c616ce0df9db7c6641 ================================================================= ==122337==ERROR: LeakSanitizer: detected memory leaks Direct leak of 263 byte(s) in 1 object(s) allocated from: ... snip ... SUMMARY: AddressSanitizer: 263 byte(s) leaked in 1 allocation(s). -- >8 -- This leak could also be triggered in our test suite if we run t3451-history-reword.sh as follows: -- 8< -- $ make SANITIZE=leak $ cd t $ GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh -v -i -- >8 --
Please do not abuse scissors line when you do not mean "discard all of the above and exclude it from the resulting commit log message".
quoted hunk ↗ jump to hunk
Helped-by: Jeff King [off-list ref] Signed-off-by: Kaartic Sivaraam <redacted> --- Changes since v2: Just updated the commit message to clarify the root cause more clearly. I haven't added an explicit test case as it wasn't clear if it is really worth it as Peff points out. Thank you, Peff, for your help with this! On a tangent, I noticed that the leak is only triggereable in the test suite, when we use `make SANITIZE=leak` and not when we use `make SANITIZE=address,leak`. It seems we intentionally disable leak detection in Asan via the following line in t/test-lib.sh: prepend_var ASAN_OPTIONS : detect_leaks=0 I noticed the comment above saying the following # If we were built with ASAN, it may complain about leaks # of program-lifetime variables. Disable it by default to lower # the noise level. I wonder if it has become stale now as we are fine with the test suite reporting leaks when we build with `make SANITIZE=leak`. Would it be worth while to avoid turning off detect_leaks while using Asan? 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; }