Re: [PATCH 16/30] merge-recursive: Introduce new functions to handle rename logic

From: Elijah Newren <newren@gmail.com>
Date: Nov 14, 2017, 18:24 UTC
Message-ID: <CABPp-BFJm9C1zTKVLeFf6fnhxrUhTd+XZsD17DezqFKwwzsEBQ@mail.gmail.com>
In-Reply-To: <xmqqy3n94f36.fsf@gitster.mtv.corp.google.com>

On Mon, Nov 13, 2017 at 9:14 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> +struct rename_info {
>>> +    struct string_list *head_renames;
>>> +    struct string_list *merge_renames;
>>> +};
>>
>> This type is added in order to allow the caller and the helper to
>> communicate the findings in a single logical structure, instead of
>> having to pass them as separate parameters, etc.  If we anticipate
>> that the information that needs to be passed will grow richer in
>> later steps (or a follow-up series), such encapsulation makes a lot
>
> Hmph, I actually am quite confused with the existing code.
>
> The caller (originally in merge_trees(), now in handle_renames())
> calls get_renames() twice and have the list of renamed paths in
> these two string lists.  get_renames() mostly works with the
> elements in the "entries" list and adds the "struct rename" to the
> string list that is to be returned.  And the caller uses these two
> string lists get_renames() returns when calling process_renames(),
> but once process_renames() is done with them, these two string lists
> are never looked at by anybody.

Actually, if I remember correctly, my first stab was to do all the cleanup at the end of handle_renames(), but then I ran into use-after-free errors. I'm not sure if I remember all the details, but I'll try to lay out the path:

process_renames() can't handle conflicts immediately because of D/F concerns (if all entries in the competing directory resolve away, then there's no more D/F conflict, but we have to wait until each of those entries is processed to find out if that happens or if a D/F conflict remains). Because of that, process_renames() needs to store information into a rename_conflict_info struct for process_entry() to look at later. Included in rename_conflict_info are things like diff_filepair and stage_data entries, both taken from the rename lists. If the rename lists are freed at the end of handle_renames(), then this information is freed before process_entry() runs and thus we get a use-after-free error.

Since both you and I thought to push this cleanup to the end of handle_renames(), though, I should probably add that explanation to the commit message. Granted, it isn't actually needed for this particular commit, because up to this point all the information used in rename_conflict_info was leaked anyway. But it becomes an issue with patch 17 when we start freeing that info.