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

[Igor Munkin <imun@tarantool.org>]

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

> ...
> 
> Follows: #1809

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

> ---
>  CMakeLists.txt                         |  2 +-
>  cmake/profile.cmake                    |  5 +++++
>  src/lib/csv/CMakeLists.txt             | 11 +++++++++++
>  src/lib/csv/test_csv.c                 | 23 +++++++++++++++++++++++
>  src/lib/http_parser/CMakeLists.txt     | 11 +++++++++++
>  src/lib/http_parser/test_http_parser.c | 22 ++++++++++++++++++++++
>  src/lib/uri/CMakeLists.txt             | 12 ++++++++++++
>  src/lib/uri/test_uri.c                 | 22 ++++++++++++++++++++++
>  8 files changed, 107 insertions(+), 1 deletion(-)
>  create mode 100644 src/lib/csv/test_csv.c
>  create mode 100644 src/lib/http_parser/test_http_parser.c
>  create mode 100644 src/lib/uri/test_uri.c
> 

<snipped>

> diff --git a/cmake/profile.cmake b/cmake/profile.cmake
> index bc4bf67f5..b9fcd7655 100644
> --- a/cmake/profile.cmake
> +++ b/cmake/profile.cmake
> @@ -42,6 +42,11 @@ else()
>      add_definitions(-DNVALGRIND=1)
>  endif()
>  
> +option(ENABLE_FUZZER "Enable fuzzing testing" OFF)
> +if (ENABLE_FUZZER)
> +    set(TESTING_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/Testing")
> +endif ()

There is no space before parentheses in other code, please adjust yours.

> +
>  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")
> +    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?

> +endif ()

There is no space before parentheses in other code, please adjust yours.

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

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

> +endif ()

There is no space before parentheses in other code, please adjust yours.

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

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

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

> +endif ()

There is no space before parentheses in other code, please adjust yours.

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

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

> +
> +  return 0;
> +}
> -- 
> 2.23.0
> 
> 
> -- 
> sergeyb@

[1]: https://www.tarantool.io/en/doc/2.2/dev_guide/c_style_guide/

-- 
Best regards,
IM