Extend CConnman fuzz tests to also exercise the methods OpenNetworkConnection(), CreateNodeFromAcceptedSocket(), InitBinds() and SocketHandler().
Previously fuzzing 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 CreateSock and g_dns_lookup are replaced in the first commit).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28584.
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.
Reviewers, 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.
The cpu utilization of this target doesn’t look great, i suspect this is due to timeouts/sleeps e.g.: https://github.com/bitcoin/bitcoin/blob/db7b5dfcc502a8a81c51f56fe753990ae8b3a202/src/net.cpp#L2099
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 net.cpp than our coverage on master with all targets.
@dergoegge, thanks for looking into this!
The cpu utilization of this target doesn’t look great
How did you measure? I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.
How did you measure?
Eyeballing htop this target only achieves about 10% - 30% per core on my machine.
I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.
Sounds good to me.
FUZZ=connman ./src/test/fuzz/fuzz it is single CPU/single threaded and it stays at 98%-100% CPU all the time. It is the same in master. Does it get executed on >1 cores for you?
When I run FUZZ=connman ./src/test/fuzz/fuzz it is single CPU/single threaded and it stays at 98%-100% CPU all the time.
For how long did you run this? The fuzzer needs to first find inputs that trigger sleep_for for you to be able to observe the problem. I have tested this on two machines now and the result is always the same.
Does it get executed on >1 cores for you?
You can let libfuzzer run on multiple cores with either -jobs=<num cpus> or -fork=<num cpus>. I prefer fork since it also includes a minimization step.
20a9fc83bd...cd5bbb12e0: rebase due to conflicts
159@@ -160,6 +160,15 @@ FUZZ_TARGET(connman, .init = initialize_connman)
160 /*strDest=*/fuzzed_data_provider.ConsumeBool() ? nullptr : random_string.c_str(),
161 /*conn_type=*/conn_type,
162 /*use_v2transport=*/fuzzed_data_provider.ConsumeBool());
163+ },
164+ [&] {
165+ connman.SetNetworkActive(true);
100+{
101+ std::vector<CService> ret;
102+ const size_t size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, max_vector_size);
103+ ret.reserve(size);
104+ for (size_t i = 0; i < size; ++i) {
105+ ret.emplace_back(ConsumeNetAddr(fuzzed_data_provider),
In d923b86264df93ad86c7bd557b9970e156585dc7: You could use ConsumeService into ConsumeServiceVector.
0diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h
1index a97017555d..78e61b51d9 100644
2--- a/src/test/fuzz/util/net.h
3+++ b/src/test/fuzz/util/net.h
4@@ -102,8 +102,7 @@ inline std::vector<CService> ConsumeServiceVector(FuzzedDataProvider& fuzzed_dat
5 const size_t size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, max_vector_size);
6 ret.reserve(size);
7 for (size_t i = 0; i < size; ++i) {
8- ret.emplace_back(ConsumeNetAddr(fuzzed_data_provider),
9- fuzzed_data_provider.ConsumeIntegral<uint16_t>());
10+ ret.emplace_back(ConsumeService(fuzzed_data_provider));
11 }
12 return ret;
13 }
cd5bbb12e0...5e3c80da14: address suggestions
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
5e3c80da14...6d9083b249: rebase due to silent merge conflict
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
6d9083b249...45f4dbe484: rebase due to conflicts
45f4dbe484...655a2cf666: the previous push resolved the merge conflict in a too late commit, causing the “test each commit” CI job to fail
655a2cf666...99b1f88fe8: rebase to pick CMake
Now that all network calls done by `CConnman::OpenNetworkConnection()`
are done via `Sock` they can be redirected (mocked) to `FuzzedSocket`
for testing.
99b1f88fe8...cf83f0c14c: rebase due to conflicts and mock the sleeps
I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.
Sounds good to me.
Done. CThreadInterrupt can now be mocked in other tests as well. Thanks for the suggestion, @dergoegge!
14+ return m_fuzzed_data_provider.ConsumeBool();
15+}
16+
17+bool FuzzedThreadInterrupt::sleep_for(Clock::duration)
18+{
19+ return m_fuzzed_data_provider.ConsumeBool();
I like simulating some time passage here.
Changing static std::atomic<std::chrono::seconds> g_mock_time{}; to a finer precision is indeed out of the scope here.
Following a discussion on IRC I checked that indeed the time is frozen already for fuzz tests so calling SetMockTime() here is not going to freeze it (since it is already frozen). I added:
0SetMockTime(ConsumeTime(m_fuzzed_data_provider)); // Time could go backwards.
It could end up increasing the time more than the argument to sleep_for() but this might happen during normal operation, so is a good exercise.
It could end up with the time going backwards. If this is undesirable then this may be changed to something like:
0SetMockTime(NodeClock::now() + ConsumeIntegralInRange(0, the_argument_to_sleep_for * 2));
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32589451018
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
* 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).
cf83f0c...c97d496: simulate time passage from FuzzedThreadInterrupt::sleep_for() and (hopefully) fix CI
