Re: [Tarantool-patches] [PATCH] vinyl: fix passing uninitialized parameter to vy_page_find_key()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

>> On 15/06/2020 20:42, Nikita Pettik wrote:
>>> vy_page_find_key() assumes that equal_key parameter is initialized since
>>> it is used unconditionally. There are several places where
>>> vy_page_find_key() is called:
>>>
>>> - vy_slice_stream_search() calls vy_page_find_key();
>>> - vy_run_iterator_read() calls vy_run_iterator_load_page(),
>>> which in turn calls vy_page_find_key();
>>> - vy_run_iterator_search() also calls vy_run_iterator_load_page().
>>>
>>> First two fixes are obvious - lifespan of parameter passed to
>>> *_find_key() is clear and restricted by caller. In the last case
>>> firstly vy_page_find_key() is called, but equal_key output value is not
>>> used. Then it is re-assigned with task->equal_found which is the result
>>> of another on vy_page_find_key() invocation in vy_page_read_cb. So it is
>>> safe to initialize equal_found parameter with 'false' value as well.
>>
>> You said that in the last case vy_page_find_key() is called first, but
>> I don't see where. vy_run_iterator_search() does not call vy_page_find_key().
> 
> vy_run_iterator_search()
>   --> vy_run_iterator_load_page()
>         --> 1021 vy_page_find_key()
>         --> 1051 task->equal_found = false;
>         --> 1056 *equal_found = task->equal_found;

But I see, that the variable is used, if vy_page_find_key() is called.
Consider this path:

	vy_run_iterator_search()
	    vy_run_iterator_load_page(&equal_in_page)
	        vy_page_find_key(&equal_in_page)
	    *equal_key = equal_in_page;

equal_in_page after all becomes output value of
vy_run_iterator_search().

If you still think it should be false, then probably we need to
make the initialization inside vy_run_iterator_load_page(). Anyway
in both invocations a caller now need to set it to false.

Perhaps even vy_page_find_key() could do that. Why is it needed
not to reset it to false on each vy_page_find_key() call?

> Also see b907231713a75d716793505ad727ecea9415d774
> Analogue of equal_key was equal_in_page which was 'false' by default.
> It's pretty sad that tests do not cover this path. Could you please
> file an issue to provide proper test? I'll try to come up with test some day.

I think we can just keep this issue open, but still push this
commit as 'Part of'. To fix the builds. Because a new issue anyway
would be exactly the same + a comment saying "need to test".

The commit LGTM, if "Closes" -> "Part of".