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

[Nikita Pettik]

On 17 Jun 01:10, Vladislav Shpilevoy wrote:
> 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().

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;

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