test: Build fuzz targets into seperate executables #15043

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!

Would be nice if someone could look at the build system changes, since the code is mostly just moved around.

<!--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:

• #14912 ([WIP] External signer support (e.g. hardware wallet) by Sjors)
• #13989 (add avx512 instrinsic by fingera)
• #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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:

1. Personally, I find it better to allow someone with a large machine to be able to run a simple command and begin fuzzing with minimal effort, if each case is fuzzed separately then this is not really practical.
2. Secondly, I personally am a big fan of maximum fuzz coverage, which means any test that can plausibly make use of randomized data should be accessible from the fuzzer. So clearly I would not want to require people to launch each test separately.
3. It seems like if one would like to fuzz a particular test case, their need can be easily supported by allowing them to pass an argument to the running which causes it to return 0 if the test is specified to anything else, thus the fuzzer will quickly prune attempts which touch other test cases.
4. If there is a large corpus of generated test data, it seems that one could use a simple python script to rename the test files to contain the name of the test case which they're touching, solving the concern about organization.

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

@cjdelisle See @kcc:s comment in #11045 (comment) :-)

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:

• Some of the fuzzers have O(executable_size) performance
• Forcing all fuzzing to enter through one entry-point multiplies the number of possible search paths and fuzzers have limited memory

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

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.

@laanwj Ok that makes sense. I think the corpus should be split up into directories by message type / fuzzing target to avoid erroneous feedback while fuzzing.

Probably want to remove the links to test inputs from doc/fuzzing.md here, as they'll no longer work as-is and one of the links is broken (#15028).

@laanwj The previous corpus can still be used by stripping the first few (4?) bytes off of the seeds. I will do that (and move them to a separate repo) after this pull has been merged.

This hasn't received any review, but I am going to merge it tomorrow unless someone objects to that.

Wouldn't it be easier to accomplish this by having a since source file for all fuzz target, but with a macro define (-D... argument) to select which fuzzer is invoked by main?

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

Ah, I see. Done and removed 400 lines of boilerplate and headers.

Now split into two commits, where the top commit is some move-only:

git diff 2ca632e5b44a8385989c8539cc4e30e60fdee16c~ 2ca632e5b44a8385989c8539cc4e30e60fdee16c --color-moved=dimmed-zebra src/test

utACK 2ca632e5b44a8385989c8539cc4e30e60fdee16c