Currently the linker is invoked more than 150 times when compiling with --enable-fuzz. This is problematic for several reasons:

• It wastes disk space north of 20 GB, as all libraries and sanitizers are linked more than 150 times
• It wastes CPU time, as the link step can practically not be cached (similar to ccache for object files)
• It makes it a blocker to compile the fuzz tests by default for non-fuzz builds #19388, for the aforementioned reasons
• The build file is several thousand lines of code, without doing anything meaningful except listing each fuzz target in a highly verbose manner
• It makes writing new fuzz tests unnecessarily hard, as build system knowledge is required; Compare that to boost unit tests, which can be added by simply editing an existing cpp file
• It encourages fuzz tests that re-use the buffer or assume the buffer to be concatenations of seeds, which increases complexity of seeds and complexity for the fuzz engine to explore; Thus reducing the effectiveness of the affected fuzz targets

Fixes #20088

Concept ACK, under the condition that we can do some basic testing that this doesn't meaningfully affect the speed at which fuzzers find issues.

The cost is dereferencing one pointer (to a function). I highly doubt that this affects performance, but I am happy to test.

The cost is dereferencing one pointer (to a function). I highly doubt that this affects performance, but I am happy to test.

That seems entirely reasonable, and is my expectation too. But given the pervasive "one binary per test" recommendation, I'd rather make sure.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20516 (Well-defined CAddress disk serialization, and addrv2 anchors.dat by sipa)
• #20487 (draft: Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
• #20457 (util: Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull} by practicalswift)
• #20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)
• #20377 (fuzz: Fill various small fuzzing gaps by practicalswift)
• #19972 ([draft] fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. by practicalswift)
• #19920 (test: Fuzzing siphash against reference implementation [Request for feedback] by elichai)
• #19415 (net: Make DNS lookup mockable, add fuzzing harness by practicalswift)
• #19288 (fuzz: Add fuzzing harness for TorController by practicalswift)
• #19259 (fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) by practicalswift)
• #19203 (net: Add regression fuzz harness for CVE-2017-18350. Add FuzzedSocket. Add thin SOCKET wrapper. by practicalswift)
• #19055 (Add MuHash3072 implementation by fjahr)

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.

@RandyMcMillan Good catch on macOS. Should be fixed now (hopefully)

<!--4a62be1de6b64f3ed646cdc7932c8cf5-->

🕵️ @achow101 @sipa @practicalswift @harding have been requested to review this pull request as specified in the REVIEWERS file.

I'm very much in favor of this with regard to organization. The large number of binaries was bothering me. That said, I know nothing about fuzzing, so only ACK if it doesn't make fuzzing less useful.

Benchmarks

Compilation

Measured was wall clock time and disk usage of the full build pipeline with a warm ccache: autogen, configure with ccache, make clean, make -j 9.

Running CI

Measured was the time to iterate over all seeds, typically done by CI:

./test/fuzz/test_runner.py -j 9

Coverage

• master: https://marcofalke.github.io/btc_cov/fuzz.coverage/index.html
• this pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/MarcoFalke/bitcoin-core/fac1885e4ceb6224/fuzz.coverage/index.html

Coverage decreased slightly because the tinyformat fuzzer had to be renamed, should recover once the seed dir is renamed as well.

Generating seeds

Measured was the number of iterations required to find a synthetic bug, typically done on a fuzzing farm (dedicated hardware). The bug used:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ec5400c3d8..f15a7a990a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2696,6 +2696,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
                     return;
                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
                     AddTxAnnouncement(pfrom, gtxid, current_time);
+                    Assert(false);
                 }
             } else {
                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());

process_message_inv harness:

	master	this pull
--with-sanitizers=address,fuzzer,undefined -use_value_profile=1
--with-sanitizers=address,fuzzer,undefined -use_value_profile=0
--with-sanitizers=fuzzer -use_value_profile=1
--with-sanitizers=fuzzer -use_value_profile=0	more than 11kk (aborted)	more than 11kk (aborted)

benchmarks updated with images

Doing a benchmark as well.

Error introducing patch (similar to a bug I had during development of #19988):

diff --git a/src/txrequest.cpp b/src/txrequest.cpp
index e54c073328..8a68e4fd8a 100644
--- a/src/txrequest.cpp
+++ b/src/txrequest.cpp
@@ -553,16 +553,17 @@ public:
             //   In other words, the situation where std::next(it) is deleted can only occur if std::next(it)
             //   belongs to a different peer but the same txhash as 'it'. This is covered by the first bulletpoint
             //   already, and we'll have set it_next to end().
-            auto it_next = (std::next(it) == index.end() || std::next(it)->m_peer != peer) ? index.end() :
-                std::next(it);
             // If the announcement isn't already COMPLETED, first make it COMPLETED (which will mark other
             // CANDIDATEs as CANDIDATE_BEST, or delete all of a txhash's announcements if no non-COMPLETED ones are
             // left).
             if (MakeCompleted(m_index.project<ByTxHash>(it))) {
                 // Then actually delete the announcement (unless it was already deleted by MakeCompleted).
-                Erase<ByPeer>(it);
+                it = Erase<ByPeer>(it);
+            } else {
+                it = std::next(it);
             }
-            it = it_next;
         }
     }

Number of iterations until crash is detected (avg +- stddev measured over 250000+ crashes for each):

• 751ffaabad82f7904fd1d9742a0b323a0ab7bfee (master), -use_value_profile=0: 786.8 +- 529.2
• fac1885e4ceb622413b1d4609ba6c860a4af3e74 (this PR), -use_value_profile=0: 812.4 +- 536.1
• 751ffaabad82f7904fd1d9742a0b323a0ab7bfee (master), -use_value_profile=1: 1452.4 +- 1180.3
• fac1885e4ceb622413b1d4609ba6c860a4af3e74 (this PR), -use_value_profile=1: 1362.0 +- 1126.7

Updated my numbers. It appears that there is a (statistically) significant difference in iteration count, but for -use_value_profile=0 it's in the other direction than -use_value_profile=1. So I'm going to assume it's just due to arbitrary alignment changes in the binary or so.

Concept ACK. I'm not concerned anymore about the impact on fuzzing speed. Will review the code changes soon.

@practicalswift Mind taking a look here as well?

@practicalswift Mind taking a look here as well?

I think the addition of an option to link all targets once is a great idea for the reasons you mentioned.

My only concern is that this removes the ability to link the targets separately.

That is problematic since the de facto standard in fuzzing is the one-binary-per-harness mode like we currently use: all fuzzing management platforms I know of assume such structure. They typically don't support setting special environment variables or wrapping binaries in shell scripts that would set such environment variables.

There are two main ways to fuzz:

• Case 1. Quick ad-hoc execution of a single harness: typically performed manually by a developer testing a specific part of the code base or a specific PR
• Case 2. Continuous long term fuzzing of all harnesses in a project: typically performed automatically using a fuzzing platform

This PR is great for case 1 but breaks case 2.

Hopefully we can find a way to make case 1 nicer without breaking case 2?

FWIW the issues I've found and reported to security@ which were found via fuzzing have mostly been found by continuous long term fuzzing (case 2 above).

I am using test/fuzz/test_runner.py to do "case 2" continuous fuzzing and it works just fine. If there is something broken specifically, it would be good to explain how to reproduce the failure and we can take it from there.

To address your feedback, I've written a oneline bash script to link each target individually.

for f in $(git grep --extended-regexp  'FUZZ_TARGET\(.*)$' | sed --regexp-extended 's/.*FUZZ_TARGET.(.*).$/\1/g'); do echo "Compiling target $f ..." && git reset --hard HEAD && sed -i "s/std::getenv(\"FUZZ\")/\"$f\"/g" ./src/test/fuzz/fuzz.cpp && make -j 9 && mv ./src/test/fuzz/fuzz ./src/test/fuzz/$f ; done

To address your feedback, I've written a oneline bash script to link each target individually.

for f in $(git grep --extended-regexp  'FUZZ_TARGET\(.*)$' | sed --regexp-extended 's/.*FUZZ_TARGET.(.*).$/\1/g'); do echo "Compiling target $f ..." && git reset --hard HEAD && sed -i "s/std::getenv(\"FUZZ\")/\"$f\"/g" ./src/test/fuzz/fuzz.cpp && make -j 9 && mv ./src/test/fuzz/fuzz ./src/test/fuzz/$f ; done

That's a really nice idea: thanks! :)

That gives us the desired property that find src/test/fuzz/ -type f -executable finds all fuzzing harnesses (which we depend on in our FuzzBuzz integration as an example: see find_targets_command in https://github.com/bitcoin/bitcoin/blob/master/.fuzzbuzz.yml).

What about making it so that ./configure --enable-fuzz --enable-one-binary-per-fuzzing-harness --with-sanitizers=address,fuzzer,undefined preserves the current behaviour?

If so everyone should be happy regardless of type of fuzzing platform used, and this PR would not break anyones workflow (+/- changing a single configure parameter). We can assume that everyone currently fuzzing Bitcoin Core using a fuzzing platform is using a platform that supports changing configure parameters easily since that is already needed for -enable-fuzz and --with-sanitizers=address,fuzzer,undefined :)

$ gh pr checkout 20560
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ find src/test/fuzz/ -type f -executable
src/test/fuzz/fuzz
$ cat contrib/devtools/build_one_binary_per_fuzzing_harness.sh
#!/bin/bash

set -e
git checkout src/test/fuzz/fuzz.cpp
for FUZZING_HARNESS in $(git grep --extended-regexp 'FUZZ_TARGET\(.*)$' | sed --regexp-extended 's/.*FUZZ_TARGET.(.*).$/\1/g' | sort -u); do
    echo "Building src/test/fuzz/${FUZZING_HARNESS} ..."
    sed -i "s/std::getenv(\"FUZZ\")/\"${FUZZING_HARNESS}\"/g" src/test/fuzz/fuzz.cpp
    make -j "$(nproc)" > /dev/null
    git checkout src/test/fuzz/fuzz.cpp
    mv src/test/fuzz/fuzz "src/test/fuzz/${FUZZING_HARNESS}"
done
echo "Successfully built all targets."
$ contrib/devtools/build_one_binary_per_fuzzing_harness.sh
Building src/test/fuzz/addition_overflow ...
Building src/test/fuzz/addrdb ...
Building src/test/fuzz/asmap ...
Building src/test/fuzz/asmap_direct ...
Building src/test/fuzz/autofile ...
…
Successfully built all targets.
$ find src/test/fuzz/ -type f -executable | sort -u | head -5
src/test/fuzz/addition_overflow
src/test/fuzz/addrdb
src/test/fuzz/asmap
src/test/fuzz/asmap_direct
src/test/fuzz/autofile

Addressed all feedback. Anything left to do here?

Began looking over the code. Concept ACK.

Thanks for adding --enable-danger-fuzz-link-all for backwards compatibility!

Concept ACK

Will review.

Tested ACK fa13e1b0c52738492310b6b421d8e38cb04da5b1

Great work! Thanks! ❤️

$ gh pr checkout 20560
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ find src/test/fuzz/ -type f -executable
src/test/fuzz/fuzz
$ PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 src/test/fuzz/fuzz | head -10
addition_overflow
addr_info_deserialize
addrdb
address_deserialize
addrman
addrman_deserialize
asmap
asmap_direct
autofile
banentry_deserialize
$ FUZZ=asmap src/test/fuzz/fuzz
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1585518257
INFO: Loaded 1 modules   (354516 inline 8-bit counters): 354516 [0x564092304428, 0x56409235acfc),
INFO: Loaded 1 PC tables (354516 PCs): 354516 [0x56409235ad00,0x5640928c3a40),
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
[#2](/github-metadata-backup-bitcoin-bitcoin/2/)      INITED cov: 76 ft: 77 corp: 1/1b exec/s: 0 rss: 96Mb
…
$ make clean
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined --enable-danger-fuzz-link-all
$ make
$ find src/test/fuzz/ -type f -executable | sort -u | head -10
src/test/fuzz/addition_overflow
src/test/fuzz/addrdb
src/test/fuzz/address_deserialize
src/test/fuzz/addr_info_deserialize
src/test/fuzz/addrman
src/test/fuzz/addrman_deserialize
src/test/fuzz/asmap
src/test/fuzz/asmap_direct
src/test/fuzz/autofile
src/test/fuzz/banentry_deserialize
$ src/test/fuzz/asmap
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2730480136
INFO: Loaded 1 modules   (354515 inline 8-bit counters): 354515 [0x555b6b9523e8, 0x555b6b9a8cbb),
INFO: Loaded 1 PC tables (354515 PCs): 354515 [0x555b6b9a8cc0,0x555b6bf119f0),
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
[#2](/github-metadata-backup-bitcoin-bitcoin/2/)      INITED cov: 76 ft: 77 corp: 1/1b exec/s: 0 rss: 96Mb
…

@sipa Mind to re-ACK?

ACK fa13e1b0c52738492310b6b421d8e38cb04da5b1. Reviewed the code changes, and tested the 3 different test_runner.py modes (run once, merge, generate). I also tested building with the new --enable-danger-fuzz-link-all

As a potential follow-up, if there is interesting in building the separate-binary strategy faster it may be useful to instead of modifying the source code in place, make the script create copies of fuzz.cpp, plus an alternative Makefile to build them all. That'd avoid messing with the source tree, and permit building all the target binaries in parallel.

Just for reference (not sure if you noticed during review), one of the targets had to be renamed, which is why I also pushed https://github.com/bitcoin-core/qa-assets/commit/70083a813b9cb1c226d480be25996e5f2567e024

Is it expected behaviour that

./configure --enable-fuzz CC=clang CXX=clang++
make

no longer works, and now requires the --with-sanitizers=address,fuzzer,undefined configure flag to work?

The error I'm seeing during make:

/usr/bin/ld: /usr/bin/ld: DWARF error: could not find variable specification at offset c4
libtest_fuzz.a(libtest_fuzz_a-fuzz.o): in function `unsigned long const& std::max<unsigned long>(unsigned long const&, unsigned long const&)':
/home/dongcarl/src/bitcoin/master/src/./test/fuzz/fuzz.h:18: multiple definition of `FuzzFrameworkEmptyFun()'; test/fuzz/fuzz-addition_overflow.o:/home/dongcarl/src/bitcoin/master/src/./test/fuzz/fuzz.h:18: first defined here

It's late and I could be misremembering but I think that flag was always needed.

@dongcarl I think this is a bug (typo). Functions with a body in a header file need to be inline unless they are member functions, because those are already inline.

So you might be able to fix it by adding inline.