fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read #30273
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
|
From CI: |
vasild
force-pushed
the
fuzzedsock_unbreak_recv_peek
branch
from
3ecd1c6 to
f479325
Compare
|
|
src/test/fuzz/util/net.cpp
Outdated
There was a problem hiding this comment.
Why would m_peek_data be guaranteed to be empty here? I've tested this with the I2P harness and this assert fails. The m_peek_data doesn't get erased, so there's still some random bytes from the previous Recv call. The length of the copied data is just whatever was left, which isn't equal to len.
There was a problem hiding this comment.
This assert is too strong! Removed and adjusted the code afterwards to expect that m_peek_data may contain some bytes.
Btw, I ran FUZZ=i2p ./src/test/fuzz/fuzz for some time and it did not crash, maybe I did not run it long enough.
There was a problem hiding this comment.
Nice, I'll retest.
Btw, I ran
FUZZ=i2p ./src/test/fuzz/fuzzfor some time and it did not crash, maybe I did not run it long enough.
Interesting, mine crashed very quickly. Did you run it with the I2P dictionary? It's a lot faster using the dict so maybe that's why.
There was a problem hiding this comment.
Did you run it with the I2P dictionary?
No. How do I do that? Do you have a Base64 of the crash unit?
There was a problem hiding this comment.
You can add it to the end of the command. So FUZZ=i2p ./src/test/fuzz/fuzz -dict=DICTFILE. The dictionary (i2p.dict) is in the qa-assets repo: https://github.com/bitcoin-core/qa-assets/tree/main/fuzz_dicts. I'm running it now on the most recent commit.
`FuzzedSock::m_peek_data` need not be an optional of a vector. It can be just a vector whereas an empty vector denotes "no peek data".
Problem: If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be retrieved from the fuzz provider, saved in `m_peek_data` and returned to the caller (ok). If after this `FuzzedSock::Recv(M, 0)` is called where `M < N` then the first `M` bytes from `m_peek_data` would be returned to the caller (ok), but the remaining `N - M` bytes in `m_peek_data` would be discarded/lost (not ok). They must be returned by a subsequent `Recv()`. To resolve this, only remove the head `N` bytes from `m_peek_data`.
vasild
force-pushed
the
fuzzedsock_unbreak_recv_peek
branch
from
f479325 to
4d81b4d
Compare
|
|
marcofleon
left a comment
marcofleon
left a comment
There was a problem hiding this comment.
ACK 4d81b4d. Tested this with the I2P fuzz target and there's no loss in coverage. I think overall this is an improvement in the robustness of Recv in FuzzedSock.
|
|
||
| // Do the latency before any of the "return" statements. | ||
| if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds{2}); |
There was a problem hiding this comment.
Unrelated to this PR but manual timeouts in a fuzz test are a bad idea. If a target requires the use of GetTime (or similar) then the harness needs to set mock time (as the test is otherwise non-deterministic).
We should get rid of this and replace it with calls to SetMockTime (we might need to make mock time more granular, i.e. currently it only supports seconds iirc).
|
CI failure looks unrelated to me |
6 participants
Problem:
If
FuzzedSock::Recv(N, MSG_PEEK)is called thenNbytes would beretrieved from the fuzz provider, saved in
m_peek_dataand returnedto the caller (ok).
If after this
FuzzedSock::Recv(M, 0)is called whereM < Nthen the first
Mbytes fromm_peek_datawould be returnedto the caller (ok), but the remaining
N - Mbytes inm_peek_datawould be discarded/lost (not ok). They must be returned by a subsequent
Recv().To resolve this, only remove the head
Nbytes fromm_peek_data.This is a followup to #30211, more specifically:
#30211 (comment)
#30211 (comment)