{"thread":{"id":"55465","subject":"[PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 1","startedAt":"2021-04-09T18:47:40Z","lastAt":"2021-04-28T00:43:44Z","messageCount":35,"participants":["Andrzej Hunt via GitGitGadget","René Scharfe","SZEDER Gábor","Junio C Hamano","Andrzej Hunt"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"421453","messageId":"pull.929.git.1617994052.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":null,"subject":"[PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 1","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:19Z","receivedAt":"2021-04-09T18:47:40Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"This series fixes approximately half of the real leaks I've found while\nrunning t0002 - t0099 under LSAN.\n\n2 more series will likely be needed to allow t0000-t0099 to pass with LSAN\nenabled. (I have all the necessary fixes ready and tested on my machine,\nalthough I want to revisit some of my changes before I'm happy enough to\nsend them out for review - either way I figure it's easiest to deal with one\nbatch at a time, so I'll hold off on sending those out for now. One series\nis going to consist almost entirely of UNLEAK annotations that are boring\nand not worth merging until the real leaks are fixed.)\n\nThe exciting news is that once we succeed in getting t000-t0099 to run leak\nfree, we'll be a significant step closer to being able to run the entire\ntest-suite leak-free:\n\n * before the merging of ah/plugleaks (fixing leaks in t0001): 53% of test\n cases fail when LSAN is enabled (12386/23330).\n * with ah/plugleaks + this series + my 2 currently unpublished series: 34%\n of test cases fail when LSAN is enabled (7829/23342).\n\n(I haven't bothered to test most of the intermediate stages, but ISTR that\nah/plugleaks which only had a marginal effect - somewhere on the order of\n51-52% test cases were failing after that work merged.)\n\nOn the topic of avoiding regressions: I've started running a subset of the\ntest-suite with LSAN enabled (in addition to the full test-suite with ASAN\nand UBSAN) on my Github fork of git, automatically on a daily basis. This\nshould hopefully help catch any new leaks that appear (and also new\nASAN/UBSAN issues). [The entire test-suite takes around 35 minutes with ASAN\nor UBSAN enabled, which isn't too bad compared to the default\nlinux-gcc/linux-clang jobs which take a similar amount of time - although\nthey run the test-suite twice with 2 configurations.]\n\nAndrzej Hunt (12):\n revision: free remainder of old commit list in limit_list\n wt-status: fix multiple small leaks\n ls-files: free max_prefix when done\n bloom: clear each bloom_key after use\n branch: FREE_AND_NULL instead of NULL'ing real_ref\n builtin/bugreport: don't leak prefixed filename\n builtin/check-ignore: clear_pathspec before returning\n builtin/checkout: clear pending objects after diffing\n mailinfo: also free strbuf lists when clearing mailinfo\n builtin/for-each-ref: free filter and UNLEAK sorting.\n builtin/rebase: release git_format_patch_opt too\n builtin/rm: avoid leaking pathspec and seen\n\n bloom.c | 1 +\n branch.c | 2 +-\n builtin/bugreport.c | 8 +++++---\n builtin/check-ignore.c | 1 +\n builtin/checkout.c | 1 +\n builtin/for-each-ref.c | 3 +++\n builtin/ls-files.c | 1 +\n builtin/rebase.c | 1 +\n builtin/rm.c | 2 ++\n mailinfo.c | 14 +++-----------\n revision.c | 1 +\n wt-status.c | 4 ++++\n 12 files changed, 24 insertions(+), 15 deletions(-)\n\n\nbase-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-929%2Fahunt%2Fleaksan-100-part1-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-929/ahunt/leaksan-100-part1-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/929\n-- \ngitgitgadget\n"},{"id":"421454","messageId":"12f0dcaef109e7577eabcc6f94f8ee72695b79aa.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 01/12] revision: free remainder of old commit list in limit_list","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:20Z","receivedAt":"2021-04-09T18:47:40Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nlimit_list() iterates over the original revs->commits list, and consumes\nmany of its entries via pop_commit. However we might stop iterating over\nthe list early (e.g. if we realise that the rest of the list is\nuninteresting). If we do stop iterating early, list will be pointing to\nthe unconsumed portion of revs->commits - and we need to free this list\nto avoid a leak. (revs->commits itself will be an invalid pointer: it\nwill have been free'd during the first pop_commit.)\n\nThis leak was found while running t0090. It's not likely to be very\nimpactful, but it can happen quite early during some checkout\ninvocations, and hence seems to be worth fixing:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n #2 0x9ac05a in xmalloc wrapper.c:62:9\n #3 0x7175d6 in commit_list_insert commit.c:540:33\n #4 0x71800f in commit_list_insert_by_date commit.c:604:9\n #5 0x8f8d2e in process_parents revision.c:1128:5\n #6 0x8f2f2c in limit_list revision.c:1418:7\n #7 0x8f210e in prepare_revision_walk revision.c:3577:7\n #8 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n #9 0x512f05 in switch_branches builtin/checkout.c:1250:3\n #10 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n #11 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n #12 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n #13 0x4cd91d in run_builtin git.c:467:11\n #14 0x4cb5f3 in handle_builtin git.c:719:3\n #15 0x4ccf47 in run_argv git.c:808:4\n #16 0x4caf49 in cmd_main git.c:939:19\n #17 0x69dc0e in main common-main.c:52:11\n #18 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 48 byte(s) in 3 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n #2 0x9ac05a in xmalloc wrapper.c:62:9\n #3 0x717de6 in commit_list_append commit.c:1609:35\n #4 0x8f1f9b in prepare_revision_walk revision.c:3554:12\n #5 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n #6 0x512f05 in switch_branches builtin/checkout.c:1250:3\n #7 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n #8 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n #9 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n revision.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/revision.c b/revision.c\nindex 553c0faa9b38..7b509aab0c87 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1460,6 +1460,7 @@ static int limit_list(struct rev_info *revs)\n \t\t\tupdate_treesame(revs, c);\n \t\t}\n \n+\tfree_commit_list(list);\n \trevs->commits = newlist;\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"421455","messageId":"beccdb1778697a2a46b81c85fc91c477c040397c.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 03/12] ls-files: free max_prefix when done","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:22Z","receivedAt":"2021-04-09T18:47:41Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\ncommon_prefix() returns a new string, which we store in max_prefix -\nthis string needs to be freed to avoid a leak. This leak is happening\nin cmd_ls_files, hence is of no real consequence - an UNLEAK would be\njust as good, but we might as well free the string properly.\n\nLeak found while running t0002, see output below:\n\nDirect leak of 8 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ab1b4 in do_xmalloc wrapper.c:41:8\n #2 0x9ab248 in do_xmallocz wrapper.c:75:8\n #3 0x9ab22a in xmallocz wrapper.c:83:9\n #4 0x9ab2d7 in xmemdupz wrapper.c:99:16\n #5 0x78d6a4 in common_prefix dir.c:191:15\n #6 0x5aca48 in cmd_ls_files builtin/ls-files.c:669:16\n #7 0x4cd92d in run_builtin git.c:453:11\n #8 0x4cb5fa in handle_builtin git.c:704:3\n #9 0x4ccf57 in run_argv git.c:771:4\n #10 0x4caf49 in cmd_main git.c:902:19\n #11 0x69ce2e in main common-main.c:52:11\n #12 0x7f64d4d94349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/ls-files.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 60a2913a01e9..53e20bbf9cce 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t}\n \n \tdir_clear(&dir);\n+\tfree((void *)max_prefix);\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"421456","messageId":"716a21b4ef73391cd7b242b4a63005777c13e1a7.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 02/12] wt-status: fix multiple small leaks","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:21Z","receivedAt":"2021-04-09T18:47:42Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrev.prune_data is populated (in multiple functions) via copy_pathspec,\nand therefore needs to be cleared after running the diff in those\nfunctions.\n\nrev(_info).pending is populated indirectly via setup_revisions, and also\nneeds to be cleared once diffing is done.\n\nThese leaks were found while running t0008 or t0021. The rev.prune_data\nleaks are small (80B) but noisy, hence I won't bother including their\nlogs - the rev.pending leaks are bigger, and can happen early in the\ncourse of other commands, and therefore possibly more valuable to fix -\nsee example log from a rebase below:\n\nDirect leak of 2048 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9ac2a6 in xrealloc wrapper.c:126:8\n #2 0x83da03 in add_object_array_with_path object.c:337:3\n #3 0x8f5d8a in add_pending_object_with_path revision.c:329:2\n #4 0x8ea50b in add_pending_object_with_mode revision.c:336:2\n #5 0x8ea4fd in add_pending_object revision.c:342:2\n #6 0x8ea610 in add_head_to_pending revision.c:354:2\n #7 0x9b55f5 in has_uncommitted_changes wt-status.c:2474:2\n #8 0x9b58c4 in require_clean_work_tree wt-status.c:2553:6\n #9 0x606bcc in cmd_rebase builtin/rebase.c:1970:6\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7f2d18909349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 5 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ac048 in xstrdup wrapper.c:29:14\n #2 0x83da8d in add_object_array_with_path object.c:349:17\n #3 0x8f5d8a in add_pending_object_with_path revision.c:329:2\n #4 0x8ea50b in add_pending_object_with_mode revision.c:336:2\n #5 0x8ea4fd in add_pending_object revision.c:342:2\n #6 0x8ea610 in add_head_to_pending revision.c:354:2\n #7 0x9b55f5 in has_uncommitted_changes wt-status.c:2474:2\n #8 0x9b58c4 in require_clean_work_tree wt-status.c:2553:6\n #9 0x606bcc in cmd_rebase builtin/rebase.c:1970:6\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7f2d18909349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 2053 byte(s) leaked in 2 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n wt-status.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 1aed68c43c26..34886655dbcc 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -616,6 +616,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_files(&rev, 0);\n+\tclear_pathspec(&rev.prune_data);\n }\n \n static void wt_status_collect_changes_index(struct wt_status *s)\n@@ -652,6 +653,8 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n+\tobject_array_clear(&rev.pending);\n+\tclear_pathspec(&rev.prune_data);\n }\n \n static void wt_status_collect_changes_initial(struct wt_status *s)\n@@ -2480,6 +2483,7 @@ int has_uncommitted_changes(struct repository *r,\n \n \tdiff_setup_done(&rev_info.diffopt);\n \tresult = run_diff_index(&rev_info, 1);\n+\tobject_array_clear(&rev_info.pending);\n \treturn diff_result_code(&rev_info.diffopt, result);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"421457","messageId":"8c7ba2b83d5d4dc6e8193955ef55282db38320e2.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 05/12] branch: FREE_AND_NULL instead of NULL'ing real_ref","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:24Z","receivedAt":"2021-04-09T18:47:46Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nreal_ref was previously populated by dwim_ref(), which allocates new\nmemory. We need to make sure to free real_ref when discarding it.\n(real_ref is already being freed at the end of create_branch() - but\nif we discard it early then it will leak.)\n\nThis fixes the following leak found while running t0002-t0099:\n\nDirect leak of 5 byte(s) in 1 object(s) allocated from:\n #0 0x486954 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xdd6484 in xstrdup wrapper.c:29:14\n #2 0xc0f658 in expand_ref refs.c:671:12\n #3 0xc0ecf1 in repo_dwim_ref refs.c:644:22\n #4 0x8b1184 in dwim_ref ./refs.h:162:9\n #5 0x8b0b02 in create_branch branch.c:284:10\n #6 0x550cbb in update_refs_for_switch builtin/checkout.c:1046:4\n #7 0x54e275 in switch_branches builtin/checkout.c:1274:2\n #8 0x548828 in checkout_branch builtin/checkout.c:1668:9\n #9 0x541306 in checkout_main builtin/checkout.c:2025:9\n #10 0x5395fa in cmd_checkout builtin/checkout.c:2077:8\n #11 0x4d02a8 in run_builtin git.c:467:11\n #12 0x4cbfe9 in handle_builtin git.c:719:3\n #13 0x4cf04f in run_argv git.c:808:4\n #14 0x4cb85a in cmd_main git.c:939:19\n #15 0x820cf6 in main common-main.c:52:11\n #16 0x7f30bd9dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex 9c9dae1eae32..514a3311d8d1 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -294,7 +294,7 @@ void create_branch(struct repository *r,\n \t\t\tif (explicit_tracking)\n \t\t\t\tdie(_(upstream_not_branch), start_name);\n \t\t\telse\n-\t\t\t\treal_ref = NULL;\n+\t\t\t\tFREE_AND_NULL(real_ref);\n \t\t}\n \t\tbreak;\n \tdefault:\n-- \ngitgitgadget\n\n"},{"id":"421458","messageId":"9ae15b94881369fa1cbd09fc2de9cc94c30edb2d.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 04/12] bloom: clear each bloom_key after use","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:23Z","receivedAt":"2021-04-09T18:47:48Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nfill_bloom_key() allocates memory into bloom_key, we need to clean that\nup once the key is no longer needed.\n\nThis fixes the following leak which was found while running t0002-t0099.\nAlthough this leak is happening in code being called from a test-helper,\nthe same code is also used in various locations around git, and could\npresumably happen during normal usage too.\n\nDirect leak of 308 byte(s) in 11 object(s) allocated from:\n #0 0x49a5e2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0x6f4032 in xcalloc wrapper.c:140:8\n #2 0x4f2905 in fill_bloom_key bloom.c:137:28\n #3 0x4f34c1 in get_or_compute_bloom_filter bloom.c:284:4\n #4 0x4cb484 in get_bloom_filter_for_commit t/helper/test-bloom.c:43:11\n #5 0x4cb072 in cmd__bloom t/helper/test-bloom.c:97:3\n #6 0x4ca7ef in cmd_main t/helper/test-tool.c:121:11\n #7 0x4caace in main common-main.c:52:11\n #8 0x7f798af95349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 308 byte(s) leaked in 11 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n bloom.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/bloom.c b/bloom.c\nindex 52b87474c6eb..5e297038bb1f 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -283,6 +283,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n \t\t\tstruct bloom_key key;\n \t\t\tfill_bloom_key(e->path, strlen(e->path), &key, settings);\n \t\t\tadd_key_to_filter(&key, filter, settings);\n+\t\t\tclear_bloom_key(&key);\n \t\t}\n \n \tcleanup:\n-- \ngitgitgadget\n\n"},{"id":"421459","messageId":"24129e3e633dbcf7c70f2962a330dc067e161712.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 06/12] builtin/bugreport: don't leak prefixed filename","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:25Z","receivedAt":"2021-04-09T18:47:49Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nprefix_filename() returns newly allocated memory, and strbuf_addstr()\ndoesn't take ownership of its inputs. Therefore we have to make sure to\nstore and free prefix_filename()'s result.\n\nAs this leak is in cmd_bugreport(), we could just as well UNLEAK the\nprefix - but there's no good reason not to just free it properly. This\nleak was found while running t0091, see output below:\n\nDirect leak of 24 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9acc66 in xrealloc wrapper.c:126:8\n #2 0x93baed in strbuf_grow strbuf.c:98:2\n #3 0x93c6ea in strbuf_add strbuf.c:295:2\n #4 0x69f162 in strbuf_addstr ./strbuf.h:304:2\n #5 0x69f083 in prefix_filename abspath.c:277:2\n #6 0x4fb275 in cmd_bugreport builtin/bugreport.c:146:9\n #7 0x4cd91d in run_builtin git.c:467:11\n #8 0x4cb5f3 in handle_builtin git.c:719:3\n #9 0x4ccf47 in run_argv git.c:808:4\n #10 0x4caf49 in cmd_main git.c:939:19\n #11 0x69df9e in main common-main.c:52:11\n #12 0x7f523a987349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/bugreport.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex ad3cc9c02f62..9915a5841def 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -129,6 +129,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tchar *option_output = NULL;\n \tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n \tconst char *user_relative_path = NULL;\n+\tchar *prefixed_filename;\n \n \tconst struct option bugreport_options[] = {\n \t\tOPT_STRING('o', \"output-directory\", &option_output, N_(\"path\"),\n@@ -142,9 +143,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\t\t bugreport_usage, 0);\n \n \t/* Prepare the path to put the result */\n-\tstrbuf_addstr(&report_path,\n-\t\t prefix_filename(prefix,\n-\t\t\t\t option_output ? option_output : \"\"));\n+\tprefixed_filename = prefix_filename(prefix,\n+\t\t\t\t\t option_output ? option_output : \"\");\n+\tstrbuf_addstr(&report_path, prefixed_filename);\n \tstrbuf_complete(&report_path, '/');\n \n \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n@@ -189,6 +190,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tfprintf(stderr, _(\"Created new report at '%s'.\\n\"),\n \t\tuser_relative_path);\n \n+\tfree(prefixed_filename);\n \tUNLEAK(buffer);\n \tUNLEAK(report_path);\n \treturn !!launch_editor(report_path.buf, NULL, NULL);\n-- \ngitgitgadget\n\n"},{"id":"421460","messageId":"563264af39c3103014fc2eb277ddd32ed78f10cf.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 07/12] builtin/check-ignore: clear_pathspec before returning","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:26Z","receivedAt":"2021-04-09T18:47:50Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nparse_pathspec() allocates new memory into pathspec, therefore we need\nto free it when we're done.\n\nAn UNLEAK would probably be just as good here - but clear_pathspec() is\nnot much more work so we might as well use it. check_ignore() is either\ncalled once directly from cmd_check_ignore() (in which case the leak\nreally doesnt matter), or it can be called multiple times in a loop from\ncheck_ignore_stdin_paths(), in which case we're potentially leaking\nmultiple times - but even in this scenario the leak is so small as to\nhave no real consequence.\n\nFound while running t0008:\n\nDirect leak of 112 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9aca44 in do_xmalloc wrapper.c:41:8\n #2 0x9aca1a in xmalloc wrapper.c:62:9\n #3 0x873c17 in parse_pathspec pathspec.c:582:2\n #4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2\n #5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17\n #6 0x4cd91d in run_builtin git.c:467:11\n #7 0x4cb5f3 in handle_builtin git.c:719:3\n #8 0x4ccf47 in run_argv git.c:808:4\n #9 0x4caf49 in cmd_main git.c:939:19\n #10 0x69e43e in main common-main.c:52:11\n #11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9acc46 in xrealloc wrapper.c:126:8\n #2 0x93baed in strbuf_grow strbuf.c:98:2\n #3 0x93d696 in strbuf_vaddf strbuf.c:392:3\n #4 0x9400c6 in xstrvfmt strbuf.c:979:2\n #5 0x940253 in xstrfmt strbuf.c:989:8\n #6 0x92b72a in prefix_path_gently setup.c:115:15\n #7 0x87442d in init_pathspec_item pathspec.c:439:11\n #8 0x873cef in parse_pathspec pathspec.c:589:3\n #9 0x503eb8 in check_ignore builtin/check-ignore.c:90:2\n #10 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17\n #11 0x4cd91d in run_builtin git.c:467:11\n #12 0x4cb5f3 in handle_builtin git.c:719:3\n #13 0x4ccf47 in run_argv git.c:808:4\n #14 0x4caf49 in cmd_main git.c:939:19\n #15 0x69e43e in main common-main.c:52:11\n #16 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 2 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ac9e8 in xstrdup wrapper.c:29:14\n #2 0x874542 in init_pathspec_item pathspec.c:468:20\n #3 0x873cef in parse_pathspec pathspec.c:589:3\n #4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2\n #5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17\n #6 0x4cd91d in run_builtin git.c:467:11\n #7 0x4cb5f3 in handle_builtin git.c:719:3\n #8 0x4ccf47 in run_argv git.c:808:4\n #9 0x4caf49 in cmd_main git.c:939:19\n #10 0x69e43e in main common-main.c:52:11\n #11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 179 byte(s) leaked in 3 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/check-ignore.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 3c652748d58c..467e92cc7b80 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -118,6 +118,7 @@ static int check_ignore(struct dir_struct *dir,\n \t\t\tnum_ignored++;\n \t}\n \tfree(seen);\n+\tclear_pathspec(&pathspec);\n \n \treturn num_ignored;\n }\n-- \ngitgitgadget\n\n"},{"id":"421461","messageId":"cdeb4b7875e3bf66bf2b492841118bab0e8f047e.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 08/12] builtin/checkout: clear pending objects after diffing","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:27Z","receivedAt":"2021-04-09T18:47:52Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nadd_pending_object() populates rev.pending, we need to take care of\nclearing it once we're done.\n\nThis code is run close to the end of a checkout, therefore this leak\nseems like it would have very little impact. See also LSAN output\nfrom t0020 below:\n\nDirect leak of 2048 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9acc46 in xrealloc wrapper.c:126:8\n #2 0x83e3a3 in add_object_array_with_path object.c:337:3\n #3 0x8f672a in add_pending_object_with_path revision.c:329:2\n #4 0x8eaeab in add_pending_object_with_mode revision.c:336:2\n #5 0x8eae9d in add_pending_object revision.c:342:2\n #6 0x5154a0 in show_local_changes builtin/checkout.c:602:2\n #7 0x513b00 in merge_working_tree builtin/checkout.c:979:3\n #8 0x512cb3 in switch_branches builtin/checkout.c:1242:9\n #9 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n #10 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n #11 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n #12 0x4cd91d in run_builtin git.c:467:11\n #13 0x4cb5f3 in handle_builtin git.c:719:3\n #14 0x4ccf47 in run_argv git.c:808:4\n #15 0x4caf49 in cmd_main git.c:939:19\n #16 0x69e43e in main common-main.c:52:11\n #17 0x7f5dd1d50349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 2048 byte(s) leaked in 1 allocation(s).\nSigned-off-by: Andrzej Hunt \n---\n builtin/checkout.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4c696ef4805b..190153c81571 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -602,6 +602,7 @@ static void show_local_changes(struct object *head,\n \tdiff_setup_done(&rev.diffopt);\n \tadd_pending_object(&rev, head, NULL);\n \trun_diff_index(&rev, 0);\n+\tobject_array_clear(&rev.pending);\n }\n \n static void describe_detached_head(const char *msg, struct commit *commit)\n-- \ngitgitgadget\n\n"},{"id":"421462","messageId":"130ef89218a47adc7ee558e75672e0e4eb5f30ca.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 09/12] mailinfo: also free strbuf lists when clearing mailinfo","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:28Z","receivedAt":"2021-04-09T18:47:53Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nmailinfo.p_hdr_info/s_hdr_info are null-terminated lists of strbuf's,\nwith entries pointing either to NULL or an allocated strbuf. Therefore\nwe need to free those strbuf's (and not just the data they contain)\nwhenever we're done with a given entry. (See handle_header() where those\nnew strbufs are malloc'd.)\n\nOnce we no longer need the list (and not just its entries) we can switch\nover to strbuf_list_free() instead of manually iterating over the list,\nwhich takes care of those additional details for us. We can only do this\nin clear_mailinfo() - in handle_commit_message() we are only clearing the\narray contents but want to reuse the array itself, hence we can't use\nstrbuf_list_free() there.\n\nLeak output from t0023:\n\nDirect leak of 72 byte(s) in 3 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac9f4 in do_xmalloc wrapper.c:41:8\n #2 0x9ac9ca in xmalloc wrapper.c:62:9\n #3 0x7f6cf7 in handle_header mailinfo.c:205:10\n #4 0x7f5abf in check_header mailinfo.c:583:4\n #5 0x7f5524 in mailinfo mailinfo.c:1197:3\n #6 0x4dcc95 in parse_mail builtin/am.c:1167:6\n #7 0x4d9070 in am_run builtin/am.c:1732:12\n #8 0x4d5b7a in cmd_am builtin/am.c:2398:3\n #9 0x4cd91d in run_builtin git.c:467:11\n #10 0x4cb5f3 in handle_builtin git.c:719:3\n #11 0x4ccf47 in run_argv git.c:808:4\n #12 0x4caf49 in cmd_main git.c:939:19\n #13 0x69e43e in main common-main.c:52:11\n #14 0x7fc1fadfa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 72 byte(s) leaked in 3 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n mailinfo.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 5681d9130db6..95ce191f385b 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -821,7 +821,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \t\tfor (i = 0; header[i]; i++) {\n \t\t\tif (mi->s_hdr_data[i])\n \t\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n-\t\t\tmi->s_hdr_data[i] = NULL;\n+\t\t\tFREE_AND_NULL(mi->s_hdr_data[i]);\n \t\t}\n \t\treturn 0;\n \t}\n@@ -1236,22 +1236,14 @@ void setup_mailinfo(struct mailinfo *mi)\n \n void clear_mailinfo(struct mailinfo *mi)\n {\n-\tint i;\n-\n \tstrbuf_release(&mi->name);\n \tstrbuf_release(&mi->email);\n \tstrbuf_release(&mi->charset);\n \tstrbuf_release(&mi->inbody_header_accum);\n \tfree(mi->message_id);\n \n-\tif (mi->p_hdr_data)\n-\t\tfor (i = 0; mi->p_hdr_data[i]; i++)\n-\t\t\tstrbuf_release(mi->p_hdr_data[i]);\n-\tfree(mi->p_hdr_data);\n-\tif (mi->s_hdr_data)\n-\t\tfor (i = 0; mi->s_hdr_data[i]; i++)\n-\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n-\tfree(mi->s_hdr_data);\n+\tstrbuf_list_free(mi->p_hdr_data);\n+\tstrbuf_list_free(mi->s_hdr_data);\n \n \twhile (mi->content < mi->content_top) {\n \t\tfree(*(mi->content_top));\n-- \ngitgitgadget\n\n"},{"id":"421463","messageId":"8f2374ee899da808ce42f55a5aacc5fe54ad38ed.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 10/12] builtin/for-each-ref: free filter and UNLEAK sorting.","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:29Z","receivedAt":"2021-04-09T18:47:55Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nsorting might be a list allocated in ref_default_sorting() (in this case\nit's a fixed single item list, which has nevertheless been xcalloc'd),\nor it might be a list allocated in parse_opt_ref_sorting(). In either\ncase we could free these lists - but instead we UNLEAK as we're at the\nend of cmd_for_each_ref. (There's no existing implementation of\nclear_ref_sorting(), and writing a loop to free the list seems more\ntrouble than it's worth.)\n\nfilter.with_commit/no_commit are populated via\nOPT_CONTAINS/OPT_NO_CONTAINS, both of which create new entries via\nparse_opt_commits(), and also need to be free'd or UNLEAK'd. Because\nfree_commit_list() already exists, we choose to use that over an UNLEAK.\n\nLSAN output from t0041:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0x9ac252 in xcalloc wrapper.c:140:8\n #2 0x8a4a55 in ref_default_sorting ref-filter.c:2486:32\n #3 0x56c6b1 in cmd_for_each_ref builtin/for-each-ref.c:72:13\n #4 0x4cd91d in run_builtin git.c:467:11\n #5 0x4cb5f3 in handle_builtin git.c:719:3\n #6 0x4ccf47 in run_argv git.c:808:4\n #7 0x4caf49 in cmd_main git.c:939:19\n #8 0x69dabe in main common-main.c:52:11\n #9 0x7f2bdc570349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9abf54 in do_xmalloc wrapper.c:41:8\n #2 0x9abf2a in xmalloc wrapper.c:62:9\n #3 0x717486 in commit_list_insert commit.c:540:33\n #4 0x8644cf in parse_opt_commits parse-options-cb.c:98:2\n #5 0x869bb5 in get_value parse-options.c:181:11\n #6 0x8677dc in parse_long_opt parse-options.c:378:10\n #7 0x8659bd in parse_options_step parse-options.c:817:11\n #8 0x867fcd in parse_options parse-options.c:870:10\n #9 0x56c62b in cmd_for_each_ref builtin/for-each-ref.c:59:2\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dabe in main common-main.c:52:11\n #15 0x7f2bdc570349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/for-each-ref.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex cb9c81a04606..84efb71f82fc 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -83,5 +83,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < maxcount; i++)\n \t\tshow_ref_array_item(array.items[i], &format);\n \tref_array_clear(&array);\n+\tfree_commit_list(filter.with_commit);\n+\tfree_commit_list(filter.no_commit);\n+\tUNLEAK(sorting);\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"421464","messageId":"c17e296bcb1476050306a29065ca3599767ff2b4.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 11/12] builtin/rebase: release git_format_patch_opt too","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:30Z","receivedAt":"2021-04-09T18:47:55Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\noptions.git_format_patch_opt can be populated during cmd_rebase's setup,\nand will therefore leak on return. Although we could just UNLEAK all of\noptions, we choose to strbuf_release() the individual member, which matches\nthe existing pattern (where we're freeing invidual members of options).\n\nLeak found when running t0021:\n\nDirect leak of 24 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9ac296 in xrealloc wrapper.c:126:8\n #2 0x93b13d in strbuf_grow strbuf.c:98:2\n #3 0x93bd3a in strbuf_add strbuf.c:295:2\n #4 0x60ae92 in strbuf_addstr strbuf.h:304:2\n #5 0x605f17 in cmd_rebase builtin/rebase.c:1759:3\n #6 0x4cd91d in run_builtin git.c:467:11\n #7 0x4cb5f3 in handle_builtin git.c:719:3\n #8 0x4ccf47 in run_argv git.c:808:4\n #9 0x4caf49 in cmd_main git.c:939:19\n #10 0x69dbfe in main common-main.c:52:11\n #11 0x7f66dae91349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/rebase.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 783b526f6e75..c0cd2c40c7d8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -2108,6 +2108,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.head_name);\n \tfree(options.gpg_sign_opt);\n \tfree(options.cmd);\n+\tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \treturn ret;\n }\n-- \ngitgitgadget\n\n"},{"id":"421465","messageId":"db1b151e2a151a0ba1ee1f8ead47480c1236113d.1617994052.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH 12/12] builtin/rm: avoid leaking pathspec and seen","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T18:47:31Z","receivedAt":"2021-04-09T18:47:56Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nparse_pathspec() populates pathspec, hence we need to clear it once it's\nno longer needed. seen is xcalloc'd within the same function and\nlikewise needs to be freed once its no longer needed.\n\ncmd_rm() has multiple early returns, therefore we need to clear or free\nas soon as this data is no longer needed, as opposed to doing a cleanup\nat the end.\n\nLSAN output from t0020:\n\nDirect leak of 112 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac0a4 in do_xmalloc wrapper.c:41:8\n #2 0x9ac07a in xmalloc wrapper.c:62:9\n #3 0x873277 in parse_pathspec pathspec.c:582:2\n #4 0x646ffa in cmd_rm builtin/rm.c:266:2\n #5 0x4cd91d in run_builtin git.c:467:11\n #6 0x4cb5f3 in handle_builtin git.c:719:3\n #7 0x4ccf47 in run_argv git.c:808:4\n #8 0x4caf49 in cmd_main git.c:939:19\n #9 0x69dc0e in main common-main.c:52:11\n #10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9ac2a6 in xrealloc wrapper.c:126:8\n #2 0x93b14d in strbuf_grow strbuf.c:98:2\n #3 0x93ccf6 in strbuf_vaddf strbuf.c:392:3\n #4 0x93f726 in xstrvfmt strbuf.c:979:2\n #5 0x93f8b3 in xstrfmt strbuf.c:989:8\n #6 0x92ad8a in prefix_path_gently setup.c:115:15\n #7 0x873a8d in init_pathspec_item pathspec.c:439:11\n #8 0x87334f in parse_pathspec pathspec.c:589:3\n #9 0x646ffa in cmd_rm builtin/rm.c:266:2\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 15 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ac048 in xstrdup wrapper.c:29:14\n #2 0x873ba2 in init_pathspec_item pathspec.c:468:20\n #3 0x87334f in parse_pathspec pathspec.c:589:3\n #4 0x646ffa in cmd_rm builtin/rm.c:266:2\n #5 0x4cd91d in run_builtin git.c:467:11\n #6 0x4cb5f3 in handle_builtin git.c:719:3\n #7 0x4ccf47 in run_argv git.c:808:4\n #8 0x4caf49 in cmd_main git.c:939:19\n #9 0x69dc0e in main common-main.c:52:11\n #10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 1 byte(s) in 1 object(s) allocated from:\n #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0x9ac392 in xcalloc wrapper.c:140:8\n #2 0x647108 in cmd_rm builtin/rm.c:294:9\n #3 0x4cd91d in run_builtin git.c:467:11\n #4 0x4cb5f3 in handle_builtin git.c:719:3\n #5 0x4ccf47 in run_argv git.c:808:4\n #6 0x4caf49 in cmd_main git.c:939:19\n #7 0x69dbfe in main common-main.c:52:11\n #8 0x7f4fac1b0349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/rm.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4858631e0f02..2927678d37b6 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -327,6 +327,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\tif (!seen_any)\n \t\t\texit(0);\n \t}\n+\tclear_pathspec(&pathspec);\n+\tfree(seen);\n \n \tif (!index_only)\n \t\tsubmodules_absorb_gitdir_if_needed();\n-- \ngitgitgadget\n"},{"id":"421519","messageId":"797e5ce8-14e2-0689-cf19-4426c1c8bd5d@web.de","threadId":"55465","inReplyTo":"12f0dcaef109e7577eabcc6f94f8ee72695b79aa.1617994052.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/12] revision: free remainder of old commit list in limit_list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-04-10T07:29:28Z","receivedAt":"2021-04-10T07:29:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget:\n> From: Andrzej Hunt \n>\n> limit_list() iterates over the original revs->commits list, and consumes\n> many of its entries via pop_commit. However we might stop iterating over\n> the list early (e.g. if we realise that the rest of the list is\n> uninteresting). If we do stop iterating early, list will be pointing to\n> the unconsumed portion of revs->commits - and we need to free this list\n> to avoid a leak. (revs->commits itself will be an invalid pointer: it\n> will have been free'd during the first pop_commit.)\n>\n> This leak was found while running t0090. It's not likely to be very\n> impactful, but it can happen quite early during some checkout\n> invocations, and hence seems to be worth fixing:\n>\n> Direct leak of 16 byte(s) in 1 object(s) allocated from:\n> #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n> #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n> #2 0x9ac05a in xmalloc wrapper.c:62:9\n> #3 0x7175d6 in commit_list_insert commit.c:540:33\n> #4 0x71800f in commit_list_insert_by_date commit.c:604:9\n> #5 0x8f8d2e in process_parents revision.c:1128:5\n> #6 0x8f2f2c in limit_list revision.c:1418:7\n> #7 0x8f210e in prepare_revision_walk revision.c:3577:7\n> #8 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n> #9 0x512f05 in switch_branches builtin/checkout.c:1250:3\n> #10 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n> #11 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n> #12 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n> #13 0x4cd91d in run_builtin git.c:467:11\n> #14 0x4cb5f3 in handle_builtin git.c:719:3\n> #15 0x4ccf47 in run_argv git.c:808:4\n> #16 0x4caf49 in cmd_main git.c:939:19\n> #17 0x69dc0e in main common-main.c:52:11\n> #18 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Indirect leak of 48 byte(s) in 3 object(s) allocated from:\n> #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n> #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n> #2 0x9ac05a in xmalloc wrapper.c:62:9\n> #3 0x717de6 in commit_list_append commit.c:1609:35\n> #4 0x8f1f9b in prepare_revision_walk revision.c:3554:12\n> #5 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n> #6 0x512f05 in switch_branches builtin/checkout.c:1250:3\n> #7 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n> #8 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n> #9 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n> #10 0x4cd91d in run_builtin git.c:467:11\n> #11 0x4cb5f3 in handle_builtin git.c:719:3\n> #12 0x4ccf47 in run_argv git.c:808:4\n> #13 0x4caf49 in cmd_main git.c:939:19\n> #14 0x69dc0e in main common-main.c:52:11\n> #15 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> revision.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 553c0faa9b38..7b509aab0c87 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1460,6 +1460,7 @@ static int limit_list(struct rev_info *revs)\n> \t\t\tupdate_treesame(revs, c);\n> \t\t}\n>\n> +\tfree_commit_list(list);\n\nThis patch would benefit from more context, but this function is quite\nlong. So let me sketch it:\n\n\tstruct commit_list *list = revs->commits;\n\n\twhile (list) {\n\t\tstruct commit *commit = pop_commit(&list);\n\t\tstruct object *obj = &commit->object;\n\n\t\tif (obj->flags & UNINTERESTING) {\n\t\t\tbreak;\n\t\t}\n\t}\n\n if (limiting_can_increase_treesame(revs))\n for (list = newlist; list; list = list->next) {\n\t\t}\n\n\tfree_commit_list(list);\n\nSo the while loop can leave list dangling and you want to free its\nremaining entries. The for loop sometimes overwrites the list pointer,\nthough, and you will end up passing NULL to free_commit_list in that\ncase. So either the call should be moved between the loops or a fresh\nvariable should be used in the second loop instead of reusing list to\nmake sure the entries are released in all cases.\n\n> \trevs->commits = newlist;\n> \treturn 0;\n> }\n>\n\n"},{"id":"421520","messageId":"6a72a920-134f-541b-7caa-debe24658005@web.de","threadId":"55465","inReplyTo":"beccdb1778697a2a46b81c85fc91c477c040397c.1617994052.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 03/12] ls-files: free max_prefix when done","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-04-10T08:12:00Z","receivedAt":"2021-04-10T08:12:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget:\n> From: Andrzej Hunt \n>\n> common_prefix() returns a new string, which we store in max_prefix -\n> this string needs to be freed to avoid a leak. This leak is happening\n> in cmd_ls_files, hence is of no real consequence - an UNLEAK would be\n> just as good, but we might as well free the string properly.\n>\n> Leak found while running t0002, see output below:\n>\n> Direct leak of 8 byte(s) in 1 object(s) allocated from:\n> #0 0x49a85d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n> #1 0x9ab1b4 in do_xmalloc wrapper.c:41:8\n> #2 0x9ab248 in do_xmallocz wrapper.c:75:8\n> #3 0x9ab22a in xmallocz wrapper.c:83:9\n> #4 0x9ab2d7 in xmemdupz wrapper.c:99:16\n> #5 0x78d6a4 in common_prefix dir.c:191:15\n> #6 0x5aca48 in cmd_ls_files builtin/ls-files.c:669:16\n> #7 0x4cd92d in run_builtin git.c:453:11\n> #8 0x4cb5fa in handle_builtin git.c:704:3\n> #9 0x4ccf57 in run_argv git.c:771:4\n> #10 0x4caf49 in cmd_main git.c:902:19\n> #11 0x69ce2e in main common-main.c:52:11\n> #12 0x7f64d4d94349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> builtin/ls-files.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 60a2913a01e9..53e20bbf9cce 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n> \t}\n>\n> \tdir_clear(&dir);\n> +\tfree((void *)max_prefix);\n\nThis cast is necessary to ignore the const attribute of the pointer.\nIt's scary, but safe here because this function owns the referenced\nobject.\n\nI think the promise to not modify the string given at the top of the\nfunction is not worth having to take back that promise forcefully at\nthe end to dispose of it. Determining the correctness of this cast\nrequires reading the whole function. Removing the const from the\ndeclaration (and the cast) would improve readability overall. Thoughts?\n\n> \treturn 0;\n> }\n>\n\n"},{"id":"421570","messageId":"20210411072651.GF2947267@szeder.dev","threadId":"55465","inReplyTo":"9ae15b94881369fa1cbd09fc2de9cc94c30edb2d.1617994052.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 04/12] bloom: clear each bloom_key after use","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-11T07:26:51Z","receivedAt":"2021-04-11T07:26:57Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 09, 2021 at 06:47:23PM +0000, Andrzej Hunt via GitGitGadget wrote:\n> From: Andrzej Hunt \n> \n> fill_bloom_key() allocates memory into bloom_key, we need to clean that\n> up once the key is no longer needed.\n> \n> This fixes the following leak which was found while running t0002-t0099.\n> Although this leak is happening in code being called from a test-helper,\n> the same code is also used in various locations around git, and could\n> presumably happen during normal usage too.\n\nIt does indeed happen: 'git commit-graph write --reachable\n--changed-paths' generates Bloom filters for every commit, with each\nfilter containing all paths modified by its associated commit, so it\nleaks a lot of 7 * 4byte hashes. This patch reduces the memory usage\nof that command:\n\n Max RSS\n before after\n ---------------------------------------------\n android-base 1275028k 1006576k -21.1%\n chromium 3245144k 3127764k -3.6%\n cmssw 793996k 699156k -12.0%\n cpython 371584k 343480k -7.6%\n elasticsearch 748104k 637936k -14.7%\n freebsd-src 819020k 741272k -9.5%\n gcc 867412k 730332k -15.8%\n gecko-dev 2619112k 2457280k -6.2%\n git 252684k 216900k -14.2%\n glibc 239000k 222228k -7.0%\n go 264132k 251344k -4.9%\n homebrew-cask 542188k 480588k -11.4%\n homebrew-core 805332k 715848k -11.1%\n jdk 417832k 342928k -17.9%\n libreoff-core 1257296k 1089980k -13.3%\n linux 2033296k 1759712k -13.5%\n llvm-project 1067216k 956704k -10.4%\n mariadb-srv 695172k 559508k -19.5%\n postgres 340132k 317416k -6.7%\n rails 325432k 294332k -9.6%\n rust 655244k 584904k -10.7%\n tensorflow 507308k 480848k -5.2%\n webkit 2466812k 2237332k -9.3%\n\nJust out of curiosity, I disabled the questionable hardcoded 512 paths\nlimit on the size of modified path Bloom filters, and the memory usage\nin the jdk repository sunk by over 55%, from 849520k to 379760k.\n\nPlease feel free to include any of the above data points in the commit\nmessage.\n\n> Direct leak of 308 byte(s) in 11 object(s) allocated from:\n> #0 0x49a5e2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n> #1 0x6f4032 in xcalloc wrapper.c:140:8\n> #2 0x4f2905 in fill_bloom_key bloom.c:137:28\n> #3 0x4f34c1 in get_or_compute_bloom_filter bloom.c:284:4\n> #4 0x4cb484 in get_bloom_filter_for_commit t/helper/test-bloom.c:43:11\n> #5 0x4cb072 in cmd__bloom t/helper/test-bloom.c:97:3\n> #6 0x4ca7ef in cmd_main t/helper/test-tool.c:121:11\n> #7 0x4caace in main common-main.c:52:11\n> #8 0x7f798af95349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n> \n> SUMMARY: AddressSanitizer: 308 byte(s) leaked in 11 allocation(s).\n> \n> Signed-off-by: Andrzej Hunt \n> ---\n> bloom.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/bloom.c b/bloom.c\n> index 52b87474c6eb..5e297038bb1f 100644\n> --- a/bloom.c\n> +++ b/bloom.c\n> @@ -283,6 +283,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n> \t\t\tstruct bloom_key key;\n> \t\t\tfill_bloom_key(e->path, strlen(e->path), &key, settings);\n> \t\t\tadd_key_to_filter(&key, filter, settings);\n> +\t\t\tclear_bloom_key(&key);\n> \t\t}\n> \n> \tcleanup:\n> -- \n> gitgitgadget\n> \n"},{"id":"421587","messageId":"xmqq4kgdhws6.fsf@gitster.g","threadId":"55465","inReplyTo":"130ef89218a47adc7ee558e75672e0e4eb5f30ca.1617994052.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 09/12] mailinfo: also free strbuf lists when clearing mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-11T11:43:05Z","receivedAt":"2021-04-11T11:43:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrzej Hunt via GitGitGadget\" writes:\n\n> void clear_mailinfo(struct mailinfo *mi)\n> {\n> -\tint i;\n> -\n> \tstrbuf_release(&mi->name);\n> \tstrbuf_release(&mi->email);\n> \tstrbuf_release(&mi->charset);\n> \tstrbuf_release(&mi->inbody_header_accum);\n> \tfree(mi->message_id);\n> \n> -\tif (mi->p_hdr_data)\n> -\t\tfor (i = 0; mi->p_hdr_data[i]; i++)\n> -\t\t\tstrbuf_release(mi->p_hdr_data[i]);\n> -\tfree(mi->p_hdr_data);\n> -\tif (mi->s_hdr_data)\n> -\t\tfor (i = 0; mi->s_hdr_data[i]; i++)\n> -\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n> -\tfree(mi->s_hdr_data);\n\nSo, the original allows mi->p_hdr_data to be NULL and does not do\nthis freeing (the same for the .s_hdr_data member).\n\n> +\tstrbuf_list_free(mi->p_hdr_data);\n> +\tstrbuf_list_free(mi->s_hdr_data);\n\nIs it safe to feed NULL to the helper?\n\n void strbuf_list_free(struct strbuf **sbs)\n {\n struct strbuf **s = sbs;\n\n while (*s) {\n strbuf_release(*s);\n free(*s++);\n }\n free(sbs);\n }\n\n\n"},{"id":"422877","messageId":"437e7381-7f93-b314-d9c6-d0f0e26ea683@ahunt.org","threadId":"55465","inReplyTo":"xmqq4kgdhws6.fsf@gitster.g","subject":"Re: [PATCH 09/12] mailinfo: also free strbuf lists when clearing mailinfo","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-04-25T13:15:59Z","receivedAt":"2021-04-25T13:16:10Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 11/04/2021 13:43, Junio C Hamano wrote:\n> \"Andrzej Hunt via GitGitGadget\" writes:\n> \n>> void clear_mailinfo(struct mailinfo *mi)\n>> {\n>> -\tint i;\n>> -\n>> \tstrbuf_release(&mi->name);\n>> \tstrbuf_release(&mi->email);\n>> \tstrbuf_release(&mi->charset);\n>> \tstrbuf_release(&mi->inbody_header_accum);\n>> \tfree(mi->message_id);\n>> \n>> -\tif (mi->p_hdr_data)\n>> -\t\tfor (i = 0; mi->p_hdr_data[i]; i++)\n>> -\t\t\tstrbuf_release(mi->p_hdr_data[i]);\n>> -\tfree(mi->p_hdr_data);\n>> -\tif (mi->s_hdr_data)\n>> -\t\tfor (i = 0; mi->s_hdr_data[i]; i++)\n>> -\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n>> -\tfree(mi->s_hdr_data);\n> \n> So, the original allows mi->p_hdr_data to be NULL and does not do\n> this freeing (the same for the .s_hdr_data member).\n> \n>> +\tstrbuf_list_free(mi->p_hdr_data);\n>> +\tstrbuf_list_free(mi->s_hdr_data);\n> \n> Is it safe to feed NULL to the helper?\n> \n> void strbuf_list_free(struct strbuf **sbs)\n> {\n> struct strbuf **s = sbs;\n> \n> while (*s) {\n> strbuf_release(*s);\n> free(*s++);\n> }\n> free(sbs);\n> }\n> \n> \n\nIndeed: AFAIUI dereferencing NULL is undefined \tbehaviour. I think the \nbest solution is to add a NULL check in strbuf_list_free() - which is \nthe pattern I've seen in several other *_free() helpers (there are also \nquite a few examples of *_free() helpers that are not NULL safe, but \nIMHO having a NULL check will lead to fewer unpleasant surprises).\n\nIncidentally I did run the entire test-suite against UBSAN, and it \ndidn't find any issues here. This seems like something that UBSAN should \nbe able to easily catch, so we probably don't have any tests exercising \nclear_mailinfo() with NULL p_hdr_info/s_hdr_info?\n"},{"id":"422878","messageId":"11ae3ce9-997b-32fc-7bc3-ee95a3d99153@ahunt.org","threadId":"55465","inReplyTo":"6a72a920-134f-541b-7caa-debe24658005@web.de","subject":"Re: [PATCH 03/12] ls-files: free max_prefix when done","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-04-25T13:16:34Z","receivedAt":"2021-04-25T13:16:45Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 10/04/2021 10:12, René Scharfe wrote:\n> Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget:\n>> From: Andrzej Hunt \n>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>> index 60a2913a01e9..53e20bbf9cce 100644\n>> --- a/builtin/ls-files.c\n>> +++ b/builtin/ls-files.c\n>> @@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n>> \t}\n>>\n>> \tdir_clear(&dir);\n>> +\tfree((void *)max_prefix);\n> \n> This cast is necessary to ignore the const attribute of the pointer.\n> It's scary, but safe here because this function owns the referenced\n> object.\n> \n> I think the promise to not modify the string given at the top of the\n> function is not worth having to take back that promise forcefully at\n> the end to dispose of it. Determining the correctness of this cast\n> requires reading the whole function. Removing the const from the\n> declaration (and the cast) would improve readability overall. Thoughts?\n\nI agree - I'll change this in V2 V2. In fact, Peff already given the \nfollowing explanation for why non-const is preferred in this scenario on \na previous patch of mine (which I failed to heed when preparing this patch):\n\n > If a variable is meant to take ownership of memory, our usual\n > convention is to not declare it as \"const\".\"\nhttps://lore.kernel.org/git/YEZ0jLppB9wOg%2Faf@coredump.intra.peff.net/\n\n> \n>> \treturn 0;\n>> }\n>>\n> \n"},{"id":"422879","messageId":"a641ca69-05c8-a2c0-59a8-93711eb3d349@ahunt.org","threadId":"55465","inReplyTo":"20210411072651.GF2947267@szeder.dev","subject":"Re: [PATCH 04/12] bloom: clear each bloom_key after use","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-04-25T13:17:38Z","receivedAt":"2021-04-25T13:17:45Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 11/04/2021 09:26, SZEDER Gábor wrote:\n> On Fri, Apr 09, 2021 at 06:47:23PM +0000, Andrzej Hunt via GitGitGadget wrote:\n>> From: Andrzej Hunt \n>>\n>> fill_bloom_key() allocates memory into bloom_key, we need to clean that\n>> up once the key is no longer needed.\n>>\n>> This fixes the following leak which was found while running t0002-t0099.\n>> Although this leak is happening in code being called from a test-helper,\n>> the same code is also used in various locations around git, and could\n>> presumably happen during normal usage too.\n> \n> It does indeed happen: 'git commit-graph write --reachable\n> --changed-paths' generates Bloom filters for every commit, with each\n> filter containing all paths modified by its associated commit, so it\n> leaks a lot of 7 * 4byte hashes. This patch reduces the memory usage\n> of that command:\n> \n> Max RSS\n> before after\n> ---------------------------------------------\n> android-base 1275028k 1006576k -21.1%\n> chromium 3245144k 3127764k -3.6%\n> cmssw 793996k 699156k -12.0%\n> cpython 371584k 343480k -7.6%\n> elasticsearch 748104k 637936k -14.7%\n> freebsd-src 819020k 741272k -9.5%\n> gcc 867412k 730332k -15.8%\n> gecko-dev 2619112k 2457280k -6.2%\n> git 252684k 216900k -14.2%\n> glibc 239000k 222228k -7.0%\n> go 264132k 251344k -4.9%\n> homebrew-cask 542188k 480588k -11.4%\n> homebrew-core 805332k 715848k -11.1%\n> jdk 417832k 342928k -17.9%\n> libreoff-core 1257296k 1089980k -13.3%\n> linux 2033296k 1759712k -13.5%\n> llvm-project 1067216k 956704k -10.4%\n> mariadb-srv 695172k 559508k -19.5%\n> postgres 340132k 317416k -6.7%\n> rails 325432k 294332k -9.6%\n> rust 655244k 584904k -10.7%\n> tensorflow 507308k 480848k -5.2%\n> webkit 2466812k 2237332k -9.3%\n> \n> Just out of curiosity, I disabled the questionable hardcoded 512 paths\n> limit on the size of modified path Bloom filters, and the memory usage\n> in the jdk repository sunk by over 55%, from 849520k to 379760k.\n> \n> Please feel free to include any of the above data points in the commit\n> message.\n\nThank you for the detailed analysis - these kinds of results are very \nmotivating! I will include a brief summary (something like \"10% typical \nimprovement for 'commit-graph write' for large repos\") along with a link \nto your posting for those who want the full picture.\n"},{"id":"422880","messageId":"c883a4b0-668c-3d43-b1d6-183ccd61133f@ahunt.org","threadId":"55465","inReplyTo":"797e5ce8-14e2-0689-cf19-4426c1c8bd5d@web.de","subject":"Re: [PATCH 01/12] revision: free remainder of old commit list in limit_list","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-04-25T13:32:53Z","receivedAt":"2021-04-25T13:33:03Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 10/04/2021 09:29, René Scharfe wrote:\n> Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget:\n>> From: Andrzej Hunt \n>>\n>> limit_list() iterates over the original revs->commits list, and consumes\n>> many of its entries via pop_commit. However we might stop iterating over\n>> the list early (e.g. if we realise that the rest of the list is\n>> uninteresting). If we do stop iterating early, list will be pointing to\n>> the unconsumed portion of revs->commits - and we need to free this list\n>> to avoid a leak. (revs->commits itself will be an invalid pointer: it\n>> will have been free'd during the first pop_commit.)\n>>\n>> This leak was found while running t0090. It's not likely to be very\n>> impactful, but it can happen quite early during some checkout\n>> invocations, and hence seems to be worth fixing:\n>>\n>> Direct leak of 16 byte(s) in 1 object(s) allocated from:\n>> #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n>> #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n>> #2 0x9ac05a in xmalloc wrapper.c:62:9\n>> #3 0x7175d6 in commit_list_insert commit.c:540:33\n>> #4 0x71800f in commit_list_insert_by_date commit.c:604:9\n>> #5 0x8f8d2e in process_parents revision.c:1128:5\n>> #6 0x8f2f2c in limit_list revision.c:1418:7\n>> #7 0x8f210e in prepare_revision_walk revision.c:3577:7\n>> #8 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n>> #9 0x512f05 in switch_branches builtin/checkout.c:1250:3\n>> #10 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n>> #11 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n>> #12 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n>> #13 0x4cd91d in run_builtin git.c:467:11\n>> #14 0x4cb5f3 in handle_builtin git.c:719:3\n>> #15 0x4ccf47 in run_argv git.c:808:4\n>> #16 0x4caf49 in cmd_main git.c:939:19\n>> #17 0x69dc0e in main common-main.c:52:11\n>> #18 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>>\n>> Indirect leak of 48 byte(s) in 3 object(s) allocated from:\n>> #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n>> #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n>> #2 0x9ac05a in xmalloc wrapper.c:62:9\n>> #3 0x717de6 in commit_list_append commit.c:1609:35\n>> #4 0x8f1f9b in prepare_revision_walk revision.c:3554:12\n>> #5 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n>> #6 0x512f05 in switch_branches builtin/checkout.c:1250:3\n>> #7 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n>> #8 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n>> #9 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n>> #10 0x4cd91d in run_builtin git.c:467:11\n>> #11 0x4cb5f3 in handle_builtin git.c:719:3\n>> #12 0x4ccf47 in run_argv git.c:808:4\n>> #13 0x4caf49 in cmd_main git.c:939:19\n>> #14 0x69dc0e in main common-main.c:52:11\n>> #15 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>>\n>> Signed-off-by: Andrzej Hunt \n>> ---\n>> revision.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/revision.c b/revision.c\n>> index 553c0faa9b38..7b509aab0c87 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -1460,6 +1460,7 @@ static int limit_list(struct rev_info *revs)\n>> \t\t\tupdate_treesame(revs, c);\n>> \t\t}\n>>\n>> +\tfree_commit_list(list);\n> \n> This patch would benefit from more context, but this function is quite\n> long. So let me sketch it:\n> \n> \tstruct commit_list *list = revs->commits;\n> \n> \twhile (list) {\n> \t\tstruct commit *commit = pop_commit(&list);\n> \t\tstruct object *obj = &commit->object;\n> \n> \t\tif (obj->flags & UNINTERESTING) {\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> \n> if (limiting_can_increase_treesame(revs))\n> for (list = newlist; list; list = list->next) {\n> \t\t}\n> \n> \tfree_commit_list(list);\n> \n> So the while loop can leave list dangling and you want to free its\n> remaining entries. The for loop sometimes overwrites the list pointer,\n> though, and you will end up passing NULL to free_commit_list in that\n> case. So either the call should be moved between the loops or a fresh\n> variable should be used in the second loop instead of reusing list to\n> make sure the entries are released in all cases.\n\nGood catch, I did not look closely enough at this one - V1 definitely is \nbuggy*. I've decided I'll add a new variable for the list in V2, but I \nalso took the opportunity to rename the original list since I think that \nmakes it more obvious where that list came from in the first place.\n\n* However I also didn't run into any failures when running the entire \ntest-suite with this change, so I'm guessing this codepath isn't being \nexercised by our tests. I'm hoping to try and investigate this in more \ndetail when I find a spare moment.\n"},{"id":"422881","messageId":"d97307edca4202a8755b0f4a33f0d7434e3afa7f.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 01/12] revision: free remainder of old commit list in limit_list","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:08Z","receivedAt":"2021-04-25T14:16:26Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nlimit_list() iterates over the original revs->commits list, and consumes\nmany of its entries via pop_commit. However we might stop iterating over\nthe list early (e.g. if we realise that the rest of the list is\nuninteresting). If we do stop iterating early, list will be pointing to\nthe unconsumed portion of revs->commits - and we need to free this list\nto avoid a leak. (revs->commits itself will be an invalid pointer: it\nwill have been free'd during the first pop_commit.)\n\nHowever the list pointer is later reused to iterate over our new list,\nbut only for the limiting_can_increase_treesame() branch. We therefore\nneed to introduce a new variable for that branch - and while we're here\nwe can rename the original list to original_list as that makes its\npurpose more obvious.\n\nThis leak was found while running t0090. It's not likely to be very\nimpactful, but it can happen quite early during some checkout\ninvocations, and hence seems to be worth fixing:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n #2 0x9ac05a in xmalloc wrapper.c:62:9\n #3 0x7175d6 in commit_list_insert commit.c:540:33\n #4 0x71800f in commit_list_insert_by_date commit.c:604:9\n #5 0x8f8d2e in process_parents revision.c:1128:5\n #6 0x8f2f2c in limit_list revision.c:1418:7\n #7 0x8f210e in prepare_revision_walk revision.c:3577:7\n #8 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n #9 0x512f05 in switch_branches builtin/checkout.c:1250:3\n #10 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n #11 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n #12 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n #13 0x4cd91d in run_builtin git.c:467:11\n #14 0x4cb5f3 in handle_builtin git.c:719:3\n #15 0x4ccf47 in run_argv git.c:808:4\n #16 0x4caf49 in cmd_main git.c:939:19\n #17 0x69dc0e in main common-main.c:52:11\n #18 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 48 byte(s) in 3 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac084 in do_xmalloc wrapper.c:41:8\n #2 0x9ac05a in xmalloc wrapper.c:62:9\n #3 0x717de6 in commit_list_append commit.c:1609:35\n #4 0x8f1f9b in prepare_revision_walk revision.c:3554:12\n #5 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6\n #6 0x512f05 in switch_branches builtin/checkout.c:1250:3\n #7 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n #8 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n #9 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n revision.c | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 553c0faa9b38..568d73c0b331 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1393,20 +1393,20 @@ static int limit_list(struct rev_info *revs)\n {\n \tint slop = SLOP;\n \ttimestamp_t date = TIME_MAX;\n-\tstruct commit_list *list = revs->commits;\n+\tstruct commit_list *original_list = revs->commits;\n \tstruct commit_list *newlist = NULL;\n \tstruct commit_list **p = &newlist;\n \tstruct commit_list *bottom = NULL;\n \tstruct commit *interesting_cache = NULL;\n \n \tif (revs->ancestry_path) {\n-\t\tbottom = collect_bottom_commits(list);\n+\t\tbottom = collect_bottom_commits(original_list);\n \t\tif (!bottom)\n \t\t\tdie(\"--ancestry-path given but there are no bottom commits\");\n \t}\n \n-\twhile (list) {\n-\t\tstruct commit *commit = pop_commit(&list);\n+\twhile (original_list) {\n+\t\tstruct commit *commit = pop_commit(&original_list);\n \t\tstruct object *obj = &commit->object;\n \t\tshow_early_output_fn_t show;\n \n@@ -1415,11 +1415,11 @@ static int limit_list(struct rev_info *revs)\n \n \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n \t\t\tobj->flags |= UNINTERESTING;\n-\t\tif (process_parents(revs, commit, &list, NULL) < 0)\n+\t\tif (process_parents(revs, commit, &original_list, NULL) < 0)\n \t\t\treturn -1;\n \t\tif (obj->flags & UNINTERESTING) {\n \t\t\tmark_parents_uninteresting(commit);\n-\t\t\tslop = still_interesting(list, date, slop, &interesting_cache);\n+\t\t\tslop = still_interesting(original_list, date, slop, &interesting_cache);\n \t\t\tif (slop)\n \t\t\t\tcontinue;\n \t\t\tbreak;\n@@ -1452,14 +1452,17 @@ static int limit_list(struct rev_info *revs)\n \t * Check if any commits have become TREESAME by some of their parents\n \t * becoming UNINTERESTING.\n \t */\n-\tif (limiting_can_increase_treesame(revs))\n+\tif (limiting_can_increase_treesame(revs)) {\n+\t\tstruct commit_list *list = NULL;\n \t\tfor (list = newlist; list; list = list->next) {\n \t\t\tstruct commit *c = list->item;\n \t\t\tif (c->object.flags & (UNINTERESTING | TREESAME))\n \t\t\t\tcontinue;\n \t\t\tupdate_treesame(revs, c);\n \t\t}\n+\t}\n \n+\tfree_commit_list(original_list);\n \trevs->commits = newlist;\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"422882","messageId":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.git.1617994052.gitgitgadget@gmail.com","subject":"[PATCH v2 00/12] Fix all leaks in tests t0002-t0099: Part 1","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:07Z","receivedAt":"2021-04-25T14:16:26Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"V2 addresses all the issues brought up during review, and adds a link to\nGabor's analysis of the bloom filter changes.\n\nAndrzej Hunt (12):\n revision: free remainder of old commit list in limit_list\n wt-status: fix multiple small leaks\n ls-files: free max_prefix when done\n bloom: clear each bloom_key after use\n branch: FREE_AND_NULL instead of NULL'ing real_ref\n builtin/bugreport: don't leak prefixed filename\n builtin/check-ignore: clear_pathspec before returning\n builtin/checkout: clear pending objects after diffing\n mailinfo: also free strbuf lists when clearing mailinfo\n builtin/for-each-ref: free filter and UNLEAK sorting.\n builtin/rebase: release git_format_patch_opt too\n builtin/rm: avoid leaking pathspec and seen\n\n bloom.c | 1 +\n branch.c | 2 +-\n builtin/bugreport.c | 8 +++++---\n builtin/check-ignore.c | 1 +\n builtin/checkout.c | 1 +\n builtin/for-each-ref.c | 3 +++\n builtin/ls-files.c | 3 ++-\n builtin/rebase.c | 1 +\n builtin/rm.c | 2 ++\n mailinfo.c | 14 +++-----------\n revision.c | 17 ++++++++++-------\n strbuf.c | 2 ++\n wt-status.c | 4 ++++\n 13 files changed, 36 insertions(+), 23 deletions(-)\n\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-929%2Fahunt%2Fleaksan-100-part1-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-929/ahunt/leaksan-100-part1-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/929\n\nRange-diff vs v1:\n\n 1: 12f0dcaef109 ! 1: d97307edca42 revision: free remainder of old commit list in limit_list\n @@ Commit message\n to avoid a leak. (revs->commits itself will be an invalid pointer: it\n will have been free'd during the first pop_commit.)\n \n + However the list pointer is later reused to iterate over our new list,\n + but only for the limiting_can_increase_treesame() branch. We therefore\n + need to introduce a new variable for that branch - and while we're here\n + we can rename the original list to original_list as that makes its\n + purpose more obvious.\n +\n This leak was found while running t0090. It's not likely to be very\n impactful, but it can happen quite early during some checkout\n invocations, and hence seems to be worth fixing:\n @@ Commit message\n \n ## revision.c ##\n @@ revision.c: static int limit_list(struct rev_info *revs)\n + {\n + \tint slop = SLOP;\n + \ttimestamp_t date = TIME_MAX;\n +-\tstruct commit_list *list = revs->commits;\n ++\tstruct commit_list *original_list = revs->commits;\n + \tstruct commit_list *newlist = NULL;\n + \tstruct commit_list **p = &newlist;\n + \tstruct commit_list *bottom = NULL;\n + \tstruct commit *interesting_cache = NULL;\n + \n + \tif (revs->ancestry_path) {\n +-\t\tbottom = collect_bottom_commits(list);\n ++\t\tbottom = collect_bottom_commits(original_list);\n + \t\tif (!bottom)\n + \t\t\tdie(\"--ancestry-path given but there are no bottom commits\");\n + \t}\n + \n +-\twhile (list) {\n +-\t\tstruct commit *commit = pop_commit(&list);\n ++\twhile (original_list) {\n ++\t\tstruct commit *commit = pop_commit(&original_list);\n + \t\tstruct object *obj = &commit->object;\n + \t\tshow_early_output_fn_t show;\n + \n +@@ revision.c: static int limit_list(struct rev_info *revs)\n + \n + \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n + \t\t\tobj->flags |= UNINTERESTING;\n +-\t\tif (process_parents(revs, commit, &list, NULL) < 0)\n ++\t\tif (process_parents(revs, commit, &original_list, NULL) < 0)\n + \t\t\treturn -1;\n + \t\tif (obj->flags & UNINTERESTING) {\n + \t\t\tmark_parents_uninteresting(commit);\n +-\t\t\tslop = still_interesting(list, date, slop, &interesting_cache);\n ++\t\t\tslop = still_interesting(original_list, date, slop, &interesting_cache);\n + \t\t\tif (slop)\n + \t\t\t\tcontinue;\n + \t\t\tbreak;\n +@@ revision.c: static int limit_list(struct rev_info *revs)\n + \t * Check if any commits have become TREESAME by some of their parents\n + \t * becoming UNINTERESTING.\n + \t */\n +-\tif (limiting_can_increase_treesame(revs))\n ++\tif (limiting_can_increase_treesame(revs)) {\n ++\t\tstruct commit_list *list = NULL;\n + \t\tfor (list = newlist; list; list = list->next) {\n + \t\t\tstruct commit *c = list->item;\n + \t\t\tif (c->object.flags & (UNINTERESTING | TREESAME))\n + \t\t\t\tcontinue;\n \t\t\tupdate_treesame(revs, c);\n \t\t}\n ++\t}\n \n -+\tfree_commit_list(list);\n ++\tfree_commit_list(original_list);\n \trevs->commits = newlist;\n \treturn 0;\n }\n 2: 716a21b4ef73 = 2: 9ad3d8e3fbf4 wt-status: fix multiple small leaks\n 3: beccdb177869 ! 3: 76519acdfee7 ls-files: free max_prefix when done\n @@ Commit message\n Signed-off-by: Andrzej Hunt \n \n ## builtin/ls-files.c ##\n +@@ builtin/ls-files.c: static int option_parse_exclude_standard(const struct option *opt,\n + int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n + {\n + \tint require_work_tree = 0, show_tag = 0, i;\n +-\tconst char *max_prefix;\n ++\tchar *max_prefix;\n + \tstruct dir_struct dir;\n + \tstruct pattern_list *pl;\n + \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n @@ builtin/ls-files.c: int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t}\n \n \tdir_clear(&dir);\n -+\tfree((void *)max_prefix);\n ++\tfree(max_prefix);\n \treturn 0;\n }\n 4: 9ae15b948813 ! 4: fb64a3dcd0b0 bloom: clear each bloom_key after use\n @@ Commit message\n fill_bloom_key() allocates memory into bloom_key, we need to clean that\n up once the key is no longer needed.\n \n - This fixes the following leak which was found while running t0002-t0099.\n - Although this leak is happening in code being called from a test-helper,\n - the same code is also used in various locations around git, and could\n - presumably happen during normal usage too.\n + This leak was found while running t0002-t0099. Although this leak is\n + happening in code being called from a test-helper, the same code is also\n + used in various locations around git, and can therefore happen during\n + normal usage too. Gabor's analysis shows that peak-memory usage during\n + 'git commit-graph write' is reduced on the order of 10% for a selection\n + of larger repos (along with an even larger reduction if we override\n + modified path bloom filter limits):\n + https://lore.kernel.org/git/20210411072651.GF2947267@szeder.dev/\n +\n + LSAN output:\n \n Direct leak of 308 byte(s) in 11 object(s) allocated from:\n #0 0x49a5e2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n 5: 8c7ba2b83d5d = 5: 154c6714f305 branch: FREE_AND_NULL instead of NULL'ing real_ref\n 6: 24129e3e633d = 6: 0ae6224e01bc builtin/bugreport: don't leak prefixed filename\n 7: 563264af39c3 = 7: 693ea82490df builtin/check-ignore: clear_pathspec before returning\n 8: cdeb4b7875e3 = 8: 20c5f2e68c54 builtin/checkout: clear pending objects after diffing\n 9: 130ef89218a4 ! 9: 217f571f8ef5 mailinfo: also free strbuf lists when clearing mailinfo\n @@ Commit message\n array contents but want to reuse the array itself, hence we can't use\n strbuf_list_free() there.\n \n + However, strbuf_list_free() cannot handle a NULL input, and the lists we\n + are freeing might be NULL. Therefore we add a NULL check in\n + strbuf_list_free() to make it safe to use with a NULL input (which is a\n + pattern used by some of the other *_free() functions around git).\n +\n Leak output from t0023:\n \n Direct leak of 72 byte(s) in 3 object(s) allocated from:\n @@ mailinfo.c: void setup_mailinfo(struct mailinfo *mi)\n \n \twhile (mi->content < mi->content_top) {\n \t\tfree(*(mi->content_top));\n +\n + ## strbuf.c ##\n +@@ strbuf.c: void strbuf_list_free(struct strbuf **sbs)\n + {\n + \tstruct strbuf **s = sbs;\n + \n ++\tif (!s)\n ++\t\treturn;\n + \twhile (*s) {\n + \t\tstrbuf_release(*s);\n + \t\tfree(*s++);\n 10: 8f2374ee899d = 10: c4363c212217 builtin/for-each-ref: free filter and UNLEAK sorting.\n 11: c17e296bcb14 = 11: a67168677477 builtin/rebase: release git_format_patch_opt too\n 12: db1b151e2a15 = 12: 703cd9656bf8 builtin/rm: avoid leaking pathspec and seen\n\n-- \ngitgitgadget\n"},{"id":"422883","messageId":"9ad3d8e3fbf4ebb0622f3b68b13ae34908ac5b87.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 02/12] wt-status: fix multiple small leaks","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:09Z","receivedAt":"2021-04-25T14:16:26Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrev.prune_data is populated (in multiple functions) via copy_pathspec,\nand therefore needs to be cleared after running the diff in those\nfunctions.\n\nrev(_info).pending is populated indirectly via setup_revisions, and also\nneeds to be cleared once diffing is done.\n\nThese leaks were found while running t0008 or t0021. The rev.prune_data\nleaks are small (80B) but noisy, hence I won't bother including their\nlogs - the rev.pending leaks are bigger, and can happen early in the\ncourse of other commands, and therefore possibly more valuable to fix -\nsee example log from a rebase below:\n\nDirect leak of 2048 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9ac2a6 in xrealloc wrapper.c:126:8\n #2 0x83da03 in add_object_array_with_path object.c:337:3\n #3 0x8f5d8a in add_pending_object_with_path revision.c:329:2\n #4 0x8ea50b in add_pending_object_with_mode revision.c:336:2\n #5 0x8ea4fd in add_pending_object revision.c:342:2\n #6 0x8ea610 in add_head_to_pending revision.c:354:2\n #7 0x9b55f5 in has_uncommitted_changes wt-status.c:2474:2\n #8 0x9b58c4 in require_clean_work_tree wt-status.c:2553:6\n #9 0x606bcc in cmd_rebase builtin/rebase.c:1970:6\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7f2d18909349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 5 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ac048 in xstrdup wrapper.c:29:14\n #2 0x83da8d in add_object_array_with_path object.c:349:17\n #3 0x8f5d8a in add_pending_object_with_path revision.c:329:2\n #4 0x8ea50b in add_pending_object_with_mode revision.c:336:2\n #5 0x8ea4fd in add_pending_object revision.c:342:2\n #6 0x8ea610 in add_head_to_pending revision.c:354:2\n #7 0x9b55f5 in has_uncommitted_changes wt-status.c:2474:2\n #8 0x9b58c4 in require_clean_work_tree wt-status.c:2553:6\n #9 0x606bcc in cmd_rebase builtin/rebase.c:1970:6\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7f2d18909349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 2053 byte(s) leaked in 2 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n wt-status.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 1aed68c43c26..34886655dbcc 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -616,6 +616,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_files(&rev, 0);\n+\tclear_pathspec(&rev.prune_data);\n }\n \n static void wt_status_collect_changes_index(struct wt_status *s)\n@@ -652,6 +653,8 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n+\tobject_array_clear(&rev.pending);\n+\tclear_pathspec(&rev.prune_data);\n }\n \n static void wt_status_collect_changes_initial(struct wt_status *s)\n@@ -2480,6 +2483,7 @@ int has_uncommitted_changes(struct repository *r,\n \n \tdiff_setup_done(&rev_info.diffopt);\n \tresult = run_diff_index(&rev_info, 1);\n+\tobject_array_clear(&rev_info.pending);\n \treturn diff_result_code(&rev_info.diffopt, result);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"422884","messageId":"76519acdfee7b7f45dc1a8b52b2083fb4fa03b3f.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 03/12] ls-files: free max_prefix when done","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:10Z","receivedAt":"2021-04-25T14:16:28Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\ncommon_prefix() returns a new string, which we store in max_prefix -\nthis string needs to be freed to avoid a leak. This leak is happening\nin cmd_ls_files, hence is of no real consequence - an UNLEAK would be\njust as good, but we might as well free the string properly.\n\nLeak found while running t0002, see output below:\n\nDirect leak of 8 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ab1b4 in do_xmalloc wrapper.c:41:8\n #2 0x9ab248 in do_xmallocz wrapper.c:75:8\n #3 0x9ab22a in xmallocz wrapper.c:83:9\n #4 0x9ab2d7 in xmemdupz wrapper.c:99:16\n #5 0x78d6a4 in common_prefix dir.c:191:15\n #6 0x5aca48 in cmd_ls_files builtin/ls-files.c:669:16\n #7 0x4cd92d in run_builtin git.c:453:11\n #8 0x4cb5fa in handle_builtin git.c:704:3\n #9 0x4ccf57 in run_argv git.c:771:4\n #10 0x4caf49 in cmd_main git.c:902:19\n #11 0x69ce2e in main common-main.c:52:11\n #12 0x7f64d4d94349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/ls-files.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 60a2913a01e9..84448b360120 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -603,7 +603,7 @@ static int option_parse_exclude_standard(const struct option *opt,\n int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n {\n \tint require_work_tree = 0, show_tag = 0, i;\n-\tconst char *max_prefix;\n+\tchar *max_prefix;\n \tstruct dir_struct dir;\n \tstruct pattern_list *pl;\n \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n@@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t}\n \n \tdir_clear(&dir);\n+\tfree(max_prefix);\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"422885","messageId":"154c6714f30596db84711b5cd639c62ace5b721b.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 05/12] branch: FREE_AND_NULL instead of NULL'ing real_ref","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:12Z","receivedAt":"2021-04-25T14:16:30Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nreal_ref was previously populated by dwim_ref(), which allocates new\nmemory. We need to make sure to free real_ref when discarding it.\n(real_ref is already being freed at the end of create_branch() - but\nif we discard it early then it will leak.)\n\nThis fixes the following leak found while running t0002-t0099:\n\nDirect leak of 5 byte(s) in 1 object(s) allocated from:\n #0 0x486954 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xdd6484 in xstrdup wrapper.c:29:14\n #2 0xc0f658 in expand_ref refs.c:671:12\n #3 0xc0ecf1 in repo_dwim_ref refs.c:644:22\n #4 0x8b1184 in dwim_ref ./refs.h:162:9\n #5 0x8b0b02 in create_branch branch.c:284:10\n #6 0x550cbb in update_refs_for_switch builtin/checkout.c:1046:4\n #7 0x54e275 in switch_branches builtin/checkout.c:1274:2\n #8 0x548828 in checkout_branch builtin/checkout.c:1668:9\n #9 0x541306 in checkout_main builtin/checkout.c:2025:9\n #10 0x5395fa in cmd_checkout builtin/checkout.c:2077:8\n #11 0x4d02a8 in run_builtin git.c:467:11\n #12 0x4cbfe9 in handle_builtin git.c:719:3\n #13 0x4cf04f in run_argv git.c:808:4\n #14 0x4cb85a in cmd_main git.c:939:19\n #15 0x820cf6 in main common-main.c:52:11\n #16 0x7f30bd9dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex b71a2de29dbe..2260325d58c0 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -294,7 +294,7 @@ void create_branch(struct repository *r,\n \t\t\tif (explicit_tracking)\n \t\t\t\tdie(_(upstream_not_branch), start_name);\n \t\t\telse\n-\t\t\t\treal_ref = NULL;\n+\t\t\t\tFREE_AND_NULL(real_ref);\n \t\t}\n \t\tbreak;\n \tdefault:\n-- \ngitgitgadget\n\n"},{"id":"422886","messageId":"fb64a3dcd0b077b1b818a285ada066a135f911a6.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 04/12] bloom: clear each bloom_key after use","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:11Z","receivedAt":"2021-04-25T14:16:30Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nfill_bloom_key() allocates memory into bloom_key, we need to clean that\nup once the key is no longer needed.\n\nThis leak was found while running t0002-t0099. Although this leak is\nhappening in code being called from a test-helper, the same code is also\nused in various locations around git, and can therefore happen during\nnormal usage too. Gabor's analysis shows that peak-memory usage during\n'git commit-graph write' is reduced on the order of 10% for a selection\nof larger repos (along with an even larger reduction if we override\nmodified path bloom filter limits):\nhttps://lore.kernel.org/git/20210411072651.GF2947267@szeder.dev/\n\nLSAN output:\n\nDirect leak of 308 byte(s) in 11 object(s) allocated from:\n #0 0x49a5e2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0x6f4032 in xcalloc wrapper.c:140:8\n #2 0x4f2905 in fill_bloom_key bloom.c:137:28\n #3 0x4f34c1 in get_or_compute_bloom_filter bloom.c:284:4\n #4 0x4cb484 in get_bloom_filter_for_commit t/helper/test-bloom.c:43:11\n #5 0x4cb072 in cmd__bloom t/helper/test-bloom.c:97:3\n #6 0x4ca7ef in cmd_main t/helper/test-tool.c:121:11\n #7 0x4caace in main common-main.c:52:11\n #8 0x7f798af95349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 308 byte(s) leaked in 11 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n bloom.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/bloom.c b/bloom.c\nindex 52b87474c6eb..5e297038bb1f 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -283,6 +283,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n \t\t\tstruct bloom_key key;\n \t\t\tfill_bloom_key(e->path, strlen(e->path), &key, settings);\n \t\t\tadd_key_to_filter(&key, filter, settings);\n+\t\t\tclear_bloom_key(&key);\n \t\t}\n \n \tcleanup:\n-- \ngitgitgadget\n\n"},{"id":"422887","messageId":"0ae6224e01bc5d7da47b844600e64e44d7805fdb.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 06/12] builtin/bugreport: don't leak prefixed filename","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:13Z","receivedAt":"2021-04-25T14:16:30Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nprefix_filename() returns newly allocated memory, and strbuf_addstr()\ndoesn't take ownership of its inputs. Therefore we have to make sure to\nstore and free prefix_filename()'s result.\n\nAs this leak is in cmd_bugreport(), we could just as well UNLEAK the\nprefix - but there's no good reason not to just free it properly. This\nleak was found while running t0091, see output below:\n\nDirect leak of 24 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9acc66 in xrealloc wrapper.c:126:8\n #2 0x93baed in strbuf_grow strbuf.c:98:2\n #3 0x93c6ea in strbuf_add strbuf.c:295:2\n #4 0x69f162 in strbuf_addstr ./strbuf.h:304:2\n #5 0x69f083 in prefix_filename abspath.c:277:2\n #6 0x4fb275 in cmd_bugreport builtin/bugreport.c:146:9\n #7 0x4cd91d in run_builtin git.c:467:11\n #8 0x4cb5f3 in handle_builtin git.c:719:3\n #9 0x4ccf47 in run_argv git.c:808:4\n #10 0x4caf49 in cmd_main git.c:939:19\n #11 0x69df9e in main common-main.c:52:11\n #12 0x7f523a987349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/bugreport.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex ad3cc9c02f62..9915a5841def 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -129,6 +129,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tchar *option_output = NULL;\n \tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n \tconst char *user_relative_path = NULL;\n+\tchar *prefixed_filename;\n \n \tconst struct option bugreport_options[] = {\n \t\tOPT_STRING('o', \"output-directory\", &option_output, N_(\"path\"),\n@@ -142,9 +143,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\t\t bugreport_usage, 0);\n \n \t/* Prepare the path to put the result */\n-\tstrbuf_addstr(&report_path,\n-\t\t prefix_filename(prefix,\n-\t\t\t\t option_output ? option_output : \"\"));\n+\tprefixed_filename = prefix_filename(prefix,\n+\t\t\t\t\t option_output ? option_output : \"\");\n+\tstrbuf_addstr(&report_path, prefixed_filename);\n \tstrbuf_complete(&report_path, '/');\n \n \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n@@ -189,6 +190,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tfprintf(stderr, _(\"Created new report at '%s'.\\n\"),\n \t\tuser_relative_path);\n \n+\tfree(prefixed_filename);\n \tUNLEAK(buffer);\n \tUNLEAK(report_path);\n \treturn !!launch_editor(report_path.buf, NULL, NULL);\n-- \ngitgitgadget\n\n"},{"id":"422888","messageId":"693ea82490df68a013582a1f3e4aa8920bfa0cee.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 07/12] builtin/check-ignore: clear_pathspec before returning","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:14Z","receivedAt":"2021-04-25T14:16:32Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nparse_pathspec() allocates new memory into pathspec, therefore we need\nto free it when we're done.\n\nAn UNLEAK would probably be just as good here - but clear_pathspec() is\nnot much more work so we might as well use it. check_ignore() is either\ncalled once directly from cmd_check_ignore() (in which case the leak\nreally doesnt matter), or it can be called multiple times in a loop from\ncheck_ignore_stdin_paths(), in which case we're potentially leaking\nmultiple times - but even in this scenario the leak is so small as to\nhave no real consequence.\n\nFound while running t0008:\n\nDirect leak of 112 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9aca44 in do_xmalloc wrapper.c:41:8\n #2 0x9aca1a in xmalloc wrapper.c:62:9\n #3 0x873c17 in parse_pathspec pathspec.c:582:2\n #4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2\n #5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17\n #6 0x4cd91d in run_builtin git.c:467:11\n #7 0x4cb5f3 in handle_builtin git.c:719:3\n #8 0x4ccf47 in run_argv git.c:808:4\n #9 0x4caf49 in cmd_main git.c:939:19\n #10 0x69e43e in main common-main.c:52:11\n #11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9acc46 in xrealloc wrapper.c:126:8\n #2 0x93baed in strbuf_grow strbuf.c:98:2\n #3 0x93d696 in strbuf_vaddf strbuf.c:392:3\n #4 0x9400c6 in xstrvfmt strbuf.c:979:2\n #5 0x940253 in xstrfmt strbuf.c:989:8\n #6 0x92b72a in prefix_path_gently setup.c:115:15\n #7 0x87442d in init_pathspec_item pathspec.c:439:11\n #8 0x873cef in parse_pathspec pathspec.c:589:3\n #9 0x503eb8 in check_ignore builtin/check-ignore.c:90:2\n #10 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17\n #11 0x4cd91d in run_builtin git.c:467:11\n #12 0x4cb5f3 in handle_builtin git.c:719:3\n #13 0x4ccf47 in run_argv git.c:808:4\n #14 0x4caf49 in cmd_main git.c:939:19\n #15 0x69e43e in main common-main.c:52:11\n #16 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 2 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ac9e8 in xstrdup wrapper.c:29:14\n #2 0x874542 in init_pathspec_item pathspec.c:468:20\n #3 0x873cef in parse_pathspec pathspec.c:589:3\n #4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2\n #5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17\n #6 0x4cd91d in run_builtin git.c:467:11\n #7 0x4cb5f3 in handle_builtin git.c:719:3\n #8 0x4ccf47 in run_argv git.c:808:4\n #9 0x4caf49 in cmd_main git.c:939:19\n #10 0x69e43e in main common-main.c:52:11\n #11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 179 byte(s) leaked in 3 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/check-ignore.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 3c652748d58c..467e92cc7b80 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -118,6 +118,7 @@ static int check_ignore(struct dir_struct *dir,\n \t\t\tnum_ignored++;\n \t}\n \tfree(seen);\n+\tclear_pathspec(&pathspec);\n \n \treturn num_ignored;\n }\n-- \ngitgitgadget\n\n"},{"id":"422889","messageId":"217f571f8ef5f3a46c0cbb1ceca022a18e5b43d2.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 09/12] mailinfo: also free strbuf lists when clearing mailinfo","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:16Z","receivedAt":"2021-04-25T14:16:32Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nmailinfo.p_hdr_info/s_hdr_info are null-terminated lists of strbuf's,\nwith entries pointing either to NULL or an allocated strbuf. Therefore\nwe need to free those strbuf's (and not just the data they contain)\nwhenever we're done with a given entry. (See handle_header() where those\nnew strbufs are malloc'd.)\n\nOnce we no longer need the list (and not just its entries) we can switch\nover to strbuf_list_free() instead of manually iterating over the list,\nwhich takes care of those additional details for us. We can only do this\nin clear_mailinfo() - in handle_commit_message() we are only clearing the\narray contents but want to reuse the array itself, hence we can't use\nstrbuf_list_free() there.\n\nHowever, strbuf_list_free() cannot handle a NULL input, and the lists we\nare freeing might be NULL. Therefore we add a NULL check in\nstrbuf_list_free() to make it safe to use with a NULL input (which is a\npattern used by some of the other *_free() functions around git).\n\nLeak output from t0023:\n\nDirect leak of 72 byte(s) in 3 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac9f4 in do_xmalloc wrapper.c:41:8\n #2 0x9ac9ca in xmalloc wrapper.c:62:9\n #3 0x7f6cf7 in handle_header mailinfo.c:205:10\n #4 0x7f5abf in check_header mailinfo.c:583:4\n #5 0x7f5524 in mailinfo mailinfo.c:1197:3\n #6 0x4dcc95 in parse_mail builtin/am.c:1167:6\n #7 0x4d9070 in am_run builtin/am.c:1732:12\n #8 0x4d5b7a in cmd_am builtin/am.c:2398:3\n #9 0x4cd91d in run_builtin git.c:467:11\n #10 0x4cb5f3 in handle_builtin git.c:719:3\n #11 0x4ccf47 in run_argv git.c:808:4\n #12 0x4caf49 in cmd_main git.c:939:19\n #13 0x69e43e in main common-main.c:52:11\n #14 0x7fc1fadfa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 72 byte(s) leaked in 3 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n mailinfo.c | 14 +++-----------\n strbuf.c | 2 ++\n 2 files changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 5681d9130db6..95ce191f385b 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -821,7 +821,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \t\tfor (i = 0; header[i]; i++) {\n \t\t\tif (mi->s_hdr_data[i])\n \t\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n-\t\t\tmi->s_hdr_data[i] = NULL;\n+\t\t\tFREE_AND_NULL(mi->s_hdr_data[i]);\n \t\t}\n \t\treturn 0;\n \t}\n@@ -1236,22 +1236,14 @@ void setup_mailinfo(struct mailinfo *mi)\n \n void clear_mailinfo(struct mailinfo *mi)\n {\n-\tint i;\n-\n \tstrbuf_release(&mi->name);\n \tstrbuf_release(&mi->email);\n \tstrbuf_release(&mi->charset);\n \tstrbuf_release(&mi->inbody_header_accum);\n \tfree(mi->message_id);\n \n-\tif (mi->p_hdr_data)\n-\t\tfor (i = 0; mi->p_hdr_data[i]; i++)\n-\t\t\tstrbuf_release(mi->p_hdr_data[i]);\n-\tfree(mi->p_hdr_data);\n-\tif (mi->s_hdr_data)\n-\t\tfor (i = 0; mi->s_hdr_data[i]; i++)\n-\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n-\tfree(mi->s_hdr_data);\n+\tstrbuf_list_free(mi->p_hdr_data);\n+\tstrbuf_list_free(mi->s_hdr_data);\n \n \twhile (mi->content < mi->content_top) {\n \t\tfree(*(mi->content_top));\ndiff --git a/strbuf.c b/strbuf.c\nindex e3397cc4c72a..4df30b45494d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -209,6 +209,8 @@ void strbuf_list_free(struct strbuf **sbs)\n {\n \tstruct strbuf **s = sbs;\n \n+\tif (!s)\n+\t\treturn;\n \twhile (*s) {\n \t\tstrbuf_release(*s);\n \t\tfree(*s++);\n-- \ngitgitgadget\n\n"},{"id":"422890","messageId":"c4363c2122170b61ca49cf4150c8408dde05f96b.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 10/12] builtin/for-each-ref: free filter and UNLEAK sorting.","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:17Z","receivedAt":"2021-04-25T14:16:33Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nsorting might be a list allocated in ref_default_sorting() (in this case\nit's a fixed single item list, which has nevertheless been xcalloc'd),\nor it might be a list allocated in parse_opt_ref_sorting(). In either\ncase we could free these lists - but instead we UNLEAK as we're at the\nend of cmd_for_each_ref. (There's no existing implementation of\nclear_ref_sorting(), and writing a loop to free the list seems more\ntrouble than it's worth.)\n\nfilter.with_commit/no_commit are populated via\nOPT_CONTAINS/OPT_NO_CONTAINS, both of which create new entries via\nparse_opt_commits(), and also need to be free'd or UNLEAK'd. Because\nfree_commit_list() already exists, we choose to use that over an UNLEAK.\n\nLSAN output from t0041:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0x9ac252 in xcalloc wrapper.c:140:8\n #2 0x8a4a55 in ref_default_sorting ref-filter.c:2486:32\n #3 0x56c6b1 in cmd_for_each_ref builtin/for-each-ref.c:72:13\n #4 0x4cd91d in run_builtin git.c:467:11\n #5 0x4cb5f3 in handle_builtin git.c:719:3\n #6 0x4ccf47 in run_argv git.c:808:4\n #7 0x4caf49 in cmd_main git.c:939:19\n #8 0x69dabe in main common-main.c:52:11\n #9 0x7f2bdc570349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9abf54 in do_xmalloc wrapper.c:41:8\n #2 0x9abf2a in xmalloc wrapper.c:62:9\n #3 0x717486 in commit_list_insert commit.c:540:33\n #4 0x8644cf in parse_opt_commits parse-options-cb.c:98:2\n #5 0x869bb5 in get_value parse-options.c:181:11\n #6 0x8677dc in parse_long_opt parse-options.c:378:10\n #7 0x8659bd in parse_options_step parse-options.c:817:11\n #8 0x867fcd in parse_options parse-options.c:870:10\n #9 0x56c62b in cmd_for_each_ref builtin/for-each-ref.c:59:2\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dabe in main common-main.c:52:11\n #15 0x7f2bdc570349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/for-each-ref.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex cb9c81a04606..84efb71f82fc 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -83,5 +83,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < maxcount; i++)\n \t\tshow_ref_array_item(array.items[i], &format);\n \tref_array_clear(&array);\n+\tfree_commit_list(filter.with_commit);\n+\tfree_commit_list(filter.no_commit);\n+\tUNLEAK(sorting);\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"422891","messageId":"20c5f2e68c54bdd7d07acdc256f8895cabbca6c2.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 08/12] builtin/checkout: clear pending objects after diffing","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:15Z","receivedAt":"2021-04-25T14:16:36Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nadd_pending_object() populates rev.pending, we need to take care of\nclearing it once we're done.\n\nThis code is run close to the end of a checkout, therefore this leak\nseems like it would have very little impact. See also LSAN output\nfrom t0020 below:\n\nDirect leak of 2048 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9acc46 in xrealloc wrapper.c:126:8\n #2 0x83e3a3 in add_object_array_with_path object.c:337:3\n #3 0x8f672a in add_pending_object_with_path revision.c:329:2\n #4 0x8eaeab in add_pending_object_with_mode revision.c:336:2\n #5 0x8eae9d in add_pending_object revision.c:342:2\n #6 0x5154a0 in show_local_changes builtin/checkout.c:602:2\n #7 0x513b00 in merge_working_tree builtin/checkout.c:979:3\n #8 0x512cb3 in switch_branches builtin/checkout.c:1242:9\n #9 0x50f8de in checkout_branch builtin/checkout.c:1646:9\n #10 0x50ba12 in checkout_main builtin/checkout.c:2003:9\n #11 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8\n #12 0x4cd91d in run_builtin git.c:467:11\n #13 0x4cb5f3 in handle_builtin git.c:719:3\n #14 0x4ccf47 in run_argv git.c:808:4\n #15 0x4caf49 in cmd_main git.c:939:19\n #16 0x69e43e in main common-main.c:52:11\n #17 0x7f5dd1d50349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 2048 byte(s) leaked in 1 allocation(s).\nSigned-off-by: Andrzej Hunt \n---\n builtin/checkout.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4c696ef4805b..190153c81571 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -602,6 +602,7 @@ static void show_local_changes(struct object *head,\n \tdiff_setup_done(&rev.diffopt);\n \tadd_pending_object(&rev, head, NULL);\n \trun_diff_index(&rev, 0);\n+\tobject_array_clear(&rev.pending);\n }\n \n static void describe_detached_head(const char *msg, struct commit *commit)\n-- \ngitgitgadget\n\n"},{"id":"422892","messageId":"a67168677477c18e9ee1416f3e4e5701e1eebcb6.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 11/12] builtin/rebase: release git_format_patch_opt too","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:18Z","receivedAt":"2021-04-25T14:16:37Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\noptions.git_format_patch_opt can be populated during cmd_rebase's setup,\nand will therefore leak on return. Although we could just UNLEAK all of\noptions, we choose to strbuf_release() the individual member, which matches\nthe existing pattern (where we're freeing invidual members of options).\n\nLeak found when running t0021:\n\nDirect leak of 24 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9ac296 in xrealloc wrapper.c:126:8\n #2 0x93b13d in strbuf_grow strbuf.c:98:2\n #3 0x93bd3a in strbuf_add strbuf.c:295:2\n #4 0x60ae92 in strbuf_addstr strbuf.h:304:2\n #5 0x605f17 in cmd_rebase builtin/rebase.c:1759:3\n #6 0x4cd91d in run_builtin git.c:467:11\n #7 0x4cb5f3 in handle_builtin git.c:719:3\n #8 0x4ccf47 in run_argv git.c:808:4\n #9 0x4caf49 in cmd_main git.c:939:19\n #10 0x69dbfe in main common-main.c:52:11\n #11 0x7f66dae91349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/rebase.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex ed1da1760e4c..a756fba23330 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -2109,6 +2109,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.head_name);\n \tfree(options.gpg_sign_opt);\n \tfree(options.cmd);\n+\tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \treturn ret;\n }\n-- \ngitgitgadget\n\n"},{"id":"422893","messageId":"703cd9656bf8827565469c71a6bcca58f1a5647a.1619360180.git.gitgitgadget@gmail.com","threadId":"55465","inReplyTo":"pull.929.v2.git.1619360180.gitgitgadget@gmail.com","subject":"[PATCH v2 12/12] builtin/rm: avoid leaking pathspec and seen","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-25T14:16:19Z","receivedAt":"2021-04-25T14:16:38Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nparse_pathspec() populates pathspec, hence we need to clear it once it's\nno longer needed. seen is xcalloc'd within the same function and\nlikewise needs to be freed once its no longer needed.\n\ncmd_rm() has multiple early returns, therefore we need to clear or free\nas soon as this data is no longer needed, as opposed to doing a cleanup\nat the end.\n\nLSAN output from t0020:\n\nDirect leak of 112 byte(s) in 1 object(s) allocated from:\n #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0x9ac0a4 in do_xmalloc wrapper.c:41:8\n #2 0x9ac07a in xmalloc wrapper.c:62:9\n #3 0x873277 in parse_pathspec pathspec.c:582:2\n #4 0x646ffa in cmd_rm builtin/rm.c:266:2\n #5 0x4cd91d in run_builtin git.c:467:11\n #6 0x4cb5f3 in handle_builtin git.c:719:3\n #7 0x4ccf47 in run_argv git.c:808:4\n #8 0x4caf49 in cmd_main git.c:939:19\n #9 0x69dc0e in main common-main.c:52:11\n #10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x9ac2a6 in xrealloc wrapper.c:126:8\n #2 0x93b14d in strbuf_grow strbuf.c:98:2\n #3 0x93ccf6 in strbuf_vaddf strbuf.c:392:3\n #4 0x93f726 in xstrvfmt strbuf.c:979:2\n #5 0x93f8b3 in xstrfmt strbuf.c:989:8\n #6 0x92ad8a in prefix_path_gently setup.c:115:15\n #7 0x873a8d in init_pathspec_item pathspec.c:439:11\n #8 0x87334f in parse_pathspec pathspec.c:589:3\n #9 0x646ffa in cmd_rm builtin/rm.c:266:2\n #10 0x4cd91d in run_builtin git.c:467:11\n #11 0x4cb5f3 in handle_builtin git.c:719:3\n #12 0x4ccf47 in run_argv git.c:808:4\n #13 0x4caf49 in cmd_main git.c:939:19\n #14 0x69dc0e in main common-main.c:52:11\n #15 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 15 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ac048 in xstrdup wrapper.c:29:14\n #2 0x873ba2 in init_pathspec_item pathspec.c:468:20\n #3 0x87334f in parse_pathspec pathspec.c:589:3\n #4 0x646ffa in cmd_rm builtin/rm.c:266:2\n #5 0x4cd91d in run_builtin git.c:467:11\n #6 0x4cb5f3 in handle_builtin git.c:719:3\n #7 0x4ccf47 in run_argv git.c:808:4\n #8 0x4caf49 in cmd_main git.c:939:19\n #9 0x69dc0e in main common-main.c:52:11\n #10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 1 byte(s) in 1 object(s) allocated from:\n #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0x9ac392 in xcalloc wrapper.c:140:8\n #2 0x647108 in cmd_rm builtin/rm.c:294:9\n #3 0x4cd91d in run_builtin git.c:467:11\n #4 0x4cb5f3 in handle_builtin git.c:719:3\n #5 0x4ccf47 in run_argv git.c:808:4\n #6 0x4caf49 in cmd_main git.c:939:19\n #7 0x69dbfe in main common-main.c:52:11\n #8 0x7f4fac1b0349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/rm.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4858631e0f02..2927678d37b6 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -327,6 +327,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\tif (!seen_any)\n \t\t\texit(0);\n \t}\n+\tclear_pathspec(&pathspec);\n+\tfree(seen);\n \n \tif (!index_only)\n \t\tsubmodules_absorb_gitdir_if_needed();\n-- \ngitgitgadget\n"},{"id":"423164","messageId":"xmqq8s53dyrn.fsf@gitster.g","threadId":"55465","inReplyTo":"217f571f8ef5f3a46c0cbb1ceca022a18e5b43d2.1619360180.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 09/12] mailinfo: also free strbuf lists when clearing mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-28T00:43:40Z","receivedAt":"2021-04-28T00:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrzej Hunt via GitGitGadget\" writes:\n> However, strbuf_list_free() cannot handle a NULL input, and the lists we\n> are freeing might be NULL. Therefore we add a NULL check in\n> strbuf_list_free() to make it safe to use with a NULL input (which is a\n> pattern used by some of the other *_free() functions around git).\n\nOK.\n\nThanks.\n"}]}