Super strong concept ACK: very happy to see more people interested in adding fuzzers. Very welcome! :)

Also: the habit of after fixing a security bug also implementing a fuzz target that could have found the bug is something I think we should strive for. I think @kcc (known for making C++ reasonably sane by introducing the sanitizers, libFuzzer, OSS-Fuzz, CFI, etc.) first coined the term "regression fuzzing" for that :)

A good example of "regression fuzzing" was this OOB read in libc++ <random> which not only was fixed but also got a couple of new fuzz targets added: https://bugs.chromium.org/p/chromium/issues/detail?id=994957

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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27491 (refactor: Move chain constants to the util library by TheCharlatan)
• #23352 (test: Extend stale_tip_peer_management test by MarcoFalke)
• #18470 (test: Extend stale tip test by MarcoFalke)

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.

This is kind of an aside, but thought I'd post here since this harness makes use of the ConsumeIntegralInRange function and I'm curious what others think.

I modified FuzzedDataProvider.h to ignore byte 48 so that afl-tmin could minimize better. afl-tmin zeros out redundant data with ascii '0' (http://lcamtuf.coredump.cx/afl/technical_details.txt) - this is done to reduce the number of execs. Since the harness uses range (0-2) and all 256 possible fuzz bytes are mapped to this range, ignoring byte 48 should allow for better minimization if running afl-tmin on a corpus. However, with and without ignoring byte 48, afl-tmin was not able to minimize at all, but logically, it SHOULD have regardless of whether the byte was ignored or not (I can look into this more so that there's more data on this). This is the only harness that used ConsumeIntegralInRange, so I wasn't able to test afl-tmin in a meaningful way on any other harnesses. Thoughts on ignoring byte 48?

Very nice fuzzing harness! Thanks for adding!

Tested ACK 5b90f46 modulo squash of last commit

Compared to the other fuzzers this one is very slow (6 executions/second), but I guess that is largely unavoidable given the code being tested :)

Also, a slow fuzzing harness is better than no fuzzing harness.

Compared to the other fuzzers this one is very slow (6 executions/second), but I guess that is largely unavoidable given the code being tested :)

Good question. This is actually a question in the review club next week: https://bitcoincore.reviews/17860.html

You (and all other reviewers) are invited to join and share your thoughts and ideas.

ACK 8b868f1

I am trying fuzzing utxo_total_supply with AFL and the fuzz is returning crash like this:
[-] PROGRAM ABORT : Test case 'id:000000,orig:f66a2f2925ab9d377ea5c18ba941e7d1601b7509' results in a crash

One possible cause for crash is memory limit as reported by the fuzzer (52MB), and it suggests doing this:
( ulimit -Sv $[51 << 10]; /path/to/binary [...] <testcase )

While setting ulimit at [51<<10] gives a SYSTEM ERROR : Unable to mmap file 'test/fuzz/utxo_total_supply', setting it to [51<<100] (and for higher values) gives the crash back.

I am new to fuzzing, and am not very sure whats exactly happening here. Seems like its crashing for the very first test case itself. Would appreciate some guidance to navigate this.

I am at 8b868f174

@codeShark149 Does it work if you fuzz with libFuzzer instead?

Try this:

$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/utxo_total_supply

@codeShark149 I fixed that error on macOS by turning AFL's forkserver off export AFL_NO_FORKSRV=1. If AFL continually crashes on the initial input, then it's probably because the binary isn't being instrumented correctly.

I've played a bit more with this harness:

I'm currently using a seed corpus of 3247 files. I'm fuzzing at a rate of 6 execs/second which means that it takes almost ten minutes just to initialize libFuzzer with all seeds in a fuzzing session.

A lot of the time is spent setting up (and tearing down) the TestingSetup object for each input.

Can we think of ways to speed up this fuzzing harness?

Perhaps a fully featured TestingSetup object is not needed? Would limiting the setup to only the absolute minimum needed speed up things?

@practicalswift I suspected as much, that the testing harness is the heavy part of the test. On my non-beefy machine it looks like I'm getting 1 a second?

Alternative might be to allow it to run longer to build a longer chain?

Another data point:

As a crude way to quantify the impact of the test setup I made TestingSetup persistent across test inputs (static TestingSetup). That obviously invalidates all assertions (and thus makes the fuzzer meaningless), but the speed jumps to 32 exec/second.

I've played a bit more with this harness:

I'm currently using a seed corpus of 3247 files. I'm fuzzing at a rate of 6 execs/second which means that it takes almost ten minutes just to initialize libFuzzer with all seeds in a fuzzing session.

A lot of the time is spent setting up (and tearing down) the TestingSetup object for each input.

Can we think of ways to speed up this fuzzing harness?

Perhaps a fully featured TestingSetup object is not needed? Would limiting the setup to only the absolute minimum needed speed up things?

You mind posting a profiler view of this? I'll try to optimize this if I can. (VS has an optimization profiler section).

@sanjaykdragon I don't have any profiler view to post :) The quoted numbers are from the output you'll get when running utxo_total_supply with libFuzzer using the following steps

$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
      --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/utxo_total_supply

@codeShark149 Does it work if you fuzz with libFuzzer instead?

@practicalswift yes libfuzzer can fuzz it.

@codeShark149 I fixed that error on macOS by turning AFL's forkserver off export AFL_NO_FORKSRV=1.

@Crypt-iQ thanks for suggesting but for some reason that did'nt work. The problem is simple though. The instrumented binary takes about 103 MB memory to run. So just had to change -m200 while running the fuzzer. Now its fuzzing with AFL.

Some note on execution: libfuzzer reported around 5 exec/sec. AFL reported 3.13/sec. Which is natural given the thing being tested.
Any idea on how to make this thing run faster? I will try parallel but i doubt its gonna complete in any reasonable time.

Tested ACK 8b868f1

Reviewed code and successfully ran with AFL.

Now running since 2 1l2 days, taking > 1 gb of RAM, and it's slowed to a crawl of < 10 executions per minute and still has not found the CVE. It also drains my laptop battery at least twice faster than normal.

#22184	REDUCE cov: 48346 ft: 261959 corp: 1540/3628Kb exec/s: 0 rss: 1020Mb L: 4077/4096 MS: 1 EraseBytes-
#22285	NEW    cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 4096/4096 MS: 1 ChangeBit-
#22296	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 838/4096 MS: 1 EraseBytes-
#22320	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 4007/4096 MS: 4 ChangeBinInt-ChangeByte-ChangeBit-EraseBytes-
#22343	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 1938/4096 MS: 3 InsertRepeatedBytes-ChangeByte-EraseBytes-
#22344	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 169/4096 MS: 1 EraseBytes-
#22370	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 3804/4096 MS: 1 CrossOver-

Maybe I've done something dumb in removing and rebuilding without the CVE fix:

diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
@@ -36,12 +36,6 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
     // Failure to run this check will result in either a crash or an inflation bug, depending on the implementation of
     // the underlying coins database.
-    std::set<COutPoint> vInOutPoints;
-    for (const auto& txin : tx.vin) {
-        if (!vInOutPoints.insert(txin.prevout).second)
-            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
-    }

I'm not sure why the test runs continually slower. Unless my experience is atypical, this seems to be a real handicap.

This also underscores how necessary it is to run the fuzz tests continuously for them to be most effective.

@jonatack The increase in memory usage and slow-down is clearly undesired. Which version of clang are you using? I presume 3.8? Could you try with https://packages.debian.org/stretch-backports/clang-6.0 , or even later, if possible?

Your patch to reintroduce the CVE looks correct.

Concept ACK, will review this week

I can't really see how this actually fuzzes BIP42.

Yeah, it was mostly a joke. Now with LIMITED_WHILE this is no longer possible to fuzz at all.

Finally, I also observed that the fuzzer gets slower with time and after a few hours creates inputs that will take several seconds to run. That seems unavoidable, because the chains that are created are getting longer, and each block must be mined at a small but non-zero difficulty. However, it means that we should probably keep an eye on the qa-corpus seeds once this gets merged.

I guess LIMITED_WHILE can be reduced so much that it can catch the CVE only. For BIP 42 and all other purposes, it might be better to add a new fuzz target for that specifically, maybe using a fuzzed assumeutxo state?