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

[Sergey Bronnikov <sergeyb@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!
>>>
> <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?
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!
>
> <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?

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
+        $<$<NOT:$<BOOL:${OSS_FUZZ}>>:
+        -fsanitize=fuzzer
+        >
+        $<$<BOOL:${OSS_FUZZ}>:
+        ${CXX}
+        ${CXXFLAGS}
+        >
  )
  target_link_libraries(
      fuzzer_config
      INTERFACE
-        -fsanitize=fuzzer,address
+        $<$<NOT:$<BOOL:${OSS_FUZZ}>>:
+        -fsanitize=fuzzer
+        >
+        $<$<BOOL:${OSS_FUZZ}>:
+        $ENV{LIB_FUZZING_ENGINE}
+        >
  )

  # Use PUBLIC to force 'fuzzer_config' for all dependent targets.


<snipped>