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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hi, Igor!

Thanks for review!

I'm too late, but I finally updated a patch and send a new version [1].

Please see comments below.

1. 
https://lists.tarantool.org/pipermail/tarantool-patches/2020-November/020959.html

On 26.05.2020 18:54, Igor Munkin wrote:
> Sergey,
>
> Thanks for the changes! Please consider my replies below.
>
> On 29.04.20, Sergey Bronnikov wrote:
>> Hi, Igor
>>
>> thanks a lot for review! See my answers inline.
>>
>> On 14:19 Tue 28 Apr , Igor Munkin wrote:
>>> Sergey,
>>>
>>> Thanks for the patch! I left several comments, please consider them.
>>>
>>> On 20.04.20, Sergey Bronnikov wrote:
>>>> - introduce CMake flag to enable building fuzzers
>>>> - add fuzzers based on LibFuzzer[1] to csv, http_parser and uri modules
>>>>
>>>> [1] https://llvm.org/docs/LibFuzzer.html
>>>>
>>>> Note: LibFuzzer requires Clang compiler
>>>>
>>>> How-To Use:
>>>>
>>>> $ mkdir build && cd build
>>>> $ CC=clang CXX=clang++ cmake -DENABLE_FUZZER=ON ..
>>>> $ make -j
>>>> $ ./Testing/test_csv corpus/ -max_total_time=60*60*60 -workers=4
> I guess corpus directory need to be created prior to the <test_csv> run.
> At least I faced the following error:
> | $ ./Testing/test_csv corpus/ -max_total_time=60*60*60 -workers=4
> | INFO: Seed: 3782841313
> | INFO: Loaded 1 modules   (3 inline 8-bit counters): 3 [0x574eb0,
> | 0x574eb3),
> | INFO: Loaded 1 PC tables (3 PCs): 3 [0x54f540,0x54f570),
> | No such file or directory: corpus/; exiting
>
> After <mkdir> everything works fine.
>
>>> I can't test your patch manually since this recipe doesn't work for me:
>> perhaps you should install package sys-libs/compiler-rt-sanitizers [1]
> This package was installed.
>
> | $ $ eix sys-libs/compiler-rt-sanitizers | head -1
> | [I] sys-libs/compiler-rt-sanitizers
>
>> 1. https://packages.gentoo.org/packages/sys-libs/compiler-rt-sanitizers
>>
>>> | $ cat /etc/os-release
>>> | NAME=Gentoo
>>> | ID=gentoo
>>> | PRETTY_NAME="Gentoo/Linux"
>>> | ANSI_COLOR="1;32"
>>> | HOME_URL="https://www.gentoo.org/"
>>> | SUPPORT_URL="https://www.gentoo.org/support/"
>>> | BUG_REPORT_URL="https://bugs.gentoo.org/"
>>> | $ clang --version
>>> | clang version 8.0.1 (tags/RELEASE_801/final)
>>> | Target: x86_64-pc-linux-gnu
>>> | Thread model: posix
>>> | InstalledDir: /usr/lib/llvm/8/bin
>>> | $ clang++ --version
>>> | clang version 8.0.1 (tags/RELEASE_801/final)
>>> | Target: x86_64-pc-linux-gnu
>>> | Thread model: posix
>>> | InstalledDir: /usr/lib/llvm/8/bin
>>> | $ git status | head -1
>>> | On branch ligurio/gh-1809-libfuzzer
>>> | $ mkdir -p libfuzzer
>>> | $ cd !$
>>> | cd libfuzzer
>>> | $ CC=clang CXX=clang++ cmake -DENABLE_FUZZER=ON ..
>>> | -- Enabling LTO: FALSE
>>> | -- OpenSSL 1.1.1d found
>>> | --
>>> | CMake Deprecation Warning at test/CMakeLists.txt:21 (cmake_policy):
>>> |   The OLD behavior for policy CMP0037 will be removed from a future version
>>> |   of CMake.
>>> |
>>> |   The cmake-policies(7) manual explains that the OLD behaviors of all
>>> |   policies are deprecated and that a policy should be set to OLD only under
>>> |   specific short-term circumstances.  Projects should be ported to the NEW
>>> |   behavior and not rely on setting a policy to OLD.
>>> |
>>> |
>>> | --
>>> | --
>>> | -- Configuring done
>>> | -- Generating done
>>> | -- Build files have been written to: /home/imun/projects/tarantool
>>> | $ echo $?
>>> | 0
>>> | $ make -j
>>> | make: *** No targets specified and no makefile found.  Stop.
>> I suspect the problem was due to specifying "CXX=clang++".
>> I have changed check for compiler and it can probably fixes your problem.
> Yeah, this might be the root cause. As I mentioned above, now everything
> is fine, thanks!
>
> <snipped>
>
>>>> 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
>>> This C file doesn't respect our C style guide[1], please adjust it.
>>>
>>>> @@ -0,0 +1,23 @@
>>>> +#include <stdint.h>
>>>> +#include <stdlib.h>
>>>> +#include <stddef.h>
>>>> +#include <string.h>
>>>> +#include "csv.h"
>>>> +
>>>> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
>>>> +  struct csv csv;
>>>> +  csv_create(&csv);
>>>> +  char *buf = calloc(size, sizeof(char*));
>>>> +  if (buf == NULL) {
>>>> +     return -1;
>>>> +  }
>>>> +  memcpy(buf, data, size);
>>>> +  buf[size] = '\0';
>>>> +  char *end = buf + size;
>>>> +  csv_parse_chunk(&csv, buf, end);
>>>> +  csv_finish_parsing(&csv);
>>>> +  csv_destroy(&csv);
>>>> +  free(buf);
>>>> +
>>>> +  return 0;
>>> So, csv parsing succeeds regardless the input? Then what is the purpose
>>> of the testing?
>> Yes, function LLVMFuzzerTestOneInput succeeds when csv parsing
>> finishes without any issues like memory corruption, buffer overflow etc.
>> Unfortunately these csv_* functions returns void and I cannot additionaly
>> check was parsing successfull or not. Anyway fuzzing is about negative testing
>> and our goal to check function for reliability with junk input.
> No, you can. Despite the void function return type you can check the
> error_status field in csv context structure which is set e.g. in
> <csv_invalid> routine (called within <csv_finish_parsing> routine).
Updated test, now it takes return value from csv_get_error_status().
>>>> +}
>>>> diff --git a/src/lib/http_parser/CMakeLists.txt b/src/lib/http_parser/CMakeLists.txt
>>>> index a48f83cb6..7af922fbb 100644
>>>> --- a/src/lib/http_parser/CMakeLists.txt
>>>> +++ b/src/lib/http_parser/CMakeLists.txt
>>>> @@ -1 +1,12 @@
>>>>   add_library(http_parser STATIC http_parser.c)
>>>> +
>>>> +if (ENABLE_FUZZER AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
>>>> +    set(TestName "test_http_parser")
>>>> +    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 http_parser)
>>>> +    set_target_properties(${TestName} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${TESTING_OUTPUT_DIRECTORY}")
>>> What if ${TESTING_OUTPUT_DIRECTORY} is not defined?
>> Why it can be undefined if I set it in cmake/profile.cmake when ENABLE_FUZZER is enabled?
> Yes, but if one runs the following command, the build succeeds:
> | $ cmake . && make -j
> | -- The C compiler identification is GNU 8.3.0
> | -- The CXX compiler identification is GNU 8.3.0
> | -- Check for working C compiler: /usr/bin/cc
> | -- Check for working C compiler: /usr/bin/cc -- works
> | -- Detecting C compiler ABI info
> | -- Detecting C compiler ABI info - done
> | -- Detecting C compile features
> | -- Detecting C compile features - done
> | -- Check for working CXX compiler: /usr/bin/c++
> | -- Check for working CXX compiler: /usr/bin/c++ -- works
> | -- Detecting CXX compiler ABI info
> | -- Detecting CXX compiler ABI info - done
> | -- Detecting CXX compile features
> | -- Detecting CXX compile features - done
> | CMake Warning (dev) in CMakeLists.txt:
> |   No cmake_minimum_required command is present.  A line of code such as
> |
> |     cmake_minimum_required(VERSION 3.14)
> |
> |   should be added at the top of the file.  The version specified may be lower
> |   if you wish to support older CMake versions for this project.  For more
> |   information run "cmake --help-policy CMP0000".
> | This warning is for project developers.  Use -Wno-dev to suppress it.
> |
> | -- Configuring done
> | -- Generating done
> | -- Build files have been written to: <tarantool-path>/src/lib/http_parser
> | Scanning dependencies of target http_parser
> | [ 50%] Building C object CMakeFiles/http_parser.dir/http_parser.o
> | [100%] Linking C static library libhttp_parser.a
> | [100%] Built target http_parser
>
> At the same time the following command fails due to invalid
> compiler/linker flags:
> | $ cmake -DENABLE_FUZZER=ON . && make -j
> | -- The C compiler identification is GNU 8.3.0
> | -- The CXX compiler identification is GNU 8.3.0
> | -- Check for working C compiler: /usr/bin/cc
> | -- Check for working C compiler: /usr/bin/cc -- works
> | -- Detecting C compiler ABI info
> | -- Detecting C compiler ABI info - done
> | -- Detecting C compile features
> | -- Detecting C compile features - done
> | -- Check for working CXX compiler: /usr/bin/c++
> | -- Check for working CXX compiler: /usr/bin/c++ -- works
> | -- Detecting CXX compiler ABI info
> | -- Detecting CXX compiler ABI info - done
> | -- Detecting CXX compile features
> | -- Detecting CXX compile features - done
> | CMake Warning (dev) in CMakeLists.txt:
> |   No cmake_minimum_required command is present.  A line of code such as
> |
> |     cmake_minimum_required(VERSION 3.14)
> |
> |   should be added at the top of the file.  The version specified may be lower
> |   if you wish to support older CMake versions for this project.  For more
> |   information run "cmake --help-policy CMP0000".
> | This warning is for project developers.  Use -Wno-dev to suppress it.
> |
> | -- Configuring done
> | -- Generating done
> | -- Build files have been written to: <tarantool-path>/src/lib/http_parser
> | Scanning dependencies of target http_parser
> | [ 25%] Building C object CMakeFiles/http_parser.dir/http_parser.o
> | [ 50%] Linking C static library libhttp_parser.a
> | [ 50%] Built target http_parser
> | Scanning dependencies of target test_http_parser
> | [ 75%] Building C object CMakeFiles/test_http_parser.dir/test_http_parser.o
> | cc: error: unrecognized argument to -fsanitize= option: 'fuzzer'
> | make[2]: *** [CMakeFiles/test_http_parser.dir/build.make:63: CMakeFiles/test_http_parser.dir/test_http_parser.o] Error 1
> | make[1]: *** [CMakeFiles/Makefile2:110: CMakeFiles/test_http_parser.dir/all] Error 2
> | make: *** [Makefile:84: all] Error 2
>
> So cmake respects the given options but has no check for toolchain. OK,
> when the proper toolchain is set <make> command succeeds and
> test_http_parser is build in the project root directory:
> | $ CC=clang CXX=clang++ cmake -DENABLE_FUZZER=ON .  && make -j
> | -- The C compiler identification is Clang 8.0.1
> | -- The CXX compiler identification is Clang 8.0.1
> | -- Check for working C compiler: /usr/lib/llvm/8/bin/clang
> | -- Check for working C compiler: /usr/lib/llvm/8/bin/clang -- works
> | -- Detecting C compiler ABI info
> | -- Detecting C compiler ABI info - done
> | -- Detecting C compile features
> | -- Detecting C compile features - done
> | -- Check for working CXX compiler: /usr/lib/llvm/8/bin/clang++
> | -- Check for working CXX compiler: /usr/lib/llvm/8/bin/clang++ -- works
> | -- Detecting CXX compiler ABI info
> | -- Detecting CXX compiler ABI info - done
> | -- Detecting CXX compile features
> | -- Detecting CXX compile features - done
> | CMake Warning (dev) in CMakeLists.txt:
> |   No cmake_minimum_required command is present.  A line of code such as
> |
> |     cmake_minimum_required(VERSION 3.14)
> |
> |   should be added at the top of the file.  The version specified may be lower
> |   if you wish to support older CMake versions for this project.  For more
> |   information run "cmake --help-policy CMP0000".
> | This warning is for project developers.  Use -Wno-dev to suppress it.
> |
> | -- Configuring done
> | -- Generating done
> | -- Build files have been written to: <tarantool-path>/src/lib/http_parser
> | Scanning dependencies of target http_parser
> | [ 25%] Building C object CMakeFiles/http_parser.dir/http_parser.o
> | [ 50%] Linking C static library libhttp_parser.a
> | [ 50%] Built target http_parser
> | Scanning dependencies of target test_http_parser
> | [ 75%] Building C object CMakeFiles/test_http_parser.dir/test_http_parser.o
> | [100%] Linking C executable test_http_parser
> | [100%] Built target test_http_parser
>
> And this is the case I was writing about. Do you consider this as a bug?
>
> Side note: this *might* be the main reason, why unit tests aren't placed
> alongside with the corresponding libs but grouped within a separate
> directory as Serge mentioned in his review.

In a new version tests moved to a single directory test/fuzz like we did 
with unit tests.


>
> <snipped>
>
>> -- 
>> sergeyb@