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 B421B45C305 for ; Mon, 7 Dec 2020 20:24:47 +0300 (MSK) Date: Mon, 7 Dec 2020 20:24:44 +0300 From: Igor Munkin Message-ID: <20201207172444.GE5396@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 1/4] test: add infrastructure for fuzzing testing and fuzzers List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org Cc: tarantool-patches@dev.tarantool.org Sergey, Thanks for the patch! Please consider the remaining comments below. On 30.11.20, sergeyb@tarantool.org wrote: > From: Sergey Bronnikov > > There is a number of bugs related to parsing and encoding/decoding data. > Examples: > > - csv: #2692, #4497, #2692 > - uri: #585 > > One of the effective method to find such issues is a fuzzing testing. > Patch introduce a CMake flag to enable building fuzzers (ENABLE_FUZZER) Typo: s/introduce/introduces/. > and add fuzzers based on LibFuzzer [1] to csv, http_parser and uri modules. > NOTE: LibFuzzer requires Clang compiler. > > [1] https://llvm.org/docs/LibFuzzer.html > > How-To Use: > > $ mkdir build && cd build > $ CC=clang CXX=clang++ cmake -DENABLE_FUZZER=ON -DENABLE_ASAN=ON -DCMAKE_BUILD_TYPE=Debug .. > $ make fuzzers > $ ./test/fuzz/csv_fuzzer -max_total_time=60*60*60 -workers=4 ../test/static/corpus/csv I tried your recipe for the current revision and got the following: | $ ./test/fuzz/csv_fuzzer -max_total_time=60*60*60 -workers=4 ../test/static/corpus/csv | INFO: Seed: 2899369680 | INFO: Loaded 1 modules (3 inline 8-bit counters): 3 [0x57a130, 0x57a133), | INFO: Loaded 1 PC tables (3 PCs): 3 [0x553870,0x5538a0), | No such file or directory: ../test/static/corpus/csv; exiting AFAICS, the required directory is added in the following patch, so I checkout the branch HEAD and try once more: | $ ./test/fuzz/csv_fuzzer -max_total_time=60*60*60 -workers=4 ../test/static/corpus/csv | INFO: Seed: 1838565059 | INFO: Loaded 1 modules (3 inline 8-bit counters): 3 [0x57a130, 0x57a133), | INFO: Loaded 1 PC tables (3 PCs): 3 [0x553870,0x5538a0), | INFO: 21 files found in ../test/static/corpus/csv | INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes | INFO: seed corpus: files: 21 min: 1b max: 462b total: 1336b rss: 27Mb | csv_fuzzer: /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerLoop.cpp:537: void fuzzer::Fuzzer::ExecuteCallback(const uint8_t *, size_t): Assertion `Res == 0' failed. | ==15230== ERROR: libFuzzer: deadly signal | #0 0x507287 in __sanitizer_print_stack_trace /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/asan/asan_stack.cc:38:3 | #1 0x44f978 in fuzzer::PrintStackTrace() /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerUtil.cpp:206:5 | #2 0x4300f3 in fuzzer::Fuzzer::CrashCallback() /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerLoop.cpp:237:3 | #3 0x4300b0 in fuzzer::Fuzzer::StaticCrashSignalCallback() /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerLoop.cpp:209:6 | #4 0x7f179300c8bf (/lib64/libpthread.so.0+0x148bf) | #5 0x7f1792bfdf3a in gsignal (/lib64/libc.so.6+0x38f3a) | #6 0x7f1792be7534 in abort (/lib64/libc.so.6+0x22534) | #7 0x7f1792be740e in __tls_get_addr (/lib64/libc.so.6+0x2240e) | #8 0x7f1792bf5731 in __assert_fail (/lib64/libc.so.6+0x30731) | #9 0x431d06 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerLoop.cpp:537:5 | #10 0x4310d5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerLoop.cpp:455:3 | #11 0x433aad in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector, std::allocator >, fuzzer::fuzzer_allocator, std::allocator > > > const&) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerLoop.cpp:745:7 | #12 0x434240 in fuzzer::Fuzzer::Loop(std::vector, std::allocator >, fuzzer::fuzzer_allocator, std::allocator > > > const&) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerLoop.cpp:768:3 | #13 0x425e60 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerDriver.cpp:760:6 | #14 0x450132 in main /var/tmp/portage/sys-libs/compiler-rt-sanitizers-8.0.1/work/compiler-rt-8.0.1.src/lib/fuzzer/FuzzerMain.cpp:20:10 | #15 0x7f1792be8eda in __libc_start_main (/lib64/libc.so.6+0x23eda) | #16 0x41e919 in _start (/tarantool/build/test/fuzz/csv_fuzzer+0x41e919) | | NOTE: libFuzzer has rudimentary signal handlers. | Combine libFuzzer with AddressSanitizer or similar for better crash reports. | SUMMARY: libFuzzer: deadly signal | MS: 0 ; base unit: 0000000000000000000000000000000000000000 | 0x22,0x61,0x62,0x63,0x22,0x2c,0x20,0x22,0x77,0x69,0x74,0x68,0x2c,0x63,0x6f,0x6d,0x6d,0x61,0x22,0x2c,0x20,0x22,0x5c,0x22,0x69,0x6e,0x20,0x71,0x75,0x6f,0x74,0x65,0x73,0x5c,0x22,0x22,0x2c,0x20,0x22,0x31,0x20,0x5c,0x22,0x20,0x71,0x75,0x6f,0x74,0x65,0x22,0xa, \"abc\", \"with,comma\", \"\\\"in quotes\\\"\", \"1 \\\" quote\"\x0a | artifact_prefix='./'; Test unit written to ./crash-6d131d28c6e20c3a0a0b46c3aa7308d3029ab636 | Base64: ImFiYyIsICJ3aXRoLGNvbW1hIiwgIlwiaW4gcXVvdGVzXCIiLCAiMSBcIiBxdW90ZSIK I have no idea whether it is OK but this does look like it's not. Maybe there are some problems with my compiler/sanitizer? JFYI, the toolchain is the following: | $ clang -v | clang version 8.0.1 (tags/RELEASE_801/final) | Target: x86_64-pc-linux-gnu | Thread model: posix | InstalledDir: /usr/lib/llvm/8/bin | Selected GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0 | Candidate multilib: .;@m64 | Candidate multilib: 32;@m32 | Selected multilib: .;@m64 | $ clang++ -v | clang version 8.0.1 (tags/RELEASE_801/final) | Target: x86_64-pc-linux-gnu | Thread model: posix | InstalledDir: /usr/lib/llvm/8/bin | Selected GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0 | Candidate multilib: .;@m64 | Candidate multilib: 32;@m32 | Selected multilib: .;@m64 > > Part of #1809 > --- > CMakeLists.txt | 2 +- > cmake/profile.cmake | 13 ++++++++++ > test/CMakeLists.txt | 3 +++ > test/fuzz/CMakeLists.txt | 45 ++++++++++++++++++++++++++++++++++ > test/fuzz/csv_fuzzer.c | 23 +++++++++++++++++ > test/fuzz/http_parser_fuzzer.c | 18 ++++++++++++++ > test/fuzz/uri_fuzzer.c | 19 ++++++++++++++ > 7 files changed, 122 insertions(+), 1 deletion(-) > create mode 100644 test/fuzz/CMakeLists.txt > create mode 100644 test/fuzz/csv_fuzzer.c > create mode 100644 test/fuzz/http_parser_fuzzer.c > create mode 100644 test/fuzz/uri_fuzzer.c > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 10882c6a1..d20a4eb5d 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -75,6 +75,9 @@ add_subdirectory(app-tap) > add_subdirectory(box) > add_subdirectory(box-tap) > add_subdirectory(unit) > +if(ENABLE_FUZZER) > + add_subdirectory(fuzz) > +endif() Minor: Well, I don't get the idea *why* this change is added here: it neither takes the place in alphabetical order, nor is added to the end of the list. > add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test > ${PROJECT_BINARY_DIR}/third_party/luajit/test) > > diff --git a/test/fuzz/CMakeLists.txt b/test/fuzz/CMakeLists.txt > new file mode 100644 > index 000000000..142d38f67 > --- /dev/null > +++ b/test/fuzz/CMakeLists.txt > @@ -0,0 +1,45 @@ > +include_directories(${PROJECT_SOURCE_DIR}/src) > +include_directories(${PROJECT_BINARY_DIR}/src) Minor: It would be nice to explicitly mention the line above is added for autogenerated headers and LuaJIT ones. Feel free to ignore. > +include_directories(${PROJECT_SOURCE_DIR}/src/box) > + > +# A special target with fuzzer and sanitizer flags. > +add_library(fuzzer_config INTERFACE) > + > +target_compile_options( > + fuzzer_config > + INTERFACE > + $<$: > + -fsanitize=fuzzer,address > + > > + $<$: > + -fsanitize=fuzzer,undefined > + > > +) > +target_link_libraries( > + fuzzer_config > + INTERFACE > + $<$: > + -fsanitize=fuzzer,address > + > > + $<$: > + -fsanitize=fuzzer,undefined > + > > +) OK, I ran with more verbose output and have two notes regarding it. | Scanning dependencies of target csv | make[3]: Leaving directory '/tarantool/build' | make -f src/lib/csv/CMakeFiles/csv.dir/build.make src/lib/csv/CMakeFiles/csv.dir/build | make[3]: Entering directory '/tarantool/build' | [ 0%] Building C object src/lib/csv/CMakeFiles/csv.dir/csv.c.o | cd /tarantool/build/src/lib/csv && /usr/lib/llvm/9/bin/clang-9 -DCORO_ASM | -DLUAJIT_SMART_STRINGS=1 -DLUAJIT_USE_ASAN=1 -DLUA_USE_APICHECK=1 | -DLUA_USE_ASSERT=1 -DNVALGRIND=1 -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE | -D__STDC_CONSTANT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D__STDC_LIMIT_MACROS=1 | -I/tarantool/src -I/tarantool/build/src -I/tarantool/src/lib | -I/tarantool/src/lib/small -I/tarantool/src/lib/small/third_party | -I/tarantool/src/lib/core -I/tarantool -I/tarantool/third_party/zstd/lib | -I/tarantool/third_party/zstd/lib/common -I/tarantool/build/third_party | -I/tarantool/third_party -I/tarantool/third_party/coro | -I/tarantool/third_party/luajit/src -I/tarantool/third_party/libyaml/include | -I/tarantool/src/lib/msgpuck -I/tarantool/build/build/curl/dest/include | -I/tarantool/build/third_party/decNumber | -I/tarantool/third_party/libutil_freebsd -fexceptions -funwind-tables | -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2 | -fsanitize=address -fsanitize-blacklist=/tarantool/asan/asan.supp | -std=c11 -Wall -Wextra -Wno-strict-aliasing -Wno-char-subscripts | -Wno-gnu-alignof-expression -Werror -g -ggdb -O0 -UASAN_INTERFACE_OLD | -o CMakeFiles/csv.dir/csv.c.o -c /tarantool/src/lib/csv/csv.c | [ 0%] Linking C static library libcsv.a | cd /tarantool/build/src/lib/csv && /usr/bin/cmake -P CMakeFiles/csv.dir/cmake_clean_target.cmake | cd /tarantool/build/src/lib/csv && /usr/bin/cmake -E cmake_link_script CMakeFiles/csv.dir/link.txt --verbose=1 | /usr/bin/ar qc libcsv.a CMakeFiles/csv.dir/csv.c.o | /usr/bin/ranlib libcsv.a | make[3]: Leaving directory '/tarantool/build' | [ 0%] Built target csv | make -f test/fuzz/CMakeFiles/csv_fuzzer.dir/build.make test/fuzz/CMakeFiles/csv_fuzzer.dir/depend | make[3]: Entering directory '/tarantool/build' | cd /tarantool/build && /usr/bin/cmake -E cmake_depends "Unix Makefiles" | /tarantool /tarantool/test/fuzz /tarantool/build /tarantool/build/test/fuzz | /tarantool/build/test/fuzz/CMakeFiles/csv_fuzzer.dir/DependInfo.cmake --color= | Dependee "/tarantool/build/test/fuzz/CMakeFiles/csv_fuzzer.dir/DependInfo.cmake" | is newer than depender "/tarantool/build/test/fuzz/CMakeFiles/csv_fuzzer.dir/depend.internal". | Dependee "/tarantool/build/test/fuzz/CMakeFiles/CMakeDirectoryInformation.cmake" | is newer than depender "/tarantool/build/test/fuzz/CMakeFiles/csv_fuzzer.dir/depend.internal". | Scanning dependencies of target csv_fuzzer | make[3]: Leaving directory '/tarantool/build' | make -f test/fuzz/CMakeFiles/csv_fuzzer.dir/build.make test/fuzz/CMakeFiles/csv_fuzzer.dir/build | make[3]: Entering directory '/tarantool/build' | [ 0%] Building C object test/fuzz/CMakeFiles/csv_fuzzer.dir/csv_fuzzer.c.o | cd /tarantool/build/test/fuzz && /usr/lib/llvm/9/bin/clang-9 -DCORO_ASM | -DLUAJIT_SMART_STRINGS=1 -DLUAJIT_USE_ASAN=1 -DLUA_USE_APICHECK=1 | -DLUA_USE_ASSERT=1 -DNVALGRIND=1 -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE | -D__STDC_CONSTANT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D__STDC_LIMIT_MACROS=1 | -I/tarantool/src -I/tarantool/build/src -I/tarantool/src/lib | -I/tarantool/src/lib/small -I/tarantool/src/lib/small/third_party | -I/tarantool/src/lib/core -I/tarantool -I/tarantool/third_party/zstd/lib | -I/tarantool/third_party/zstd/lib/common -I/tarantool/third_party/luajit/src | -I/tarantool/src/lib/msgpuck -I/tarantool/src/box -fexceptions | -funwind-tables -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2 | -fsanitize=address -fsanitize-blacklist=/tarantool/asan/asan.supp | -std=c11 -Wall -Wextra -Wno-strict-aliasing -Wno-char-subscripts | -Wno-gnu-alignof-expression -Werror -Wno-unused-parameter -g -ggdb -O0 | -UASAN_INTERFACE_OLD -fsanitize=fuzzer,address | -o CMakeFiles/csv_fuzzer.dir/csv_fuzzer.c.o -c /tarantool/test/fuzz/csv_fuzzer.c | [ 0%] Linking C executable csv_fuzzer | cd /tarantool/build/test/fuzz && /usr/bin/cmake -E cmake_link_script CMakeFiles/csv_fuzzer.dir/link.txt --verbose=1 | /usr/lib/llvm/9/bin/clang-9 -fexceptions -funwind-tables | -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2 | -fsanitize=address -fsanitize-blacklist=/tarantool/asan/asan.supp | -std=c11 -Wall -Wextra -Wno-strict-aliasing -Wno-char-subscripts | -Wno-gnu-alignof-expression -Werror -Wno-unused-parameter -g -ggdb -O0 | -rdynamic CMakeFiles/csv_fuzzer.dir/csv_fuzzer.c.o -o csv_fuzzer | ../../src/lib/csv/libcsv.a -fsanitize=fuzzer,address | make[3]: Leaving directory '/tarantool/build' | [ 0%] Built target csv_fuzzer 1. I'm totally not an expert, but quite confused with the fact the libcsv is build w/o flag, but csv_fuzzer is build with it. 2. Do you need to specify
flag once more, when ASAN is enabled? If not the hunk above looks excess, doesn't it? > + > + > +set(fuzzing_binaries csv_fuzzer > + http_parser_fuzzer > + uri_fuzzer) Spaces are used for indentation in CMake-related sources, not tabs. Surprisingly, you made it the right way below. > + > +add_custom_target(fuzzers > + DEPENDS ${fuzzing_binaries} > + COMMENT "Build fuzzers") > diff --git a/test/fuzz/csv_fuzzer.c b/test/fuzz/csv_fuzzer.c > new file mode 100644 > index 000000000..8853d6308 > --- /dev/null > +++ b/test/fuzz/csv_fuzzer.c */me feeling myself like a parrot* Why do you violate our style guides[1] using spaces instead of tabs for indentation? IIRC I've already mentioned it here[2]... Otherwise this hunk looks fine. > @@ -0,0 +1,23 @@ > diff --git a/test/fuzz/http_parser_fuzzer.c b/test/fuzz/http_parser_fuzzer.c > new file mode 100644 > index 000000000..a0aaf6786 > --- /dev/null > +++ b/test/fuzz/http_parser_fuzzer.c Ditto. > @@ -0,0 +1,18 @@ > diff --git a/test/fuzz/uri_fuzzer.c b/test/fuzz/uri_fuzzer.c > new file mode 100644 > index 000000000..8397505bd > --- /dev/null > +++ b/test/fuzz/uri_fuzzer.c Ditto. > @@ -0,0 +1,19 @@ > 2.25.1 > [1]: https://www.tarantool.io/en/doc/latest/dev_guide/c_style_guide/#chapter-1-indentation [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016409.html -- Best regards, IM