{"thread":{"id":"55176","subject":"[PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()","startedAt":"2021-02-19T20:14:09Z","lastAt":"2021-02-22T19:29:20Z","messageCount":4,"participants":["Andrzej Hunt via GitGitGadget","Junio C Hamano","Derrick Stolee","Andrzej Hunt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"417418","messageId":"pull.881.git.1613765590412.gitgitgadget@gmail.com","threadId":"55176","inReplyTo":null,"subject":"[PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-19T20:13:10Z","receivedAt":"2021-02-19T20:14:09Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt <ajrhunt@google.com>\n\nwrite_commit_graph initialises topo_levels using init_topo_level_slab(),\nnext it calls compute_topological_levels() which can cause the slab to\ngrow, we therefore need to clear the slab again using\nclear_topo_level_slab() when we're done.\n\nFirst introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which\nis currently only in master and not on maint.\n\nLeakSanitizer output:\n\n==1026==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 8 byte(s) in 1 object(s) allocated from:\n    #0 0x498ae9 in realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n    #1 0xafbed8 in xrealloc /src/git/wrapper.c:126:8\n    #2 0x7966d1 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1\n    #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1\n    #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12\n    #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2\n    #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6\n    #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11\n    #8 0x4cddb1 in run_builtin /src/git/git.c:453:11\n    #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3\n    #10 0x4cd084 in run_argv /src/git/git.c:771:4\n    #11 0x4ca424 in cmd_main /src/git/git.c:902:19\n    #12 0x707fb6 in main /src/git/common-main.c:52:11\n    #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n\nIndirect leak of 524256 byte(s) in 1 object(s) allocated from:\n    #0 0x498942 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n    #1 0xafc088 in xcalloc /src/git/wrapper.c:140:8\n    #2 0x796870 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1\n    #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1\n    #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12\n    #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2\n    #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6\n    #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11\n    #8 0x4cddb1 in run_builtin /src/git/git.c:453:11\n    #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3\n    #10 0x4cd084 in run_argv /src/git/git.c:771:4\n    #11 0x4ca424 in cmd_main /src/git/git.c:902:19\n    #12 0x707fb6 in main /src/git/common-main.c:52:11\n    #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n\nSUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).\n\nSigned-off-by: Andrzej Hunt <ajrhunt@google.com>\n---\n    commit-graph: avoid leaking topo_levels slab in write_commit_graph()\n    \n    write_commit_graph initialises topo_levels using init_topo_level_slab(),\n    next it calls compute_topological_levels() which can cause the slab to\n    grow, we therefore need to clear the slab again using\n    clear_topo_level_slab() when we're done.\n    \n    First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which is\n    currently only in master and not on maint.\n    \n    LeakSanitizer output:\n    \n    ==1026==ERROR: LeakSanitizer: detected memory leaks\n    \n    Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x498ae9 in\n    realloc\n    /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1\n    0xafbed8 in xrealloc /src/git/wrapper.c:126:8 #2 0x7966d1 in\n    topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in\n    topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in\n    compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3\n    in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in\n    graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in\n    cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in\n    run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin\n    /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11\n    0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main\n    /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main\n    (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n    \n    Indirect leak of 524256 byte(s) in 1 object(s) allocated from: #0\n    0x498942 in calloc\n    /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1\n    0xafc088 in xcalloc /src/git/wrapper.c:140:8 #2 0x796870 in\n    topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in\n    topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in\n    compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3\n    in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in\n    graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in\n    cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in\n    run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin\n    /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11\n    0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main\n    /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main\n    (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n    \n    SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-881%2Fahunt%2Fcommit-graph-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-881/ahunt/commit-graph-leak-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/881\n\n commit-graph.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ed31843fa522..9529ec552139 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2471,6 +2471,7 @@ int write_commit_graph(struct object_directory *odb,\n \tfree(ctx->graph_name);\n \tfree(ctx->commits.list);\n \toid_array_clear(&ctx->oids);\n+\tclear_topo_level_slab(&topo_levels);\n \n \tif (ctx->commit_graph_filenames_after) {\n \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\n\nbase-commit: 2283e0e9af55689215afa39c03beb2315ce18e83\n-- \ngitgitgadget\n"},{"id":"417432","messageId":"xmqqa6rz9zrx.fsf@gitster.g","threadId":"55176","inReplyTo":"pull.881.git.1613765590412.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-20T03:36:02Z","receivedAt":"2021-02-20T03:36:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrzej Hunt via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Andrzej Hunt <ajrhunt@google.com>\n>\n> write_commit_graph initialises topo_levels using init_topo_level_slab(),\n> next it calls compute_topological_levels() which can cause the slab to\n> grow, we therefore need to clear the slab again using\n> clear_topo_level_slab() when we're done.\n>\n> First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which\n> is currently only in master and not on maint.\n\nThanks.  \n\nForwarding to those who were involved in the said commit for\ninsights.\n\n\n\n> LeakSanitizer output:\n>\n> ==1026==ERROR: LeakSanitizer: detected memory leaks\n>\n> Direct leak of 8 byte(s) in 1 object(s) allocated from:\n>     #0 0x498ae9 in realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n>     #1 0xafbed8 in xrealloc /src/git/wrapper.c:126:8\n>     #2 0x7966d1 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1\n>     #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1\n>     #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12\n>     #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2\n>     #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6\n>     #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11\n>     #8 0x4cddb1 in run_builtin /src/git/git.c:453:11\n>     #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3\n>     #10 0x4cd084 in run_argv /src/git/git.c:771:4\n>     #11 0x4ca424 in cmd_main /src/git/git.c:902:19\n>     #12 0x707fb6 in main /src/git/common-main.c:52:11\n>     #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n>\n> Indirect leak of 524256 byte(s) in 1 object(s) allocated from:\n>     #0 0x498942 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n>     #1 0xafc088 in xcalloc /src/git/wrapper.c:140:8\n>     #2 0x796870 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1\n>     #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1\n>     #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12\n>     #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2\n>     #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6\n>     #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11\n>     #8 0x4cddb1 in run_builtin /src/git/git.c:453:11\n>     #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3\n>     #10 0x4cd084 in run_argv /src/git/git.c:771:4\n>     #11 0x4ca424 in cmd_main /src/git/git.c:902:19\n>     #12 0x707fb6 in main /src/git/common-main.c:52:11\n>     #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n>\n> SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>\n> ---\n>     commit-graph: avoid leaking topo_levels slab in write_commit_graph()\n>     \n>     write_commit_graph initialises topo_levels using init_topo_level_slab(),\n>     next it calls compute_topological_levels() which can cause the slab to\n>     grow, we therefore need to clear the slab again using\n>     clear_topo_level_slab() when we're done.\n>     \n>     First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which is\n>     currently only in master and not on maint.\n>     \n>     LeakSanitizer output:\n>     \n>     ==1026==ERROR: LeakSanitizer: detected memory leaks\n>     \n>     Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x498ae9 in\n>     realloc\n>     /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1\n>     0xafbed8 in xrealloc /src/git/wrapper.c:126:8 #2 0x7966d1 in\n>     topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in\n>     topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in\n>     compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3\n>     in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in\n>     graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in\n>     cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in\n>     run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin\n>     /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11\n>     0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main\n>     /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main\n>     (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n>     \n>     Indirect leak of 524256 byte(s) in 1 object(s) allocated from: #0\n>     0x498942 in calloc\n>     /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1\n>     0xafc088 in xcalloc /src/git/wrapper.c:140:8 #2 0x796870 in\n>     topo_level_slab_at_peek /src/git/commit-graph.c:71:1 #3 0x7965e0 in\n>     topo_level_slab_at /src/git/commit-graph.c:71:1 #4 0x78fbf5 in\n>     compute_topological_levels /src/git/commit-graph.c:1472:12 #5 0x78c5c3\n>     in write_commit_graph /src/git/commit-graph.c:2456:2 #6 0x535c5f in\n>     graph_write /src/git/builtin/commit-graph.c:299:6 #7 0x5350ca in\n>     cmd_commit_graph /src/git/builtin/commit-graph.c:337:11 #8 0x4cddb1 in\n>     run_builtin /src/git/git.c:453:11 #9 0x4cabe2 in handle_builtin\n>     /src/git/git.c:704:3 #10 0x4cd084 in run_argv /src/git/git.c:771:4 #11\n>     0x4ca424 in cmd_main /src/git/git.c:902:19 #12 0x707fb6 in main\n>     /src/git/common-main.c:52:11 #13 0x7fee4249383f in __libc_start_main\n>     (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)\n>     \n>     SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-881%2Fahunt%2Fcommit-graph-leak-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-881/ahunt/commit-graph-leak-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/881\n>\n>  commit-graph.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index ed31843fa522..9529ec552139 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2471,6 +2471,7 @@ int write_commit_graph(struct object_directory *odb,\n>  \tfree(ctx->graph_name);\n>  \tfree(ctx->commits.list);\n>  \toid_array_clear(&ctx->oids);\n> +\tclear_topo_level_slab(&topo_levels);\n>  \n>  \tif (ctx->commit_graph_filenames_after) {\n>  \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\n>\n> base-commit: 2283e0e9af55689215afa39c03beb2315ce18e83\n"},{"id":"417468","messageId":"6ef5487b-905d-8f34-a53c-d1138f5528d9@gmail.com","threadId":"55176","inReplyTo":"xmqqa6rz9zrx.fsf@gitster.g","subject":"Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-22T14:15:15Z","receivedAt":"2021-02-22T14:19:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/19/2021 10:36 PM, Junio C Hamano wrote:\n> \"Andrzej Hunt via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Andrzej Hunt <ajrhunt@google.com>\n>>\n>> write_commit_graph initialises topo_levels using init_topo_level_slab(),\n>> next it calls compute_topological_levels() which can cause the slab to\n>> grow, we therefore need to clear the slab again using\n>> clear_topo_level_slab() when we're done.\n>>\n>> First introduced in 72a2bfcaf01860ce8dd6921490d903dc0ad59c89 - which\n>> is currently only in master and not on maint.\n> \n> Thanks.  \n> \n> Forwarding to those who were involved in the said commit for\n> insights.\n\n>> index ed31843fa522..9529ec552139 100644\n>> --- a/commit-graph.c\n>> +++ b/commit-graph.c\n>> @@ -2471,6 +2471,7 @@ int write_commit_graph(struct object_directory *odb,\n>>  \tfree(ctx->graph_name);\n>>  \tfree(ctx->commits.list);\n>>  \toid_array_clear(&ctx->oids);\n>> +\tclear_topo_level_slab(&topo_levels);\n\nThis change looks like a sane change to me. It definitely fixes a leak.\nThe leak \"wasn't hurting anybody\" because write_commit_graph() is only\ncalled at most once per process, and the process closes itself out\nshortly after. Still, it's good to have good memory hygiene here.\n\nThanks,\n-Stolee\n"},{"id":"417490","messageId":"dfc713c2-c5f6-6c7f-230b-810da0e39ebf@ahunt.org","threadId":"55176","inReplyTo":"6ef5487b-905d-8f34-a53c-d1138f5528d9@gmail.com","subject":"Re: [PATCH] commit-graph: avoid leaking topo_levels slab in write_commit_graph()","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-02-22T19:14:15Z","receivedAt":"2021-02-22T19:29:20Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"On 22/02/2021 15:15, Derrick Stolee wrote:\n> This change looks like a sane change to me. It definitely fixes a leak.\n> The leak \"wasn't hurting anybody\" because write_commit_graph() is only\n> called at most once per process, and the process closes itself out\n> shortly after. Still, it's good to have good memory hygiene here.\n\nGood to know - thank you! As I become more familiar with git, I'm \nbeginning to realise that most leaks are unlikely to be much importance \n(even though I personally err on the side of fixing any and all issues).\n\n\nOne thing I forgot to mention: in this specific case the leak was \ncausing a build failure when trying to build git's fuzzers within \noss-fuzz locally*. Specifically the following command would fail (see \nalso fuzz failure reproduction instructions which describe the setup [1]).\n\n  $ python infra/helper.py build_fuzzers --sanitizer address git\n\nAs far as I can tell the issue is that: a copy of git built with ASAN is \nused to produce the fuzzing corpus as part of the git-specific build \nscript [2] - the leak warning causes the script to fail. (It's possible \nto argue that the build script should disable ASAN's leak checking when \nrunning git, via detect_leaks=0 to reduce the risk of such breakage - I \nmay try to suggest such a change to oss-fuzz.)\n\nATB,\n   Andrzej\n\n\n* Given that oss-fuzz is building via docker, I would intuitively \nsuspect that the same issue occurs in automation - I'm not sure how to \nverify this myself.\n\n[1] \nhttps://google.github.io/oss-fuzz/advanced-topics/reproducing/#building-using-docker\n[2] \nhttps://github.com/google/oss-fuzz/blob/1b0115eefd70491376cf3cb6f88e49632c78ee18/projects/git/build.sh#L37\n"}]}