Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin <imun@tarantool.org>
To: sergeyb@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 1/4] test: add infrastructure for fuzzing testing and fuzzers
Date: Mon, 7 Dec 2020 20:24:44 +0300	[thread overview]
Message-ID: <20201207172444.GE5396@tarantool.org> (raw)
In-Reply-To: <ce3bb1f82c97a2fa077a41640c6ae9f9bf74a1e4.1606766417.git.sergeyb@tarantool.org>

Sergey,

Thanks for the patch! Please consider the remaining comments below.

On 30.11.20, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> 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::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > 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::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > 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
> 

<snipped>

> 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
> +        $<$<BOOL:${ENABLE_ASAN}>:
> +        -fsanitize=fuzzer,address
> +        >
> +        $<$<BOOL:${ENABLE_UB_SANITIZER}>:
> +        -fsanitize=fuzzer,undefined
> +        >
> +)
> +target_link_libraries(
> +    fuzzer_config
> +    INTERFACE
> +        $<$<BOOL:${ENABLE_ASAN}>:
> +        -fsanitize=fuzzer,address
> +        >
> +        $<$<BOOL:${ENABLE_UB_SANITIZER}>:
> +        -fsanitize=fuzzer,undefined
> +        >
> +)

OK, I ran <make fuzzers> 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 <fuzzer> flag, but csv_fuzzer is build with it.
2. Do you need to specify <address> flag once more, when ASAN is
enabled? If not the hunk above looks excess, doesn't it?

> +

<snipped>

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

<snipped>

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

<snipped>

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

<snipped>

> 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

  reply	other threads:[~2020-12-07 17:24 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-30 20:24 [Tarantool-patches] [PATCH 0/4] Add fuzzing testing sergeyb
2020-11-30 20:24 ` [Tarantool-patches] [PATCH 1/4] test: add infrastructure for fuzzing testing and fuzzers sergeyb
2020-12-07 17:24   ` Igor Munkin [this message]
2020-12-07 19:54     ` Igor Munkin
2020-12-13 18:56     ` Sergey Bronnikov
2020-12-20 13:31       ` Igor Munkin
2020-12-24 10:18         ` Sergey Bronnikov
2020-12-24 13:22           ` Igor Munkin
2020-12-24 17:25             ` Sergey Bronnikov
2020-12-24 17:50               ` Igor Munkin
2020-12-25  7:07                 ` Sergey Bronnikov
2020-12-25  9:02                   ` Igor Munkin
2020-12-25 10:33                     ` Sergey Bronnikov
2020-11-30 20:24 ` [Tarantool-patches] [PATCH 2/4] test: add corpus to be used with fuzzers sergeyb
2020-12-07 17:34   ` Igor Munkin
2020-12-13 18:56     ` Sergey Bronnikov
2020-11-30 20:24 ` [Tarantool-patches] [PATCH 3/4] travis: build tarantool with ENABLE_FUZZER sergeyb
2020-12-07 17:38   ` Igor Munkin
2020-11-30 20:24 ` [Tarantool-patches] [PATCH 4/4] test: integrate with OSS Fuzz sergeyb
2020-12-07 17:42   ` Igor Munkin
2020-12-01 10:54 ` [Tarantool-patches] [PATCH 0/4] Add fuzzing testing Serge Petrenko
2020-12-01 14:41   ` Sergey Bronnikov
2020-12-01 14:45     ` Serge Petrenko
2020-12-07 17:49 ` Igor Munkin
2020-12-25 13:08 ` Igor Munkin
2020-12-25 14:52 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201207172444.GE5396@tarantool.org \
    --to=imun@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 1/4] test: add infrastructure for fuzzing testing and fuzzers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox