From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 9AC464429E1 for ; Mon, 22 Jun 2020 03:02:19 +0300 (MSK) Date: Mon, 22 Jun 2020 00:02:18 +0000 From: Nikita Pettik Message-ID: <20200622000218.GA29547@tarantool.org> References: <20200616232142.GB9358@tarantool.org> <1e383509-9e5e-03ea-b138-ef0ffc8c4b08@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1e383509-9e5e-03ea-b138-ef0ffc8c4b08@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 17 Jun 23:56, Vladislav Shpilevoy wrote: > >> 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? Agree, let's do it. New patch is: diff --git a/src/box/vy_run.c b/src/box/vy_run.c index 54cf028d0..b9822dc3e 100644 --- a/src/box/vy_run.c +++ b/src/box/vy_run.c @@ -790,7 +790,7 @@ vy_page_stmt(struct vy_page *page, uint32_t stmt_no, * Binary search in page * In terms of STL, makes lower_bound for EQ,GE,LT and upper_bound for GT,LE * Additionally *equal_key argument is set to true if the found value is - * equal to given key (untouched otherwise) + * equal to given key (set to false otherwise). * @retval position in the page */ static uint32_t @@ -800,6 +800,7 @@ vy_page_find_key(struct vy_page *page, struct vy_entry key, { uint32_t beg = 0; uint32_t end = page->row_count; + *equal_key = false; /* for upper bound we change zero comparison result to -1 */ int zero_cmp = (iterator_type == ITER_GT || iterator_type == ITER_LE ? -1 : 0); > > 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". I've dived a bit in git history and found original patch introducing this function: b7724150b3017af60b28e3491d971d9eab1d709e (it was called vy_run_iterator_search_in_page()). Its the only usage was with equal_key initialized to 'false'. I believe it is the right way this function should be used.