Fuzz: extend CConnman tests #28584
Fuzz: extend CConnman tests #28584
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28584. ReviewsSee 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. 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. |
|
Concept ACK |
|
The cpu utilization of this target doesn't look great, i suspect this is due to timeouts/sleeps e.g.: Line 2099 in db7b5df Would be nice to address that (maybe in a follow up). Here is a preliminary coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28584/fuzz.coverage/index.html (I'll update this after fuzzing for longer). Looks good to me as the target alone now has more coverage (by line count) in |
|
Concept ACK |
|
@dergoegge, thanks for looking into this!
How did you measure? I guess it should be possible to mock |
Eyeballing
Sounds good to me. |
|
When I run |
For how long did you run this? The fuzzer needs to first find inputs that trigger
You can let libfuzzer run on multiple cores with either |
Another use-case for that mock: #28635:
|
|
|
|
|
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
|
|
|
|
I believe all issues here have been addressed and this is ready for review. Has a stale ACK from @brunoerg and @jonatack and Concept ACK from @dergoegge. |
There was a problem hiding this comment.
reACK 582d9e3
Just generated this coverage report with some hours of running: https://brunoerg.xyz/bitcoin-core-coverage/28584/coverage_report/
There was a problem hiding this comment.
re-ACK 582d9e3 per git range-diff 1c66023 c97d496 582d9e3
changes since my last review #28584 (review) have been rebases and adding commit cf0a497
`FuzzedSock::Accept()` properly returns a new socket, but it forgot to set the output argument `addr`, like `accept(2)` is expected to. This could lead to reading uninitialized data during testing when we read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family` member. Set `addr` to a fuzzed IPv4 or IPv6 address.
Now that all network calls done by `CConnman::OpenNetworkConnection()` are done via `Sock` they can be redirected (mocked) to `FuzzedSocket` for testing.
* Make the methods of `CThreadInterrupt` virtual and store a pointer to it in `CConnman`, thus making it possible to override with a mocked instance. * Initialize `CConnman::m_interrupt_net` from the constructor, making it possible for callers to supply mocked version. * Introduce `FuzzedThreadInterrupt` and `ConsumeThreadInterrupt()` and use them in `src/test/fuzz/connman.cpp` and `src/test/fuzz/i2p.cpp`. This improves the CPU utilization of the `connman` fuzz test. As a nice side effect, the `std::shared_ptr` used for `CConnman::m_interrupt_net` resolves the possible lifetime issues with it (see the removed comment for that variable).
|
|
Extend
CConnmanfuzz tests to also exercise the methodsOpenNetworkConnection(),CreateNodeFromAcceptedSocket(),InitBinds()andSocketHandler().Previously fuzzin 8000 g those methods would have resulted in real socket functions being called in the operating system which is undesirable during fuzzing. Now that #21878 is complete all those are mocked to a fuzzed socket and a fuzzed DNS resolver (see how
CreateSockandg_dns_lookupare replaced in the first commit).