From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 50D284696C3 for ; Tue, 28 Apr 2020 14:26:10 +0300 (MSK) Date: Tue, 28 Apr 2020 14:19:01 +0300 From: Igor Munkin Message-ID: <20200428111901.GQ11314@tarantool.org> References: <33ba1d50585e101aa025d899116c48d56be59202.1587372354.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <33ba1d50585e101aa025d899116c48d56be59202.1587372354.git.sergeyb@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2] Add infrastructure for fuzzing testing and fuzzers List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Bronnikov Cc: o.piskunov@tarantool.org, tarantool-patches@dev.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 > > 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 > +#include > +#include > +#include > +#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 > +#include > +#include > +#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 > +#include > +#include > +#include > +#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