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

[Igor Munkin <imun@tarantool.org>]

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

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

> 

<snipped>

> 
> -- 
> sergeyb@

-- 
Best regards,
IM