From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 95D9F42EF5C for ; Thu, 18 Jun 2020 00:56:42 +0300 (MSK) References: <20200616232142.GB9358@tarantool.org> From: Vladislav Shpilevoy Message-ID: <1e383509-9e5e-03ea-b138-ef0ffc8c4b08@tarantool.org> Date: Wed, 17 Jun 2020 23:56:40 +0200 MIME-Version: 1.0 In-Reply-To: <20200616232142.GB9358@tarantool.org> 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 Cc: tarantool-patches@dev.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".