# [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph() 4 messages from 2021-02-19 to 2021-02-22. Participants: Andrzej Hunt via GitGitGadget, Junio C Hamano, Derrick Stolee, Andrzej Hunt. Thread: https://gitlist.dev/t/55176 ## Andrzej Hunt via GitGitGadget, 2021-02-19 20:13 Subject: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph() Message-ID: URL: https://gitlist.dev/e/pull.881.git.1613765590412.gitgitgadget%40gmail.com ``` From: Andrzej Hunt 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 --- 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 ``` ## Junio C Hamano, 2021-02-20 03:36 Subject: Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph() Message-ID: URL: https://gitlist.dev/e/xmqqa6rz9zrx.fsf%40gitster.g In-Reply-To: ``` "Andrzej Hunt via GitGitGadget" writes: > From: Andrzej Hunt > > 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 > --- > 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 ``` ## Derrick Stolee, 2021-02-22 14:15 Subject: Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph() Message-ID: <6ef5487b-905d-8f34-a53c-d1138f5528d9@gmail.com> URL: https://gitlist.dev/e/6ef5487b-905d-8f34-a53c-d1138f5528d9%40gmail.com In-Reply-To: ``` On 2/19/2021 10:36 PM, Junio C Hamano wrote: > "Andrzej Hunt via GitGitGadget" writes: > >> From: Andrzej Hunt >> >> 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 ``` ## Andrzej Hunt, 2021-02-22 19:14 Subject: Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph() Message-ID: URL: https://gitlist.dev/e/dfc713c2-c5f6-6c7f-230b-810da0e39ebf%40ahunt.org In-Reply-To: <6ef5487b-905d-8f34-a53c-d1138f5528d9@gmail.com> ``` 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 ```