tests: Add fuzzing harness for BanMan #19222
Conversation
|
Now fuzzing |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
7261bb3 to
e048d48
Compare
e048d48 to
ad953a2
Compare
| const fs::path banlist_file = GetDataDir() / "fuzzed_banlist.dat"; | ||
| fs::remove(banlist_file); | ||
| { | ||
| BanMan ban_man{banlist_file, nullptr, ConsumeBanTimeOffset(fuzzed_data_provider)}; |
There was a problem hiding this comment.
@practicalswift Do you think adding random data to "fuzzed_banlist.dat", then calling BanMan ban_man{banlist_file, nullptr, ConsumeBanTimeOffset(fuzzed_data_provider)}; could be its own fuzz test? It would basically fuzz DeserializeFileDB in src/addrdb.cpp.
There was a problem hiding this comment.
That is a good idea that can be tackled in a follow-up PR, but instead of writing files to disk I think we should use a mocked/fuzzed filesystem interface like the one that will be introduced as part of #19143. Please consider reviewing that PR :)
ad953a2 to
97846d7
Compare
|
Tested ACK :) Ran against ad953a2 for about 18 hours on a Ubuntu VM with 2 core and 2g RAM. It was pretty slow even for a weak VM, probably because of file io when dumping the banlist. Coverage for |
| #include <primitives/transaction.h> | ||
| #include <script/script.h> | ||
| #include <script/standard.h> | ||
| #include <serialize.h> | ||
| #include <streams.h> | ||
| #include <test/fuzz/FuzzedDataProvider.h> | ||
| #include <test/fuzz/fuzz.h> | ||
| #include <test/util/setup_common.h> | ||
| #include <txmempool.h> | ||
| #include <uint256.h> | ||
| #include <version.h> |
There was a problem hiding this comment.
instead of having a fuzz header which is included in all fuzz test, and itself includes all of bitcoin core, what about moving the implementations to a cpp file or maybe even a fuzz/util/net etc
There was a problem hiding this comment.
Sounds like a plan! :)
|
@Crypt-iQ Thanks a lot for testing and reviewing. Great to have you on board as one of the actively reviewing fuzzing enthusiasts of the project! :) |
97846d7 tests: Add fuzzing harness for BanMan (practicalswift) deba199 tests: Add ConsumeSubNet(...). Move and increase coverage in ConsumeNetAddr(...). (practicalswift) Pull request description: Add fuzzing harness for `BanMan`. See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets). Happy fuzzing :) Top commit has no ACKs. Tree-SHA512: f4126c15bbb77638833367d73f58193c8f05d16bed0b1d6c33b39387d5b610ff34af78cd721adb51778062ce3ac5e79756d1c3895ef54c6c80c61dcf056e94ff
Summary: Backport of core [[bitcoin/bitcoin#19222 | PR19222]]. Test Plan: ninja bitcoin-fuzzers ./src/test/fuzz/banman <path_to_corpus> Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9130
backport: bitcoin#18867, bitcoin#19247, bitcoin#19222, bitcoin#18363, bitcoin#18190, bitcoin#18393, partial bitcoin#18047, bitcoin#18314, bitcoin#19143 (fuzzing harness backports: part 3)
Add fuzzing harness for
BanMan.See
doc/fuzzing.mdfor information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.Happy fuzzing :)