From: Andrzej Hunt Date: Sun, 25 Apr 2021 13:32:53 GMT Subject: Re: [PATCH 01/12] revision: free remainder of old commit list in limit_list Message-ID: In-Reply-To: <797e5ce8-14e2-0689-cf19-4426c1c8bd5d@web.de> On 10/04/2021 09:29, René Scharfe wrote: > Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget: >> From: Andrzej Hunt >> >> 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.) >> >> 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 >> --- >> revision.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/revision.c b/revision.c >> index 553c0faa9b38..7b509aab0c87 100644 >> --- a/revision.c >> +++ b/revision.c >> @@ -1460,6 +1460,7 @@ static int limit_list(struct rev_info *revs) >> update_treesame(revs, c); >> } >> >> + free_commit_list(list); > > This patch would benefit from more context, but this function is quite > long. So let me sketch it: > > struct commit_list *list = revs->commits; > > while (list) { > struct commit *commit = pop_commit(&list); > struct object *obj = &commit->object; > > if (obj->flags & UNINTERESTING) { > break; > } > } > > if (limiting_can_increase_treesame(revs)) > for (list = newlist; list; list = list->next) { > } > > free_commit_list(list); > > So the while loop can leave list dangling and you want to free its > remaining entries. The for loop sometimes overwrites the list pointer, > though, and you will end up passing NULL to free_commit_list in that > case. So either the call should be moved between the loops or a fresh > variable should be used in the second loop instead of reusing list to > make sure the entries are released in all cases. Good catch, I did not look closely enough at this one - V1 definitely is buggy*. I've decided I'll add a new variable for the list in V2, but I also took the opportunity to rename the original list since I think that makes it more obvious where that list came from in the first place. * However I also didn't run into any failures when running the entire test-suite with this change, so I'm guessing this codepath isn't being exercised by our tests. I'm hoping to try and investigate this in more detail when I find a spare moment.