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

[Junio C Hamano <gitster@pobox.com>]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> 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".

>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
> 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;
>  }