Re: [PATCH 01/12] revision: free remainder of old commit list in limit_list

> Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget:
>> From: Andrzej Hunt <ajrhunt@google.com>
>>
>> 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 <ajrhunt@google.com>
>> ---
>>   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.