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 ED28B4765E0 for ; Thu, 24 Dec 2020 16:22:59 +0300 (MSK) Date: Thu, 24 Dec 2020 16:22:54 +0300 From: Igor Munkin Message-ID: <20201224132254.GA5396@tarantool.org> References: <20201207172444.GE5396@tarantool.org> <51ad8f7a-c804-7e06-ceb9-0c8adcc56d16@tarantool.org> <20201220133109.GN5396@tarantool.org> <1cc806a8-62d2-947b-eb74-efd80dafca49@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1cc806a8-62d2-947b-eb74-efd80dafca49@tarantool.org> 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: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org 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. 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? > > 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? > > Side note: You can oblige one to enable ASAN/UBSAN the same way, you > > restrict building via clang. Of course if it makes sense :) > Looks like it is no sense. GCC also has support of sanitizers, [3]. > > 1. > https://github.com/google/oss-fuzz/blob/master/projects/unbound/fuzz_2.c#L16 > > 2. > https://google.github.io/oss-fuzz/getting-started/new-project-guide/#buildsh > > 3. https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html > -- Best regards, IM