Re: [Tarantool-patches] [PATCH 1/4] test: add infrastructure for fuzzing testing and fuzzers

[Igor Munkin <imun@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!
> >

<snipped>

> >>
> > 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.

> 

<snipped>

> > I believe the testing is not OK if <calloc> 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!

> 

<snipped>

> > 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!

> 

<snipped>

> >
> >>> 2. Do you need to specify <address> 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].

<snipped>

> 
> 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