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 1566045C305 for ; Mon, 7 Dec 2020 15:25:01 +0300 (MSK) References: <86d41c348ae271f5a8a8dec6fdd4ccc045a0408d.1607006062.git.kyukhin@tarantool.org> <7bae6a08-1931-3616-4429-4685ea18737a@tarantool.org> <20201204085422.36tzl24q2czeipte@tarantool.org> From: Aleksandr Lyapunov Message-ID: <08a83497-ec1c-66e8-1df8-916865f36aa4@tarantool.org> Date: Mon, 7 Dec 2020 15:24:59 +0300 MIME-Version: 1.0 In-Reply-To: <20201204085422.36tzl24q2czeipte@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH] test: fix local array access in heap_iterator unit test List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirill Yukhin Cc: tarantool-patches@dev.tarantool.org Hi, thanks for the path! LGTM. On 04.12.2020 11:54, Kirill Yukhin wrote: > Hello, > > On 04 Dec 09:33, Aleksandr Lyapunov wrote: >> Hello! Thanks for the patch, see my comment below: >> >> On 03.12.2020 17:38, Kirill Yukhin wrote: >>> Index variable run from 1 to 5 and was used to index >>> array of size 4. Use iv - 1 instead. >>> >>> Discovered by Coverity. >>> --- >>> >>> Issue: N/A >>> Branch: https://github.com/tarantool/tarantool/tree/kyukhin/fix-heap_iterator >>> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/224811793 >>> >>> test/unit/heap_iterator.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/test/unit/heap_iterator.c b/test/unit/heap_iterator.c >>> index 4fde39d..3fcefff 100644 >>> --- a/test/unit/heap_iterator.c >>> +++ b/test/unit/heap_iterator.c >>> @@ -111,15 +111,15 @@ test_iterator_small() >>> if (val < 1 || val > 5) >> Accessing below by [val - 1] is ok, but the main problem is that this check >> is wrong. >> If the heap works correctly - 'val' must be in range [1..4]. >> I think we should fix to "val < 1 || val > 4". >> After that both variants of accessing array by [val] and [val - 1] would be >> ok. > I agree, thanks. Updated patch in the bottom. Branch force-pushed. > I've accidentally committed this version to master. So, this is iterative. > > -- > Regards, Kirill Yukhin > > Author: Kirill Yukhin > Date: Fri Dec 4 11:50:31 2020 +0300 > > test: fixup unit/heap_iterator > > Accidentally committed [1] which was under review. > This is proper fix. > > [1] - 86d41c348ae271f5a8a8dec6fdd4ccc045a0408d > > diff --git a/test/unit/heap_iterator.c b/test/unit/heap_iterator.c > index 3fcefff..46723b3 100644 > --- a/test/unit/heap_iterator.c > +++ b/test/unit/heap_iterator.c > @@ -98,7 +98,7 @@ test_iterator_small() > } > > struct heap_iterator it; > - bool used_key[5]; > + bool used_key[4]; > memset((void *)used_key, 0, sizeof(used_key)); > > test_heap_iterator_init(&heap, &it); > @@ -108,9 +108,9 @@ test_iterator_small() > if (value == NULL) > fail("NULL returned from iterator", "value == NULL"); > uint32_t val = value->val1; > - if (val < 1 || val > 5) > + if (val < 1 || val > 4) > fail("from iterator returned incorrect value", > - "val < 1 || val > 5"); > + "val < 1 || val > 4"); > if (used_key[val - 1]) > fail("from iterator some value returned twice", > "used[val]"); > @@ -118,8 +118,8 @@ test_iterator_small() > } > > bool f = true; > - for (uint32_t i = 1; i < 5; ++i) > - f = used_key[i - 1] && f; > + for (uint32_t i = 0; i < 4; ++i) > + f = used_key[i] && f; > if (!f) > fail("some node was skipped", "!f");