git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()
@ 2021-02-19 20:13 Andrzej Hunt via GitGitGadget
  2021-02-20  3:36 ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-02-19 20:13 UTC (permalink / raw)
  To: git; +Cc: Andrzej Hunt, Andrzej Hunt

From: Andrzej Hunt <ajrhunt@google.com>

write_commit_graph initialises topo_levels using init_topo_level_slab(),
next it calls compute_topological_levels() which can cause the slab to
grow, we therefore need to clear the slab again using
clear_topo_level_slab() when we're done.

First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which
is currently only in master and not on maint.

LeakSanitizer output:

==1026==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x498ae9 in realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xafbed8 in xrealloc /src/git/wrapper.c:126:8
    #2 0x7966d1 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
    #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
    #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
    #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
    #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
    #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
    #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
    #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
    #10 0x4cd084 in run_argv /src/git/git.c:771:4
    #11 0x4ca424 in cmd_main /src/git/git.c:902:19
    #12 0x707fb6 in main /src/git/common-main.c:52:11
    #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)

Indirect leak of 524256 byte(s) in 1 object(s) allocated from:
    #0 0x498942 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0xafc088 in xcalloc /src/git/wrapper.c:140:8
    #2 0x796870 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
    #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
    #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
    #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
    #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
    #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
    #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
    #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
    #10 0x4cd084 in run_argv /src/git/git.c:771:4
    #11 0x4ca424 in cmd_main /src/git/git.c:902:19
    #12 0x707fb6 in main /src/git/common-main.c:52:11
    #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)

SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
    commit-graph: avoid leaking topo_levels slab in write_commit_graph()
    
    write_commit_graph initialises topo_levels using init_topo_level_slab(),
    next it calls compute_topological_levels() which can cause the slab to
    grow, we therefore need to clear the slab again using
    clear_topo_level_slab() when we're done.
    
    First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which is
    currently only in master and not on maint.
    
    LeakSanitizer output:
    
    ==1026==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x498ae9 in
    realloc
    /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1
    0xafbed8 in xrealloc /src/git/wrapper.c:126:8 #2 0x7966d1 in
    topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in
    topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in
    compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3
    in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in
    graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in
    cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in
    run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin
    /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11
    0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main
    /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main
    (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
    
    Indirect leak of 524256 byte(s) in 1 object(s) allocated from: #0
    0x498942 in calloc
    /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1
    0xafc088 in xcalloc /src/git/wrapper.c:140:8 #2 0x796870 in
    topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in
    topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in
    compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3
    in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in
    graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in
    cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in
    run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin
    /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11
    0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main
    /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main
    (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
    
    SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-881%2Fahunt%2Fcommit-graph-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-881/ahunt/commit-graph-leak-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/881

 commit-graph.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/commit-graph.c b/commit-graph.c
index ed31843fa522..9529ec552139 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2471,6 +2471,7 @@ int write_commit_graph(struct object_directory *odb,
 	free(ctx->graph_name);
 	free(ctx->commits.list);
 	oid_array_clear(&ctx->oids);
+	clear_topo_level_slab(&topo_levels);
 
 	if (ctx->commit_graph_filenames_after) {
 		for (i = 0; i < ctx->num_commit_graphs_after; i++) {

base-commit: 2283e0e9af55689215afa39c03beb2315ce18e83
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()
  2021-02-19 20:13 [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph() Andrzej Hunt via GitGitGadget
@ 2021-02-20  3:36 ` Junio C Hamano
  2021-02-22 14:15   ` Derrick Stolee
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2021-02-20  3:36 UTC (permalink / raw)
  To: Andrzej Hunt via GitGitGadget, Derrick Stolee, Abhishek Kumar,
	Taylor Blau
  Cc: git, Andrzej Hunt, Andrzej Hunt

"Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Andrzej Hunt <ajrhunt@google.com>
>
> write_commit_graph initialises topo_levels using init_topo_level_slab(),
> next it calls compute_topological_levels() which can cause the slab to
> grow, we therefore need to clear the slab again using
> clear_topo_level_slab() when we're done.
>
> First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which
> is currently only in master and not on maint.

Thanks.  

Forwarding to those who were involved in the said commit for
insights.



> LeakSanitizer output:
>
> ==1026==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 8 byte(s) in 1 object(s) allocated from:
>     #0 0x498ae9 in realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xafbed8 in xrealloc /src/git/wrapper.c:126:8
>     #2 0x7966d1 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
>     #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
>     #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
>     #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
>     #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
>     #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
>     #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
>     #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
>     #10 0x4cd084 in run_argv /src/git/git.c:771:4
>     #11 0x4ca424 in cmd_main /src/git/git.c:902:19
>     #12 0x707fb6 in main /src/git/common-main.c:52:11
>     #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
>
> Indirect leak of 524256 byte(s) in 1 object(s) allocated from:
>     #0 0x498942 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
>     #1 0xafc088 in xcalloc /src/git/wrapper.c:140:8
>     #2 0x796870 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
>     #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
>     #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
>     #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
>     #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
>     #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
>     #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
>     #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
>     #10 0x4cd084 in run_argv /src/git/git.c:771:4
>     #11 0x4ca424 in cmd_main /src/git/git.c:902:19
>     #12 0x707fb6 in main /src/git/common-main.c:52:11
>     #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
>
> SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).
>
> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
> ---
>     commit-graph: avoid leaking topo_levels slab in write_commit_graph()
>     
>     write_commit_graph initialises topo_levels using init_topo_level_slab(),
>     next it calls compute_topological_levels() which can cause the slab to
>     grow, we therefore need to clear the slab again using
>     clear_topo_level_slab() when we're done.
>     
>     First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which is
>     currently only in master and not on maint.
>     
>     LeakSanitizer output:
>     
>     ==1026==ERROR: LeakSanitizer: detected memory leaks
>     
>     Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x498ae9 in
>     realloc
>     /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1
>     0xafbed8 in xrealloc /src/git/wrapper.c:126:8 #2 0x7966d1 in
>     topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in
>     topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in
>     compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3
>     in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in
>     graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in
>     cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in
>     run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin
>     /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11
>     0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main
>     /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main
>     (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
>     
>     Indirect leak of 524256 byte(s) in 1 object(s) allocated from: #0
>     0x498942 in calloc
>     /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1
>     0xafc088 in xcalloc /src/git/wrapper.c:140:8 #2 0x796870 in
>     topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in
>     topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in
>     compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3
>     in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in
>     graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in
>     cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in
>     run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin
>     /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11
>     0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main
>     /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main
>     (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
>     
>     SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-881%2Fahunt%2Fcommit-graph-leak-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-881/ahunt/commit-graph-leak-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/881
>
>  commit-graph.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index ed31843fa522..9529ec552139 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2471,6 +2471,7 @@ int write_commit_graph(struct object_directory *odb,
>  	free(ctx->graph_name);
>  	free(ctx->commits.list);
>  	oid_array_clear(&ctx->oids);
> +	clear_topo_level_slab(&topo_levels);
>  
>  	if (ctx->commit_graph_filenames_after) {
>  		for (i = 0; i < ctx->num_commit_graphs_after; i++) {
>
> base-commit: 2283e0e9af55689215afa39c03beb2315ce18e83

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()
  2021-02-20  3:36 ` Junio C Hamano
@ 2021-02-22 14:15   ` Derrick Stolee
  2021-02-22 19:14     ` Andrzej Hunt
  0 siblings, 1 reply; 5+ messages in thread
From: Derrick Stolee @ 2021-02-22 14:15 UTC (permalink / raw)
  To: Junio C Hamano, Andrzej Hunt via GitGitGadget, Derrick Stolee,
	Abhishek Kumar, Taylor Blau
  Cc: git, Andrzej Hunt, Andrzej Hunt

On 2/19/2021 10:36 PM, Junio C Hamano wrote:
> "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Andrzej Hunt <ajrhunt@google.com>
>>
>> write_commit_graph initialises topo_levels using init_topo_level_slab(),
>> next it calls compute_topological_levels() which can cause the slab to
>> grow, we therefore need to clear the slab again using
>> clear_topo_level_slab() when we're done.
>>
>> First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which
>> is currently only in master and not on maint.
> 
> Thanks.  
> 
> Forwarding to those who were involved in the said commit for
> insights.

>> index ed31843fa522..9529ec552139 100644
>> --- a/commit-graph.c
>> +++ b/commit-graph.c
>> @@ -2471,6 +2471,7 @@ int write_commit_graph(struct object_directory *odb,
>>  	free(ctx->graph_name);
>>  	free(ctx->commits.list);
>>  	oid_array_clear(&ctx->oids);
>> +	clear_topo_level_slab(&topo_levels);

This change looks like a sane change to me. It definitely fixes a leak.
The leak "wasn't hurting anybody" because write_commit_graph() is only
called at most once per process, and the process closes itself out
shortly after. Still, it's good to have good memory hygiene here.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()
  2021-02-22 14:15   ` Derrick Stolee
@ 2021-02-22 19:14     ` Andrzej Hunt
  0 siblings, 0 replies; 5+ messages in thread
From: Andrzej Hunt @ 2021-02-22 19:14 UTC (permalink / raw)
  To: Derrick Stolee, Junio C Hamano, Andrzej Hunt via GitGitGadget,
	Derrick Stolee, Abhishek Kumar, Taylor Blau
  Cc: git, Andrzej Hunt

On 22/02/2021 15:15, Derrick Stolee wrote:
> This change looks like a sane change to me. It definitely fixes a leak.
> The leak "wasn't hurting anybody" because write_commit_graph() is only
> called at most once per process, and the process closes itself out
> shortly after. Still, it's good to have good memory hygiene here.

Good to know - thank you! As I become more familiar with git, I'm 
beginning to realise that most leaks are unlikely to be much importance 
(even though I personally err on the side of fixing any and all issues).


One thing I forgot to mention: in this specific case the leak was 
causing a build failure when trying to build git's fuzzers within 
oss-fuzz locally*. Specifically the following command would fail (see 
also fuzz failure reproduction instructions which describe the setup [1]).

  $ python infra/helper.py build_fuzzers --sanitizer address git

As far as I can tell the issue is that: a copy of git built with ASAN is 
used to produce the fuzzing corpus as part of the git-specific build 
script [2] - the leak warning causes the script to fail. (It's possible 
to argue that the build script should disable ASAN's leak checking when 
running git, via detect_leaks=0 to reduce the risk of such breakage - I 
may try to suggest such a change to oss-fuzz.)

ATB,
   Andrzej


* Given that oss-fuzz is building via docker, I would intuitively 
suspect that the same issue occurs in automation - I'm not sure how to 
verify this myself.

[1] 
https://google.github.io/oss-fuzz/advanced-topics/reproducing/#building-using-docker
[2] 
https://github.com/google/oss-fuzz/blob/1b0115eefd70491376cf3cb6f88e49632c78ee18/projects/git/build.sh#L37

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()
@ 2021-02-25  6:26 Abhishek Kumar
  0 siblings, 0 replies; 5+ messages in thread
From: Abhishek Kumar @ 2021-02-25  6:26 UTC (permalink / raw)
  To: gitgitgadget; +Cc: ajrhunt, andrzej, git

"Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Andrzej Hunt <ajrhunt@google.com>
> 
> write_commit_graph initialises topo_levels using init_topo_level_slab(),
> next it calls compute_topological_levels() which can cause the slab to
> grow, we therefore need to clear the slab again using
> clear_topo_level_slab() when we're done.
> 
> First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which
> is currently only in master and not on maint.
> 

Thanks for identifying and fixing this memory leak!
- Abhishek

> LeakSanitizer output:
> ==1026==ERROR: LeakSanitizer: detected memory leaks

> Direct leak of 8 byte(s) in 1 object(s) allocated from:
>     #0 0x498ae9 in realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xafbed8 in xrealloc /src/git/wrapper.c:126:8
>     #2 0x7966d1 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
>     #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
>     #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
>     #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
>     #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
>     #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
>     #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
>     #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
>     #10 0x4cd084 in run_argv /src/git/git.c:771:4
>     #11 0x4ca424 in cmd_main /src/git/git.c:902:19
>     #12 0x707fb6 in main /src/git/common-main.c:52:11
>     #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)

> Indirect leak of 524256 byte(s) in 1 object(s) allocated from:
>     #0 0x498942 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
>     #1 0xafc088 in xcalloc /src/git/wrapper.c:140:8
>     #2 0x796870 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
>     #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
>     #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
>     #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
>     #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
>     #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
>     #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
>     #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
>     #10 0x4cd084 in run_argv /src/git/git.c:771:4
>     #11 0x4ca424 in cmd_main /src/git/git.c:902:19
>     #12 0x707fb6 in main /src/git/common-main.c:52:11
>     #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)

> SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).

> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
> ---
>     commit-graph: avoid leaking topo_levels slab in write_commit_graph()
>     
>     write_commit_graph initialises topo_levels using init_topo_level_slab(),
>     next it calls compute_topological_levels() which can cause the slab to
>     grow, we therefore need to clear the slab again using
>     clear_topo_level_slab() when we're done.
>     
>     First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which is
>     currently only in master and not on maint.
>     
>     LeakSanitizer output:
>     
>     ==1026==ERROR: LeakSanitizer: detected memory leaks
>     
>     Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x498ae9 in
>     realloc
>     /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1
>     0xafbed8 in xrealloc /src/git/wrapper.c:126:8 #2 0x7966d1 in
>     topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in
>     topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in
>     compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3
>     in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in
>     graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in
>     cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in
>     run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin
>     /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11
>     0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main
>     /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main
>     (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
>     
>     Indirect leak of 524256 byte(s) in 1 object(s) allocated from: #0
>     0x498942 in calloc
>     /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1
>     0xafc088 in xcalloc /src/git/wrapper.c:140:8 #2 0x796870 in
>     topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in
>     topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in
>     compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3
>     in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in
>     graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in
>     cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in
>     run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin
>     /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11
>     0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main
>     /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main
>     (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
>     
>     SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-881%2Fahunt%2Fcommit-graph-leak-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-881/ahunt/commit-graph-leak-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/881

>  commit-graph.c | 1 +
>  1 file changed, 1 insertion(+)

> diff --git a/commit-graph.c b/commit-graph.c
> index ed31843fa522..9529ec552139 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2471,6 +2471,7 @@ int write_commit_graph(struct object_directory *odb,
>         free(ctx->graph_name);
>         free(ctx->commits.list);
>         oid_array_clear(&ctx->oids);
> +	clear_topo_level_slab(&topo_levels);
>  
>         if (ctx->commit_graph_filenames_after) {
>                 for (i = 0; i < ctx->num_commit_graphs_after; i++) {

> base-commit: 2283e0e9af55689215afa39c03beb2315ce18e83
> -- 
> gitgitgadget

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-02-25  6:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-19 20:13 [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph() Andrzej Hunt via GitGitGadget
2021-02-20  3:36 ` Junio C Hamano
2021-02-22 14:15   ` Derrick Stolee
2021-02-22 19:14     ` Andrzej Hunt
2021-02-25  6:26 Abhishek Kumar

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).