Retropex/bitcoin
1
0
Fork 0
mirror of https://github.com/Retropex/bitcoin.git synced 2025-06-02 15:32:34 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#30273: fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read

Browse Source 
4d81b4de33 fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read (Vasil Dimov)
b51d75ea97 fuzz: simplify FuzzedSock::m_peek_data (Vasil Dimov)

Pull request description:

  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`.

  ---

  This is a followup to https://github.com/bitcoin/bitcoin/pull/30211, more specifically:

  https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633199919
  https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633216366

ACKs for top commit:
  marcofleon:
    ACK 4d81b4de33. 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`.
  dergoegge:
    Code review ACK 4d81b4de33
  brunoerg:
    utACK 4d81b4de33

Tree-SHA512: 73b5cb396784652447874998850e45899e8cba49dcd2cc96b2d1f63be78e48201ab88a76cf1c3cb880abac57af07f2c65d673a1021ee1a577d0496c3a4b0c5dd
This commit is contained in:
merge-script 2024-07-01 11:58:58 +01:00
commit c3b446a494
No known key found for this signature in database
GPG Key ID: 2EEB9F5CC09526C1
2 changed files with 36 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
src/test/fuzz/util/net.cpp
View File

@ -182,6 +182,12 @@ ssize_t FuzzedSock::Recv(void* buf, size_t len, int flags) const
EWOULDBLOCK, EWOULDBLOCK,
}; };
assert(buf != nullptr || len == 0); assert(buf != nullptr || len == 0);
// 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});
}
if (len == 0 || m_fuzzed_data_provider.ConsumeBool()) { if (len == 0 || m_fuzzed_data_provider.ConsumeBool()) {
const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
if (r == -1) { if (r == -1) {
@ -189,47 +195,41 @@ ssize_t FuzzedSock::Recv(void* buf, size_t len, int flags) const
} }
return r; return r;
} }
std::vector<uint8_t> random_bytes;
bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()}; size_t copied_so_far{0};
if (m_peek_data.has_value()) {
// `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`. if (!m_peek_data.empty()) {
random_bytes = m_peek_data.value(); // `MSG_PEEK` was used in the preceding `Recv()` call, copy the first bytes from `m_peek_data`.
const size_t copy_len{std::min(len, m_peek_data.size())};
std::memcpy(buf, m_peek_data.data(), copy_len);
copied_so_far += copy_len;
if ((flags & MSG_PEEK) == 0) { if ((flags & MSG_PEEK) == 0) {
m_peek_data.reset(); m_peek_data.erase(m_peek_data.begin(), m_peek_data.begin() + copy_len);
} }
pad_to_len_bytes = false;
} else if ((flags & MSG_PEEK) != 0) {
// New call with `MSG_PEEK`.
random_bytes = ConsumeRandomLengthByteVector(m_fuzzed_data_provider, len);
if (!random_bytes.empty()) {
m_peek_data = random_bytes;
pad_to_len_bytes = false;
}
} else {
random_bytes = ConsumeRandomLengthByteVector(m_fuzzed_data_provider, len);
} }
if (random_bytes.empty()) {
const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; if (copied_so_far == len) {
if (r == -1) { return copied_so_far;
SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
}
return r;
} }
// `random_bytes` might exceed the size of `buf` if e.g. Recv is called with
// len=N and MSG_PEEK first and afterwards called with len=M (M < N) and auto new_data = ConsumeRandomLengthByteVector(m_fuzzed_data_provider, len - copied_so_far);
// without MSG_PEEK. if (new_data.empty()) return copied_so_far;
size_t recv_len{std::min(random_bytes.size(), len)};
std::memcpy(buf, random_bytes.data(), recv_len); std::memcpy(reinterpret_cast<uint8_t*>(buf) + copied_so_far, new_data.data(), new_data.size());
if (pad_to_len_bytes) { copied_so_far += new_data.size();
if (len > random_bytes.size()) {
std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size()); if ((flags & MSG_PEEK) != 0) {
} m_peek_data.insert(m_peek_data.end(), new_data.begin(), new_data.end());
return len;
} }
if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) {
std::this_thread::sleep_for(std::chrono::milliseconds{2}); if (copied_so_far == len || m_fuzzed_data_provider.ConsumeBool()) {
return copied_so_far;
} }
return recv_len;
// Pad to len bytes.
std::memset(reinterpret_cast<uint8_t*>(buf) + copied_so_far, 0x0, len - copied_so_far);
return len;
} }
int FuzzedSock::Connect(const sockaddr*, socklen_t) const int FuzzedSock::Connect(const sockaddr*, socklen_t) const

2
src/test/fuzz/util/net.h
View File

@ -43,7 +43,7 @@ class FuzzedSock : public Sock
* If `MSG_PEEK` is used, then our `Recv()` returns some random data as usual, but on the next * If `MSG_PEEK` is used, then our `Recv()` returns some random data as usual, but on the next
* `Recv()` call we must return the same data, thus we remember it here. * `Recv()` call we must return the same data, thus we remember it here.
*/ */
mutable std::optional<std::vector<uint8_t>> m_peek_data; mutable std::vector<uint8_t> m_peek_data;
/** /**
* Whether to pretend that the socket is select(2)-able. This is randomly set in the * Whether to pretend that the socket is select(2)-able. This is randomly set in the