From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 9E9FC45C304 for ; Tue, 1 Dec 2020 19:22:02 +0300 (MSK) References: <33ba1d50585e101aa025d899116c48d56be59202.1587372354.git.sergeyb@tarantool.org> <20200428111901.GQ11314@tarantool.org> <20200429120956.GB83580@pony.bronevichok.ru> <20200526155404.GN5455@tarantool.org> From: Sergey Bronnikov Message-ID: <30a7b59a-65f3-dad2-dbec-3b089a2bd249@tarantool.org> Date: Tue, 1 Dec 2020 19:22:00 +0300 MIME-Version: 1.0 In-Reply-To: <20200526155404.GN5455@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US 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, alexander.turenko@tarantool.org Hi, Igor! Thanks for review! I'm too late, but I finally updated a patch and send a new version [1]. Please see comments below. 1. https://lists.tarantool.org/pipermail/tarantool-patches/2020-November/020959.html On 26.05.2020 18:54, Igor Munkin wrote: > 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 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 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! > > > >>>> 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. > 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 > routine (called within routine). Updated test, now it takes return value from csv_get_error_status(). >>>> +} >>>> 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: /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: /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 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: /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. In a new version tests moved to a single directory test/fuzz like we did with unit tests. > > > >> -- >> sergeyb@