From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 C245E4696C3 for ; Wed, 29 Apr 2020 15:10:10 +0300 (MSK) Date: Wed, 29 Apr 2020 15:09:56 +0300 From: Sergey Bronnikov Message-ID: <20200429120956.GB83580@pony.bronevichok.ru> References: <33ba1d50585e101aa025d899116c48d56be59202.1587372354.git.sergeyb@tarantool.org> <20200428111901.GQ11314@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200428111901.GQ11314@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: Igor Munkin Cc: o.piskunov@tarantool.org, tarantool-patches@dev.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. > > +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. > > +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 > > +#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? 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 > > +#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. 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 > > +#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. Fixed. -- sergeyb@