Currently our fuzzer is a single binary that decides on the first few bits of the buffer what target to pick. This is ineffective as the fuzzer needs to “learn” how the fuzz targets are organized and could get easily confused. Not to mention that the (seed) corpus can not be categorized by target, since targets might “leak” into each other. Also the corpus would potentially become invalid if we ever wanted to remove a target…
Solve that by building each fuzz target into their own executable.
Concept ACK
Thanks for doing this!
11+ CDataStream ds(buffer, SER_NETWORK, INIT_PROTO_VERSION);
12+ try {
13+ int nVersion;
14+ ds >> nVersion;
15+ ds.SetVersion(nVersion);
16+ } catch (const std::ios_base::failure& e) {
e variable. Applies throughout this PR.
0@@ -0,0 +1,25 @@
1+// Copyright (c) 2009-2018 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <primitives/transaction.h>
6+
7+#include <test/test_bitcoin_fuzzy.hpp>
8+
9+static void test_one_input(std::vector<uint8_t> buffer)
buffer const reference. Applies throughout this PR :-)
46+ test_one_input(std::vector<uint8_t>(data, data + size));
47+ return 0;
48+}
49+
50+// This function is used by libFuzzer
51+extern "C" int LLVMFuzzerInitialize(int* argc, char*** argv)
argc and argv.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK
I think this should be behind a configure flag, building so many executables is going to be slow when statically linking, or with slow filesystems, and it contributes nothing to testing unless the user is planning to do fuzzing.
As a non-stakeholder, feel free to ignore, but as someone who is using the same test methodology, I would like to understand why you’re proposing to change.
I have a number of reasons why I tend to prefer a single fuzz entry point, but I would like to know if there is any evidence that putting the test-case in the data can harm the fuzzer’s ability to efficiently find paths.
My reasoning is as follows:
return 0 if the test is specified to anything else, thus the fuzzer will quickly prune attempts which touch other test cases.Thanks, Caleb
Hey @cjdelisle,
I think the fuzz tests should be split up. AFL (and other fuzzers) can splice test cases with one another and for AFL, this can discover 20% additional execution paths. From my understanding, this can cause efficiency problems if the fuzzer is not fuzzing just one target.
If we have two inputs, input A meant for address_deserialize and input B meant for banentry_deserialize and they are spliced together to create input C (which is a nonsense input), input C could be selected to be in the queue for further mutation if it provides the same edge coverage as input A or B (depending on its prefix) and cause even more of an efficiency problem. The splicing mutation is really meant to be used on similar, but diverse test cases.
Also I think seeding the fuzzer with “bad” corpus inputs in general isn’t a good idea because of efficiency and because of the splicing issue, even if we are only running one test and all others are disabled with a flag.
Anyways I’m pretty much a noob to fuzzing, but this is what I could gather from reading the AFL technical_notes, lcamtuf’s blog, and the discussion on https://github.com/bitcoin/bitcoin/issues/11045
Thanks @Crypt-iQ and @practicalswift , for those who weren’t following carefully, my understanding is that it is the recommendation of Google’s OSS-Fuzz project that fuzz targets should be broken up ( https://github.com/google/oss-fuzz/blob/master/docs/ideal_integration.md ) because:
So I hereby reverse my opinion, because any kind of ease of management is negated by the fact that it’s always better to do what works.
I think that the fuzzing targets should be run on the corpus as part of regression testing. This would require the corpus to be included in this project. Is there a reason why it’s not currently included? Maybe this can be done in another PR.
See @kcc comment: #11045 (comment)
OSS Fuzz recommendations: https://github.com/google/oss-fuzz/blob/master/docs/ideal_integration.md#regression-testing
25@@ -26,7 +26,7 @@ To build Bitcoin Core using AFL instrumentation (this assumes that the
26 ./configure --disable-ccache --disable-shared --enable-tests CC=${AFLPATH}/afl-gcc CXX=${AFLPATH}/afl-g++
27 export AFL_HARDEN=1
28 cd src/
29-make test/test_bitcoin_fuzzy
30+make -C src/test
src directory so this will fail. I ran make with no arguments in the src directory and it built the fuzz targets.
make)
I think that the fuzzing targets should be run on the corpus as part of regression testing. This would require the corpus to be included in this project. Is there a reason why it’s not currently included?
The idea is to have a separate repository with the corpus. The problem with including it in the main repository, besides taking up space, is that e.g. all changes have to go through the bottleneck of maintainers here.
doc/fuzzing.md here, as they’ll no longer work as-is and one of the links is broken (#15028).
@sipa Wouldn’t that require to pass in and compile for each fuzz target? That seems to move the verbosity to the user/ci script/fuzz runner.
Also having a single source file makes it harder to see what each single fuzz test does (and depends on).
@MarcoFalke The build system would automatically build all of them, supplying the necessary -D arguments for each binary.
This would avoid all the boilerplate in all those source files.
The return value is always 0 and not used, so might as well return void
Now split into two commits, where the top commit is some move-only:
0git diff 2ca632e5b44a8385989c8539cc4e30e60fdee16c~ 2ca632e5b44a8385989c8539cc4e30e60fdee16c --color-moved=dimmed-zebra src/test
145@@ -147,6 +146,11 @@ AC_ARG_ENABLE([extended-functional-tests],
146 [use_extended_functional_tests=$enableval],
147 [use_extended_functional_tests=no])
148
149+AC_ARG_ENABLE([fuzz],
150+ AS_HELP_STRING([--enable-fuzz],[enable building of fuzz targets (default no)]),
--enable-fuzzing feels more natural to me than --enable-fuzz.
194+if ENABLE_FUZZ
195+test_fuzz_block_deserialize_SOURCES = $(FUZZ_SUITE) test/test_bitcoin_fuzzy.cpp
196+test_fuzz_block_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DBLOCK_DESERIALIZE=1
197+test_fuzz_block_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
198+test_fuzz_block_deserialize_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
199+test_fuzz_block_deserialize_LDADD = \
You could deduplicate some boilerplate by defining common variables like:
0fuzz_common_ldflags = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
1fuzz_common_ldadd = $(LIBUNIVALUE) $(LIBBITCOIN_SERVER) $(LIBBITCOIN_COMMON)
and then individual targets could be shortened to:
0test_fuzz_transaction_deserialize_LDFLAGS = $(fuzz_common_ldflags)
1test_fuzz_transaction_deserialize_LDADD = $(fuzz_common_ldadd)
2...
3
4test_fuzz_block_deserialize_LDFLAGS = $(fuzz_common_ldflags)
5test_fuzz_block_deserialize_LDADD = $(fuzz_common_ldadd)
6...
At least this is what I did in 2060a30063866ed1599134fe28846a69deda773c for #10102. One catch is that LDFLAGS, LDADD, etc suffix can’t be capitalized or the variables will be treated specially by automake.
84 }
85
86- switch(test_id) {
87- case CBLOCK_DESERIALIZE:
88- {
89+#if BLOCK_DESERIALIZE