From db69bf608df31dde05d7be4fe2ea16f5d96c006e Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:08 +0000 Subject: [PATCH 01/12] revision: free remainder of old commit list in limit_list limit_list() iterates over the original revs->commits list, and consumes many of its entries via pop_commit. However we might stop iterating over the list early (e.g. if we realise that the rest of the list is uninteresting). If we do stop iterating early, list will be pointing to the unconsumed portion of revs->commits - and we need to free this list to avoid a leak. (revs->commits itself will be an invalid pointer: it will have been free'd during the first pop_commit.) However the list pointer is later reused to iterate over our new list, but only for the limiting_can_increase_treesame() branch. We therefore need to introduce a new variable for that branch - and while we're here we can rename the original list to original_list as that makes its purpose more obvious. This leak was found while running t0090. It's not likely to be very impactful, but it can happen quite early during some checkout invocations, and hence seems to be worth fixing: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x9ac084 in do_xmalloc wrapper.c:41:8 #2 0x9ac05a in xmalloc wrapper.c:62:9 #3 0x7175d6 in commit_list_insert commit.c:540:33 #4 0x71800f in commit_list_insert_by_date commit.c:604:9 #5 0x8f8d2e in process_parents revision.c:1128:5 #6 0x8f2f2c in limit_list revision.c:1418:7 #7 0x8f210e in prepare_revision_walk revision.c:3577:7 #8 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6 #9 0x512f05 in switch_branches builtin/checkout.c:1250:3 #10 0x50f8de in checkout_branch builtin/checkout.c:1646:9 #11 0x50ba12 in checkout_main builtin/checkout.c:2003:9 #12 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8 #13 0x4cd91d in run_builtin git.c:467:11 #14 0x4cb5f3 in handle_builtin git.c:719:3 #15 0x4ccf47 in run_argv git.c:808:4 #16 0x4caf49 in cmd_main git.c:939:19 #17 0x69dc0e in main common-main.c:52:11 #18 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 48 byte(s) in 3 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x9ac084 in do_xmalloc wrapper.c:41:8 #2 0x9ac05a in xmalloc wrapper.c:62:9 #3 0x717de6 in commit_list_append commit.c:1609:35 #4 0x8f1f9b in prepare_revision_walk revision.c:3554:12 #5 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6 #6 0x512f05 in switch_branches builtin/checkout.c:1250:3 #7 0x50f8de in checkout_branch builtin/checkout.c:1646:9 #8 0x50ba12 in checkout_main builtin/checkout.c:2003:9 #9 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8 #10 0x4cd91d in run_builtin git.c:467:11 #11 0x4cb5f3 in handle_builtin git.c:719:3 #12 0x4ccf47 in run_argv git.c:808:4 #13 0x4caf49 in cmd_main git.c:939:19 #14 0x69dc0e in main common-main.c:52:11 #15 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349) Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- revision.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/revision.c b/revision.c index b78733f508..5fdf4ca75b 100644 --- a/revision.c +++ b/revision.c @@ -1393,20 +1393,20 @@ static int limit_list(struct rev_info *revs) { int slop = SLOP; timestamp_t date = TIME_MAX; - struct commit_list *list = revs->commits; + struct commit_list *original_list = revs->commits; struct commit_list *newlist = NULL; struct commit_list **p = &newlist; struct commit_list *bottom = NULL; struct commit *interesting_cache = NULL; if (revs->ancestry_path) { - bottom = collect_bottom_commits(list); + bottom = collect_bottom_commits(original_list); if (!bottom) die("--ancestry-path given but there are no bottom commits"); } - while (list) { - struct commit *commit = pop_commit(&list); + while (original_list) { + struct commit *commit = pop_commit(&original_list); struct object *obj = &commit->object; show_early_output_fn_t show; @@ -1415,11 +1415,11 @@ static int limit_list(struct rev_info *revs) if (revs->max_age != -1 && (commit->date < revs->max_age)) obj->flags |= UNINTERESTING; - if (process_parents(revs, commit, &list, NULL) < 0) + if (process_parents(revs, commit, &original_list, NULL) < 0) return -1; if (obj->flags & UNINTERESTING) { mark_parents_uninteresting(commit); - slop = still_interesting(list, date, slop, &interesting_cache); + slop = still_interesting(original_list, date, slop, &interesting_cache); if (slop) continue; break; @@ -1452,14 +1452,17 @@ static int limit_list(struct rev_info *revs) * Check if any commits have become TREESAME by some of their parents * becoming UNINTERESTING. */ - if (limiting_can_increase_treesame(revs)) + if (limiting_can_increase_treesame(revs)) { + struct commit_list *list = NULL; for (list = newlist; list; list = list->next) { struct commit *c = list->item; if (c->object.flags & (UNINTERESTING | TREESAME)) continue; update_treesame(revs, c); } + } + free_commit_list(original_list); revs->commits = newlist; return 0; } From 5493ce7af95a266f4207d85b53fe59a8a1fa2d24 Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:09 +0000 Subject: [PATCH 02/12] wt-status: fix multiple small leaks rev.prune_data is populated (in multiple functions) via copy_pathspec, and therefore needs to be cleared after running the diff in those functions. rev(_info).pending is populated indirectly via setup_revisions, and also needs to be cleared once diffing is done. These leaks were found while running t0008 or t0021. The rev.prune_data leaks are small (80B) but noisy, hence I won't bother including their logs - the rev.pending leaks are bigger, and can happen early in the course of other commands, and therefore possibly more valuable to fix - see example log from a rebase below: Direct leak of 2048 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0x9ac2a6 in xrealloc wrapper.c:126:8 #2 0x83da03 in add_object_array_with_path object.c:337:3 #3 0x8f5d8a in add_pending_object_with_path revision.c:329:2 #4 0x8ea50b in add_pending_object_with_mode revision.c:336:2 #5 0x8ea4fd in add_pending_object revision.c:342:2 #6 0x8ea610 in add_head_to_pending revision.c:354:2 #7 0x9b55f5 in has_uncommitted_changes wt-status.c:2474:2 #8 0x9b58c4 in require_clean_work_tree wt-status.c:2553:6 #9 0x606bcc in cmd_rebase builtin/rebase.c:1970:6 #10 0x4cd91d in run_builtin git.c:467:11 #11 0x4cb5f3 in handle_builtin git.c:719:3 #12 0x4ccf47 in run_argv git.c:808:4 #13 0x4caf49 in cmd_main git.c:939:19 #14 0x69dc0e in main common-main.c:52:11 #15 0x7f2d18909349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 5 byte(s) in 1 object(s) allocated from: #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 #1 0x9ac048 in xstrdup wrapper.c:29:14 #2 0x83da8d in add_object_array_with_path object.c:349:17 #3 0x8f5d8a in add_pending_object_with_path revision.c:329:2 #4 0x8ea50b in add_pending_object_with_mode revision.c:336:2 #5 0x8ea4fd in add_pending_object revision.c:342:2 #6 0x8ea610 in add_head_to_pending revision.c:354:2 #7 0x9b55f5 in has_uncommitted_changes wt-status.c:2474:2 #8 0x9b58c4 in require_clean_work_tree wt-status.c:2553:6 #9 0x606bcc in cmd_rebase builtin/rebase.c:1970:6 #10 0x4cd91d in run_builtin git.c:467:11 #11 0x4cb5f3 in handle_builtin git.c:719:3 #12 0x4ccf47 in run_argv git.c:808:4 #13 0x4caf49 in cmd_main git.c:939:19 #14 0x69dc0e in main common-main.c:52:11 #15 0x7f2d18909349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 2053 byte(s) leaked in 2 allocation(s). Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- wt-status.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/wt-status.c b/wt-status.c index 0c8287a023..41f4c4c6ac 100644 --- a/wt-status.c +++ b/wt-status.c @@ -616,6 +616,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s) rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score; copy_pathspec(&rev.prune_data, &s->pathspec); run_diff_files(&rev, 0); + clear_pathspec(&rev.prune_data); } static void wt_status_collect_changes_index(struct wt_status *s) @@ -652,6 +653,8 @@ static void wt_status_collect_changes_index(struct wt_status *s) rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score; copy_pathspec(&rev.prune_data, &s->pathspec); run_diff_index(&rev, 1); + object_array_clear(&rev.pending); + clear_pathspec(&rev.prune_data); } static void wt_status_collect_changes_initial(struct wt_status *s) @@ -2480,6 +2483,7 @@ int has_uncommitted_changes(struct repository *r, diff_setup_done(&rev_info.diffopt); result = run_diff_index(&rev_info, 1); + object_array_clear(&rev_info.pending); return diff_result_code(&rev_info.diffopt, result); } From 4c217a4c34be29aee4aac402eed668438e8d21a4 Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:10 +0000 Subject: [PATCH 03/12] ls-files: free max_prefix when done common_prefix() returns a new string, which we store in max_prefix - this string needs to be freed to avoid a leak. This leak is happening in cmd_ls_files, hence is of no real consequence - an UNLEAK would be just as good, but we might as well free the string properly. Leak found while running t0002, see output below: Direct leak of 8 byte(s) in 1 object(s) allocated from: #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 #1 0x9ab1b4 in do_xmalloc wrapper.c:41:8 #2 0x9ab248 in do_xmallocz wrapper.c:75:8 #3 0x9ab22a in xmallocz wrapper.c:83:9 #4 0x9ab2d7 in xmemdupz wrapper.c:99:16 #5 0x78d6a4 in common_prefix dir.c:191:15 #6 0x5aca48 in cmd_ls_files builtin/ls-files.c:669:16 #7 0x4cd92d in run_builtin git.c:453:11 #8 0x4cb5fa in handle_builtin git.c:704:3 #9 0x4ccf57 in run_argv git.c:771:4 #10 0x4caf49 in cmd_main git.c:902:19 #11 0x69ce2e in main common-main.c:52:11 #12 0x7f64d4d94349 in __libc_start_main (/lib64/libc.so.6+0x24349) Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- builtin/ls-files.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index f6f9e483b2..7d1ac1eb1e 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -530,7 +530,7 @@ static int option_parse_exclude_standard(const struct option *opt, int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) { int require_work_tree = 0, show_tag = 0, i; - const char *max_prefix; + char *max_prefix; struct dir_struct dir; struct pattern_list *pl; struct string_list exclude_list = STRING_LIST_INIT_NODUP; @@ -708,5 +708,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) } dir_clear(&dir); + free(max_prefix); return 0; } From b180c681bb911714e2c9735038effe0251d6586e Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:11 +0000 Subject: [PATCH 04/12] bloom: clear each bloom_key after use fill_bloom_key() allocates memory into bloom_key, we need to clean that up once the key is no longer needed. This leak was found while running t0002-t0099. Although this leak is happening in code being called from a test-helper, the same code is also used in various locations around git, and can therefore happen during normal usage too. Gabor's analysis shows that peak-memory usage during 'git commit-graph write' is reduced on the order of 10% for a selection of larger repos (along with an even larger reduction if we override modified path bloom filter limits): https://lore.kernel.org/git/20210411072651.GF2947267@szeder.dev/ LSAN output: Direct leak of 308 byte(s) in 11 object(s) allocated from: #0 0x49a5e2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1 0x6f4032 in xcalloc wrapper.c:140:8 #2 0x4f2905 in fill_bloom_key bloom.c:137:28 #3 0x4f34c1 in get_or_compute_bloom_filter bloom.c:284:4 #4 0x4cb484 in get_bloom_filter_for_commit t/helper/test-bloom.c:43:11 #5 0x4cb072 in cmd__bloom t/helper/test-bloom.c:97:3 #6 0x4ca7ef in cmd_main t/helper/test-tool.c:121:11 #7 0x4caace in main common-main.c:52:11 #8 0x7f798af95349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 308 byte(s) leaked in 11 allocation(s). Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- bloom.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bloom.c b/bloom.c index b176f28f53..5e1db433f2 100644 --- a/bloom.c +++ b/bloom.c @@ -283,6 +283,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r, struct bloom_key key; fill_bloom_key(e->path, strlen(e->path), &key, settings); add_key_to_filter(&key, filter, settings); + clear_bloom_key(&key); } cleanup: From d895804b5a73306fa47db3745782920f60f3a040 Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:12 +0000 Subject: [PATCH 05/12] branch: FREE_AND_NULL instead of NULL'ing real_ref real_ref was previously populated by dwim_ref(), which allocates new memory. We need to make sure to free real_ref when discarding it. (real_ref is already being freed at the end of create_branch() - but if we discard it early then it will leak.) This fixes the following leak found while running t0002-t0099: Direct leak of 5 byte(s) in 1 object(s) allocated from: #0 0x486954 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 #1 0xdd6484 in xstrdup wrapper.c:29:14 #2 0xc0f658 in expand_ref refs.c:671:12 #3 0xc0ecf1 in repo_dwim_ref refs.c:644:22 #4 0x8b1184 in dwim_ref ./refs.h:162:9 #5 0x8b0b02 in create_branch branch.c:284:10 #6 0x550cbb in update_refs_for_switch builtin/checkout.c:1046:4 #7 0x54e275 in switch_branches builtin/checkout.c:1274:2 #8 0x548828 in checkout_branch builtin/checkout.c:1668:9 #9 0x541306 in checkout_main builtin/checkout.c:2025:9 #10 0x5395fa in cmd_checkout builtin/checkout.c:2077:8 #11 0x4d02a8 in run_builtin git.c:467:11 #12 0x4cbfe9 in handle_builtin git.c:719:3 #13 0x4cf04f in run_argv git.c:808:4 #14 0x4cb85a in cmd_main git.c:939:19 #15 0x820cf6 in main common-main.c:52:11 #16 0x7f30bd9dd349 in __libc_start_main (/lib64/libc.so.6+0x24349) Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- branch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/branch.c b/branch.c index 9c9dae1eae..514a3311d8 100644 --- a/branch.c +++ b/branch.c @@ -294,7 +294,7 @@ void create_branch(struct repository *r, if (explicit_tracking) die(_(upstream_not_branch), start_name); else - real_ref = NULL; + FREE_AND_NULL(real_ref); } break; default: From 4fa268738ca91347be0945c9ef12dd1ace0e70da Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:13 +0000 Subject: [PATCH 06/12] builtin/bugreport: don't leak prefixed filename prefix_filename() returns newly allocated memory, and strbuf_addstr() doesn't take ownership of its inputs. Therefore we have to make sure to store and free prefix_filename()'s result. As this leak is in cmd_bugreport(), we could just as well UNLEAK the prefix - but there's no good reason not to just free it properly. This leak was found while running t0091, see output below: Direct leak of 24 byte(s) in 1 object(s) allocated from: #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 #1 0x9acc66 in xrealloc wrapper.c:126:8 #2 0x93baed in strbuf_grow strbuf.c:98:2 #3 0x93c6ea in strbuf_add strbuf.c:295:2 #4 0x69f162 in strbuf_addstr ./strbuf.h:304:2 #5 0x69f083 in prefix_filename abspath.c:277:2 #6 0x4fb275 in cmd_bugreport builtin/bugreport.c:146:9 #7 0x4cd91d in run_builtin git.c:467:11 #8 0x4cb5f3 in handle_builtin git.c:719:3 #9 0x4ccf47 in run_argv git.c:808:4 #10 0x4caf49 in cmd_main git.c:939:19 #11 0x69df9e in main common-main.c:52:11 #12 0x7f523a987349 in __libc_start_main (/lib64/libc.so.6+0x24349) Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- builtin/bugreport.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/builtin/bugreport.c b/builtin/bugreport.c index ad3cc9c02f..9915a5841d 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -129,6 +129,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) char *option_output = NULL; char *option_suffix = "%Y-%m-%d-%H%M"; const char *user_relative_path = NULL; + char *prefixed_filename; const struct option bugreport_options[] = { OPT_STRING('o', "output-directory", &option_output, N_("path"), @@ -142,9 +143,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) bugreport_usage, 0); /* Prepare the path to put the result */ - strbuf_addstr(&report_path, - prefix_filename(prefix, - option_output ? option_output : "")); + prefixed_filename = prefix_filename(prefix, + option_output ? option_output : ""); + strbuf_addstr(&report_path, prefixed_filename); strbuf_complete(&report_path, '/'); strbuf_addstr(&report_path, "git-bugreport-"); @@ -189,6 +190,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) fprintf(stderr, _("Created new report at '%s'.\n"), user_relative_path); + free(prefixed_filename); UNLEAK(buffer); UNLEAK(report_path); return !!launch_editor(report_path.buf, NULL, NULL); From 265644367fb9fee551da2bf8349b800b196552ce Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:14 +0000 Subject: [PATCH 07/12] builtin/check-ignore: clear_pathspec before returning parse_pathspec() allocates new memory into pathspec, therefore we need to free it when we're done. An UNLEAK would probably be just as good here - but clear_pathspec() is not much more work so we might as well use it. check_ignore() is either called once directly from cmd_check_ignore() (in which case the leak really doesnt matter), or it can be called multiple times in a loop from check_ignore_stdin_paths(), in which case we're potentially leaking multiple times - but even in this scenario the leak is so small as to have no real consequence. Found while running t0008: Direct leak of 112 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x9aca44 in do_xmalloc wrapper.c:41:8 #2 0x9aca1a in xmalloc wrapper.c:62:9 #3 0x873c17 in parse_pathspec pathspec.c:582:2 #4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2 #5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17 #6 0x4cd91d in run_builtin git.c:467:11 #7 0x4cb5f3 in handle_builtin git.c:719:3 #8 0x4ccf47 in run_argv git.c:808:4 #9 0x4caf49 in cmd_main git.c:939:19 #10 0x69e43e in main common-main.c:52:11 #11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 65 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0x9acc46 in xrealloc wrapper.c:126:8 #2 0x93baed in strbuf_grow strbuf.c:98:2 #3 0x93d696 in strbuf_vaddf strbuf.c:392:3 #4 0x9400c6 in xstrvfmt strbuf.c:979:2 #5 0x940253 in xstrfmt strbuf.c:989:8 #6 0x92b72a in prefix_path_gently setup.c:115:15 #7 0x87442d in init_pathspec_item pathspec.c:439:11 #8 0x873cef in parse_pathspec pathspec.c:589:3 #9 0x503eb8 in check_ignore builtin/check-ignore.c:90:2 #10 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17 #11 0x4cd91d in run_builtin git.c:467:11 #12 0x4cb5f3 in handle_builtin git.c:719:3 #13 0x4ccf47 in run_argv git.c:808:4 #14 0x4caf49 in cmd_main git.c:939:19 #15 0x69e43e in main common-main.c:52:11 #16 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 2 byte(s) in 1 object(s) allocated from: #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 #1 0x9ac9e8 in xstrdup wrapper.c:29:14 #2 0x874542 in init_pathspec_item pathspec.c:468:20 #3 0x873cef in parse_pathspec pathspec.c:589:3 #4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2 #5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17 #6 0x4cd91d in run_builtin git.c:467:11 #7 0x4cb5f3 in handle_builtin git.c:719:3 #8 0x4ccf47 in run_argv git.c:808:4 #9 0x4caf49 in cmd_main git.c:939:19 #10 0x69e43e in main common-main.c:52:11 #11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 179 byte(s) leaked in 3 allocation(s). Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- builtin/check-ignore.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 3c652748d5..467e92cc7b 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -118,6 +118,7 @@ static int check_ignore(struct dir_struct *dir, num_ignored++; } free(seen); + clear_pathspec(&pathspec); return num_ignored; } From 52a9436aa7fd48648b6322c7c5610926165baa56 Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:15 +0000 Subject: [PATCH 08/12] builtin/checkout: clear pending objects after diffing add_pending_object() populates rev.pending, we need to take care of clearing it once we're done. This code is run close to the end of a checkout, therefore this leak seems like it would have very little impact. See also LSAN output from t0020 below: Direct leak of 2048 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0x9acc46 in xrealloc wrapper.c:126:8 #2 0x83e3a3 in add_object_array_with_path object.c:337:3 #3 0x8f672a in add_pending_object_with_path revision.c:329:2 #4 0x8eaeab in add_pending_object_with_mode revision.c:336:2 #5 0x8eae9d in add_pending_object revision.c:342:2 #6 0x5154a0 in show_local_changes builtin/checkout.c:602:2 #7 0x513b00 in merge_working_tree builtin/checkout.c:979:3 #8 0x512cb3 in switch_branches builtin/checkout.c:1242:9 #9 0x50f8de in checkout_branch builtin/checkout.c:1646:9 #10 0x50ba12 in checkout_main builtin/checkout.c:2003:9 #11 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8 #12 0x4cd91d in run_builtin git.c:467:11 #13 0x4cb5f3 in handle_builtin git.c:719:3 #14 0x4ccf47 in run_argv git.c:808:4 #15 0x4caf49 in cmd_main git.c:939:19 #16 0x69e43e in main common-main.c:52:11 #17 0x7f5dd1d50349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 2048 byte(s) leaked in 1 allocation(s). Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- builtin/checkout.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/checkout.c b/builtin/checkout.c index 2d6550bc3c..f7158af2f6 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -601,6 +601,7 @@ static void show_local_changes(struct object *head, diff_setup_done(&rev.diffopt); add_pending_object(&rev, head, NULL); run_diff_index(&rev, 0); + object_array_clear(&rev.pending); } static void describe_detached_head(const char *msg, struct commit *commit) From f3a96807912187928b904fa9374491258a30bac8 Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:16 +0000 Subject: [PATCH 09/12] mailinfo: also free strbuf lists when clearing mailinfo mailinfo.p_hdr_info/s_hdr_info are null-terminated lists of strbuf's, with entries pointing either to NULL or an allocated strbuf. Therefore we need to free those strbuf's (and not just the data they contain) whenever we're done with a given entry. (See handle_header() where those new strbufs are malloc'd.) Once we no longer need the list (and not just its entries) we can switch over to strbuf_list_free() instead of manually iterating over the list, which takes care of those additional details for us. We can only do this in clear_mailinfo() - in handle_commit_message() we are only clearing the array contents but want to reuse the array itself, hence we can't use strbuf_list_free() there. However, strbuf_list_free() cannot handle a NULL input, and the lists we are freeing might be NULL. Therefore we add a NULL check in strbuf_list_free() to make it safe to use with a NULL input (which is a pattern used by some of the other *_free() functions around git). Leak output from t0023: Direct leak of 72 byte(s) in 3 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x9ac9f4 in do_xmalloc wrapper.c:41:8 #2 0x9ac9ca in xmalloc wrapper.c:62:9 #3 0x7f6cf7 in handle_header mailinfo.c:205:10 #4 0x7f5abf in check_header mailinfo.c:583:4 #5 0x7f5524 in mailinfo mailinfo.c:1197:3 #6 0x4dcc95 in parse_mail builtin/am.c:1167:6 #7 0x4d9070 in am_run builtin/am.c:1732:12 #8 0x4d5b7a in cmd_am builtin/am.c:2398:3 #9 0x4cd91d in run_builtin git.c:467:11 #10 0x4cb5f3 in handle_builtin git.c:719:3 #11 0x4ccf47 in run_argv git.c:808:4 #12 0x4caf49 in cmd_main git.c:939:19 #13 0x69e43e in main common-main.c:52:11 #14 0x7fc1fadfa349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 72 byte(s) leaked in 3 allocation(s). Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- mailinfo.c | 14 +++----------- strbuf.c | 2 ++ 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/mailinfo.c b/mailinfo.c index 5681d9130d..95ce191f38 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -821,7 +821,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) for (i = 0; header[i]; i++) { if (mi->s_hdr_data[i]) strbuf_release(mi->s_hdr_data[i]); - mi->s_hdr_data[i] = NULL; + FREE_AND_NULL(mi->s_hdr_data[i]); } return 0; } @@ -1236,22 +1236,14 @@ void setup_mailinfo(struct mailinfo *mi) void clear_mailinfo(struct mailinfo *mi) { - int i; - strbuf_release(&mi->name); strbuf_release(&mi->email); strbuf_release(&mi->charset); strbuf_release(&mi->inbody_header_accum); free(mi->message_id); - if (mi->p_hdr_data) - for (i = 0; mi->p_hdr_data[i]; i++) - strbuf_release(mi->p_hdr_data[i]); - free(mi->p_hdr_data); - if (mi->s_hdr_data) - for (i = 0; mi->s_hdr_data[i]; i++) - strbuf_release(mi->s_hdr_data[i]); - free(mi->s_hdr_data); + strbuf_list_free(mi->p_hdr_data); + strbuf_list_free(mi->s_hdr_data); while (mi->content < mi->content_top) { free(*(mi->content_top)); diff --git a/strbuf.c b/strbuf.c index e3397cc4c7..4df30b4549 100644 --- a/strbuf.c +++ b/strbuf.c @@ -209,6 +209,8 @@ void strbuf_list_free(struct strbuf **sbs) { struct strbuf **s = sbs; + if (!s) + return; while (*s) { strbuf_release(*s); free(*s++); From a317a553b89ddad240252f1339e557eb7fc8a9d7 Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:17 +0000 Subject: [PATCH 10/12] builtin/for-each-ref: free filter and UNLEAK sorting. sorting might be a list allocated in ref_default_sorting() (in this case it's a fixed single item list, which has nevertheless been xcalloc'd), or it might be a list allocated in parse_opt_ref_sorting(). In either case we could free these lists - but instead we UNLEAK as we're at the end of cmd_for_each_ref. (There's no existing implementation of clear_ref_sorting(), and writing a loop to free the list seems more trouble than it's worth.) filter.with_commit/no_commit are populated via OPT_CONTAINS/OPT_NO_CONTAINS, both of which create new entries via parse_opt_commits(), and also need to be free'd or UNLEAK'd. Because free_commit_list() already exists, we choose to use that over an UNLEAK. LSAN output from t0041: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1 0x9ac252 in xcalloc wrapper.c:140:8 #2 0x8a4a55 in ref_default_sorting ref-filter.c:2486:32 #3 0x56c6b1 in cmd_for_each_ref builtin/for-each-ref.c:72:13 #4 0x4cd91d in run_builtin git.c:467:11 #5 0x4cb5f3 in handle_builtin git.c:719:3 #6 0x4ccf47 in run_argv git.c:808:4 #7 0x4caf49 in cmd_main git.c:939:19 #8 0x69dabe in main common-main.c:52:11 #9 0x7f2bdc570349 in __libc_start_main (/lib64/libc.so.6+0x24349) Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x9abf54 in do_xmalloc wrapper.c:41:8 #2 0x9abf2a in xmalloc wrapper.c:62:9 #3 0x717486 in commit_list_insert commit.c:540:33 #4 0x8644cf in parse_opt_commits parse-options-cb.c:98:2 #5 0x869bb5 in get_value parse-options.c:181:11 #6 0x8677dc in parse_long_opt parse-options.c:378:10 #7 0x8659bd in parse_options_step parse-options.c:817:11 #8 0x867fcd in parse_options parse-options.c:870:10 #9 0x56c62b in cmd_for_each_ref builtin/for-each-ref.c:59:2 #10 0x4cd91d in run_builtin git.c:467:11 #11 0x4cb5f3 in handle_builtin git.c:719:3 #12 0x4ccf47 in run_argv git.c:808:4 #13 0x4caf49 in cmd_main git.c:939:19 #14 0x69dabe in main common-main.c:52:11 #15 0x7f2bdc570349 in __libc_start_main (/lib64/libc.so.6+0x24349) Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- builtin/for-each-ref.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index cb9c81a046..84efb71f82 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -83,5 +83,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) for (i = 0; i < maxcount; i++) show_ref_array_item(array.items[i], &format); ref_array_clear(&array); + free_commit_list(filter.with_commit); + free_commit_list(filter.no_commit); + UNLEAK(sorting); return 0; } From 805b789a69b664bbcf9a93bff85b90d1c60bfbf5 Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:18 +0000 Subject: [PATCH 11/12] builtin/rebase: release git_format_patch_opt too options.git_format_patch_opt can be populated during cmd_rebase's setup, and will therefore leak on return. Although we could just UNLEAK all of options, we choose to strbuf_release() the individual member, which matches the existing pattern (where we're freeing invidual members of options). Leak found when running t0021: Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0x9ac296 in xrealloc wrapper.c:126:8 #2 0x93b13d in strbuf_grow strbuf.c:98:2 #3 0x93bd3a in strbuf_add strbuf.c:295:2 #4 0x60ae92 in strbuf_addstr strbuf.h:304:2 #5 0x605f17 in cmd_rebase builtin/rebase.c:1759:3 #6 0x4cd91d in run_builtin git.c:467:11 #7 0x4cb5f3 in handle_builtin git.c:719:3 #8 0x4ccf47 in run_argv git.c:808:4 #9 0x4caf49 in cmd_main git.c:939:19 #10 0x69dbfe in main common-main.c:52:11 #11 0x7f66dae91349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s). Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- builtin/rebase.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index de400f9a19..7a2e98015c 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -2119,6 +2119,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) free(options.head_name); free(options.gpg_sign_opt); free(options.cmd); + strbuf_release(&options.git_format_patch_opt); free(squash_onto_name); return ret; } From 37be11994f95d0519f3a7274df5254e56f56a7fe Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Sun, 25 Apr 2021 14:16:19 +0000 Subject: [PATCH 12/12] builtin/rm: avoid leaking pathspec and seen parse_pathspec() populates pathspec, hence we need to clear it once it's no longer needed. seen is xcalloc'd within the same function and likewise needs to be freed once its no longer needed. cmd_rm() has multiple early returns, therefore we need to clear or free as soon as this data is no longer needed, as opposed to doing a cleanup at the end. LSAN output from t0020: Direct leak of 112 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x9ac0a4 in do_xmalloc wrapper.c:41:8 #2 0x9ac07a in xmalloc wrapper.c:62:9 #3 0x873277 in parse_pathspec pathspec.c:582:2 #4 0x646ffa in cmd_rm builtin/rm.c:266:2 #5 0x4cd91d in run_builtin git.c:467:11 #6 0x4cb5f3 in handle_builtin git.c:719:3 #7 0x4ccf47 in run_argv git.c:808:4 #8 0x4caf49 in cmd_main git.c:939:19 #9 0x69dc0e in main common-main.c:52:11 #10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 65 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0x9ac2a6 in xrealloc wrapper.c:126:8 #2 0x93b14d in strbuf_grow strbuf.c:98:2 #3 0x93ccf6 in strbuf_vaddf strbuf.c:392:3 #4 0x93f726 in xstrvfmt strbuf.c:979:2 #5 0x93f8b3 in xstrfmt strbuf.c:989:8 #6 0x92ad8a in prefix_path_gently setup.c:115:15 #7 0x873a8d in init_pathspec_item pathspec.c:439:11 #8 0x87334f in parse_pathspec pathspec.c:589:3 #9 0x646ffa in cmd_rm builtin/rm.c:266:2 #10 0x4cd91d in run_builtin git.c:467:11 #11 0x4cb5f3 in handle_builtin git.c:719:3 #12 0x4ccf47 in run_argv git.c:808:4 #13 0x4caf49 in cmd_main git.c:939:19 #14 0x69dc0e in main common-main.c:52:11 #15 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 15 byte(s) in 1 object(s) allocated from: #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 #1 0x9ac048 in xstrdup wrapper.c:29:14 #2 0x873ba2 in init_pathspec_item pathspec.c:468:20 #3 0x87334f in parse_pathspec pathspec.c:589:3 #4 0x646ffa in cmd_rm builtin/rm.c:266:2 #5 0x4cd91d in run_builtin git.c:467:11 #6 0x4cb5f3 in handle_builtin git.c:719:3 #7 0x4ccf47 in run_argv git.c:808:4 #8 0x4caf49 in cmd_main git.c:939:19 #9 0x69dc0e in main common-main.c:52:11 #10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349) Direct leak of 1 byte(s) in 1 object(s) allocated from: #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 #1 0x9ac392 in xcalloc wrapper.c:140:8 #2 0x647108 in cmd_rm builtin/rm.c:294:9 #3 0x4cd91d in run_builtin git.c:467:11 #4 0x4cb5f3 in handle_builtin git.c:719:3 #5 0x4ccf47 in run_argv git.c:808:4 #6 0x4caf49 in cmd_main git.c:939:19 #7 0x69dbfe in main common-main.c:52:11 #8 0x7f4fac1b0349 in __libc_start_main (/lib64/libc.so.6+0x24349) Signed-off-by: Andrzej Hunt Signed-off-by: Junio C Hamano --- builtin/rm.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/rm.c b/builtin/rm.c index 4858631e0f..2927678d37 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -327,6 +327,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (!seen_any) exit(0); } + clear_pathspec(&pathspec); + free(seen); if (!index_only) submodules_absorb_gitdir_if_needed();