From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 814214765E0 for ; Thu, 24 Dec 2020 20:25:49 +0300 (MSK) References: <20201207172444.GE5396@tarantool.org> <51ad8f7a-c804-7e06-ceb9-0c8adcc56d16@tarantool.org> <20201220133109.GN5396@tarantool.org> <1cc806a8-62d2-947b-eb74-efd80dafca49@tarantool.org> <20201224132254.GA5396@tarantool.org> From: Sergey Bronnikov Message-ID: <2ab21816-244d-7fd7-48cf-4e7076fbb64f@tarantool.org> Date: Thu, 24 Dec 2020 20:25:48 +0300 MIME-Version: 1.0 In-Reply-To: <20201224132254.GA5396@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH 1/4] test: add infrastructure for fuzzing testing and fuzzers List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, On 24.12.2020 16:22, Igor Munkin wrote: > Sergey, > > Thanks for the fixes! Unfortunately, I still see 72 symbols violation in > commit message for the first and last patches on your remote branch. > Please, also consider my other notes below. Fixed them too. > On 24.12.20, Sergey Bronnikov wrote: >> Hi, >> >> On 20.12.2020 16:31, Igor Munkin wrote: >>> Sergey, >>> >>> Thanks for the changes! >>> > > >>> Neat, now everything works fine. However, considering your comment, I >>> have a newbie question (since I'm not an expert in fuzzing testing): how >>> do we need to check whether parsing finishes right or not? >> libfuzzer has a number of settings and one of them is flag that controls >> time of single unit execution. > I asked about the check if parsing succeeds or not, but you answered > this question below. > > > >>> I believe the testing is not OK if yields NULL, but the code >>> returns 0. This is odd, IMHO. What about adding either assert or abort >>> to handle this branch? To make asserts work all time simply undefine >>> NDEBUG at the beginning of the test. Same for other cases. >> Igor, I think you get everything wrong ;) Let me explain. >> >> We don't write a highly reliable and safety code here. Everything we >> need is just to properly pass a junk to a function under test. >> >> The goal of fuzzing testing is to find errors like buffer-overflows, >> use-after-free and so on. > AFAIU, these tests do not check if the "passed junk" is parsed fine. Am > I right? Absolutely! These tests are not about correctness. > >> Lack of memory during testing is rare case and I think we don't need to >> catch such cases here. >> >> Because triggered assert due to lack of memory is useless information >> from test, >> >> I don't know how we can improve Tarantool with such information. >> Gracefully exit is more than enough. > OK, then. > >> Moreover I have took a look on source code of tests for other opensource >> projects that were already used in OSS-Fuzz. >> >> They don't care about return codes from calloc(), malloc() functions at >> all. See for example [1]. > "А если все пойдут с моста прыгать, ты тоже пойдешь?" Ты напомнил мне мою учительницу, она тоже так говорила. > Anyway, I get your point, thanks for clarification! > > > >>> Why these compile flags are added under this particular condition? >> Because when OSS Fuzz is enabled compiler and link flags passed >> >> from outside. See description how to integrate project to OSS Fuzz in [2]. > Glad to see this in commit message, thanks! > > > >>>>> 2. Do you need to specify
flag once more, when ASAN is >>>>> enabled? If not the hunk above looks excess, doesn't it? >>>> Agree, it was a bad idea to manage UBSan and ASAN flags in yet another >>>> place. > I guess this should be fixed in scope of the first patch, but I see you > squashed it to the last one. Why? It seems because I was triggered by OSS_FUZZ in hunk and squashed it to a commit that introduce an OSS_FUZZ support. Fixed it and also moved add_compile_options() to the first commit too (otherwise project source code is not instrumented with -fsanitize=fuzzer-no-link). In last commit now: --- a/test/fuzz/CMakeLists.txt +++ b/test/fuzz/CMakeLists.txt @@ -9,12 +9,23 @@ add_library(fuzzer_config INTERFACE)  target_compile_options(      fuzzer_config      INTERFACE -        -fsanitize=fuzzer,address +        $<$>: +        -fsanitize=fuzzer +        > +        $<$: +        ${CXX} +        ${CXXFLAGS} +        >  )  target_link_libraries(      fuzzer_config      INTERFACE -        -fsanitize=fuzzer,address +        $<$>: +        -fsanitize=fuzzer +        > +        $<$: +        $ENV{LIB_FUZZING_ENGINE} +        >  )  # Use PUBLIC to force 'fuzzer_config' for all dependent targets.