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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

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().

What is bothering me, is that this not initialized bool was almost always
true on my machine (due to not-zero garbage). I changed it to true, and
the tests passed. Reverted back to false - passed as well.

So there either is missing a good test on that, or vy_run_iterator_load_page()
does not need the parameter 'bool *equal_found'. Because its input and
output values don't matter. In both invocation places this parameter does
not affect anything.

> diff --git a/src/box/vy_run.c b/src/box/vy_run.c
> index 54cf028d0..db4565954 100644
> --- a/src/box/vy_run.c
> +++ b/src/box/vy_run.c
> @@ -1129,7 +1129,7 @@ vy_run_iterator_search(struct vy_run_iterator *itr,
>  					       equal_key);
>  	if (pos->page_no == itr->slice->run->info.page_count)
>  		return 1;
> -	bool equal_in_page;
> +	bool equal_in_page = false;

This I changed to true, and all the tests passed.