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

[Sergey Bronnikov <sergeyb@tarantool.org>]

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

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.

> > ...
> > 
> > Follows: #1809
> 
> Please, don't use ':' in gh tags. It just doesn't respect our commit
> message style.

Fixed.

<snipped>

> > +if (ENABLE_FUZZER)
> > +    set(TESTING_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/Testing")
> > +endif ()
> 
> There is no space before parentheses in other code, please adjust yours.

Fixed.

<snipped>

> > +if (ENABLE_FUZZER AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
> > +    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}")
> 
> 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?

> > +endif ()
> 
> There is no space before parentheses in other code, please adjust yours.

Fixed.

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

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

> > +endif ()
> 
> There is no space before parentheses in other code, please adjust yours.

Fixed.

> > diff --git a/src/lib/http_parser/test_http_parser.c b/src/lib/http_parser/test_http_parser.c
> > new file mode 100644
> > index 000000000..a189e71e9
> > --- /dev/null
> > +++ b/src/lib/http_parser/test_http_parser.c
> 
> This C file doesn't respect our C style guide[1], please adjust it.

Fixed.

> > @@ -0,0 +1,22 @@
> > +#include <stdlib.h>
> > +#include <stdint.h>
> > +#include <stddef.h>
> > +#include "http_parser.h"
> > +
> > +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
> > +  struct http_parser parser;
> > +  char *buf = (char*)data;
> > +  http_parser_create(&parser);
> > +  parser.hdr_name = (char *)calloc((int)size, sizeof(char));
> > +  if (parser.hdr_name == NULL) {
> > +    return -1;
> > +  }
> > +  char *end_buf = buf + size;
> > +  int rc = http_parse_header_line(&parser, &buf, end_buf, size);
> > +  free(parser.hdr_name);
> > +  if (rc != 0) {
> > +    return rc;
> > +  }
> 
> As Serge already mentioned, this branch is excess.

Agree, it's useless check.

> > +
> > +  return 0;
> > +}
> > diff --git a/src/lib/uri/CMakeLists.txt b/src/lib/uri/CMakeLists.txt
> > index 96410e5bf..77f5c5d57 100644
> > --- a/src/lib/uri/CMakeLists.txt
> > +++ b/src/lib/uri/CMakeLists.txt
> > @@ -8,3 +8,15 @@ if (CC_HAS_WNO_IMPLICIT_FALLTHROUGH)
> >          -Wno-implicit-fallthrough)
> >  endif()
> >  add_library(uri STATIC uri.c)
> > +
> > +if (ENABLE_FUZZER AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
> > +    set(TestName "test_uri")
> > +    add_executable(${TestName} ${TestName}.c)
> > +    add_compile_options(-fsanitize=fuzzer,address -g -O1)
> > +    set_target_properties(${TestName}
> > +            PROPERTIES
> > +            COMPILE_FLAGS "-fsanitize=fuzzer,address -g -O1"
> > +            LINK_FLAGS "-fsanitize=fuzzer,address")
> > +    target_link_libraries(${TestName} PRIVATE uri)
> > +    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?

> > +endif ()
> 
> There is no space before parentheses in other code, please adjust yours.

Fixed.

> > diff --git a/src/lib/uri/test_uri.c b/src/lib/uri/test_uri.c
> > new file mode 100644
> > index 000000000..ad8db6ef2
> > --- /dev/null
> > +++ b/src/lib/uri/test_uri.c
> 
> This C file doesn't respect our C style guide[1], please adjust it.

Fixed.

> > @@ -0,0 +1,22 @@
> > +#include <stdlib.h>
> > +#include <stdint.h>
> > +#include <stddef.h>
> > +#include <string.h>
> > +#include "uri.h"
> > +
> > +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
> > +  char *buf = calloc(size, sizeof(char*));
> > +  if (!buf) {
> > +      return -1;
> > +  }
> > +  strncpy(buf, (char*)data, size);
> > +  buf[size] = '\0';
> > +  struct uri uri;
> > +  int rc = uri_parse(&uri, buf);
> > +  free(buf);
> > +  if (rc != 0) {
> > +      return rc;
> > +  }
> 
> As Serge already mentioned, this branch is excess.

Fixed.

<snipped>

-- 
sergeyb@