From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8AB7F42F4AD for ; Wed, 17 Jun 2020 02:10:03 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 17 Jun 2020 01:10:00 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] vinyl: fix passing uninitialized parameter to vy_page_find_key() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org 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.