Re: [Tarantool-patches] [PATCH v2] Add infrastructure for fuzzing testing and fuzzers

[Serge Petrenko <sergepetrenko@tarantool.org>]

29.04.2020 12:28, Sergey Bronnikov wrote:
> Hi, Serge
>
> thanks for review. See my answers inline.
>
> On 16:56 Fri 24 Apr , Serge Petrenko wrote:
>
> <snipped>
>
Hi! Thanks for  the answers!


>>> +option(ENABLE_FUZZER "Enable fuzzing testing" OFF)
>>> +if (ENABLE_FUZZER)
>>> +    set(TESTING_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/Testing")
>>> +endif ()
>> Maybe report some error on an attemt to set ENABLE_FUZZER=ON
>> when the compiler isn't clang? Then there’ll be no need to check
>> for CMAKE_CXX_COMPILER_ID in each test’s file.
> Agree, added check for compiler here.

Let me paste bits of the new patch below for commenting.

==================

diff --git a/cmake/profile.cmake b/cmake/profile.cmake
index bc4bf67f5..419f7b3cc 100644
--- a/cmake/profile.cmake
+++ b/cmake/profile.cmake
@@ -42,6 +42,15 @@ else()
      add_definitions(-DNVALGRIND=1)
  endif()

+option(ENABLE_FUZZER "Enable fuzzing testing" OFF)
+if(ENABLE_FUZZER)
+    if(NOT (CMAKE_C_COMPILER_ID STREQUAL "Clang"))
+        message(WARNING "Fuzzing supported with Clang compiler only.")
+    else()
+        set(TESTING_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/Testing")
+    endif()
+endif()
+
  option(ENABLE_ASAN "Enable AddressSanitizer, a fast memory error 
detector based on compiler instrumentation" OFF)
  if (ENABLE_ASAN)
      if (CMAKE_COMPILER_IS_GNUCC)

===============

shouldn't it be message(FATAL_ERROR "...") ?  So  that cmake config fails.

Otherwise it'll produce the warning, but proceed  to build anyway.


>
>>> +
>>> option(ENABLE_ASAN "Enable AddressSanitizer, a fast memory error detector based on compiler instrumentation" OFF)
>>> if (ENABLE_ASAN)
>>>      if (CMAKE_COMPILER_IS_GNUCC)
>>> diff --git a/src/lib/csv/CMakeLists.txt b/src/lib/csv/CMakeLists.txt
>>> index 3580e4da2..d5a3ed1f6 100644
>>> --- a/src/lib/csv/CMakeLists.txt
>>> +++ b/src/lib/csv/CMakeLists.txt
>>> @@ -4,3 +4,14 @@ set(lib_sources
>>>
>>> set_source_files_compile_flags(${lib_sources})
>>> add_library(csv STATIC ${lib_sources})
>>> +
>>> +if (ENABLE_FUZZER AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
>> Is there such a thing as CMAKE_C_COMPILER_ID ?
>> If yes, then maybe check it instead of _CXX_ one ?
> Agree, replaced.

==============

diff --git a/src/lib/csv/CMakeLists.txt b/src/lib/csv/CMakeLists.txt
index 3580e4da2..9be0a9e73 100644
--- a/src/lib/csv/CMakeLists.txt
+++ b/src/lib/csv/CMakeLists.txt
@@ -4,3 +4,14 @@ set(lib_sources

  set_source_files_compile_flags(${lib_sources})
  add_library(csv STATIC ${lib_sources})
+
+if(ENABLE_FUZZER AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    set(TestName "test_csv")
=================


If you error on compiler check, as  mentioned above, there's no need to 
check

for compiler here. It's the only  place left, besides. You've omitted 
the checks in

http_parser and uri  tests.


>
>>> +    set(TestName "test_csv")
>>> +    add_executable(${TestName} ${TestName}.c)
>>> +    set_target_properties(${TestName}
>>> +            PROPERTIES
>>> +            COMPILE_FLAGS "-fsanitize=fuzzer,address -g -O1"
>>> +            LINK_FLAGS "-fsanitize=fuzzer,address")
>>> +    target_link_libraries(${TestName} PRIVATE csv)
>>> +    set_target_properties(${TestName} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${TESTING_OUTPUT_DIRECTORY}")
>>> +endif ()
>>> diff --git a/src/lib/csv/test_csv.c b/src/lib/csv/test_csv.c
>>> new file mode 100644
>>> index 000000000..aa1703514
>>> --- /dev/null
>>> +++ b/src/lib/csv/test_csv.c
>> Maybe we should put all the tests to test/fuzzing/smth ?
>> Since we have test/unit/smth.
> I did it for convenience - it is more convenient when tests lie together
> with code. I don't know why we place tarantool unit tests and
> other tests in a single directory seperately with modules that they
> tests. These three tests (csv, uri and http) are related to small libraries and
> I don't see any reasons to place them outside of library dir.

Ok, it's up to you then.

> <snipped>

--

Serge Petrenko