Re: [PATCH v6 6/6] convert: add "status=delayed" to filter process protocol
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 26, 2017, 22:38 UTC
- Message-ID
- <xmqqzicu2x4a.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <xmqqa84u4cuk.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 42 quoted lines
>>>> + filter->string = "";
>>>> + continue;
>>>> + }
>>>> +
>>>> + /* In dco->paths we store a list of all delayed paths.
>>>> + The filter just send us a list of available paths.
>>>> + Remove them from the list.
>>>> + */
>>>> + filter_string_list(&dco->paths, 0,
>>>> + &remove_available_paths, &available_paths);
>>>
>>> We first remove from the outstanding request list (dco->paths) what
>>> are now ready...
>>>
>>>> + for_each_string_list_item(path, &available_paths) {
>>>
>>> ...and go over those paths that are now ready.
>>>
>>>> + struct cache_entry* ce = index_file_exists(
>>>> + state->istate, path->string,
>>>> + strlen(path->string), 0);
>>>> + assert(dco->state == CE_RETRY);
>>>> + errs |= (ce ? checkout_entry(ce, state, NULL) : 1);
>>>> + }
>>>
>>> But we never checked if the contents of this available_paths list is
>>> a subset of the set of paths we originally asked the external
>>> process to filter.
>>
>> Correct.
>>
>>> This will allow the process to overwrite any
>>> random path that is not even involved in the checkout.
>>
>> No, not "any random path". Only paths that are part of the checkout.
>
> Isn't it "any path that index_file_exists() returns a CE for". Did
> you filter out elements in available_paths that weren't part of
> dco->paths? I thought the filter-string-list you have are for the
> other way around (which is necessary to keep track of work to be
> done, but that filtering does not help rejecting rogue responses at
> all).That is, something along the lines of this on top of the series. When going over the list of delayed paths to see if any of them is answered, in remove_available_paths(), we mark the util field of the answered one. Later when we go over the list of answered one, we make sure that it was actually matched with something on dco->paths before calling checkout_entry().
entry.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/entry.c b/entry.c index c6d5cc01dc..f2af67e015 100644 --- a/entry.c +++ b/entry.c @@ -150,7 +150,12 @@ void enable_delayed_checkout(struct checkout *state) static int remove_available_paths(struct string_list_item *item, void *cb_data) { struct string_list *available_paths = cb_data; - return !string_list_has_string(available_paths, item->string); + struct string_list_item *available; + + available = string_list_lookup(available_paths, item->string); + if (available) + avaiable->util = (void *)item->string; + return !available; } int finish_delayed_checkout(struct checkout *state) @@ -192,9 +197,16 @@ int finish_delayed_checkout(struct checkout *state) &remove_available_paths, &available_paths); for_each_string_list_item(path, &available_paths) { - struct cache_entry* ce = index_file_exists( - state->istate, path->string, - strlen(path->string), 0); + struct cache_entry* ce; + + if (!path->util) { + error("filter gave '%s' which was unasked for", + path->string); + errs |= 1; + continue; + } + ce = index_file_exists(state->istate, path->string, + strlen(path->string), 0); assert(dco->state == CE_RETRY); errs |= (ce ? checkout_entry(ce, state, NULL) : 1); }