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

4d81b4d fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read (Vasil Dimov)
b51d75e 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 bitcoin/bitcoin#30211, more specifically:

  bitcoin/bitcoin#30211 (comment)
  bitcoin/bitcoin#30211 (comment)

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

Tree-SHA512: 73b5cb396784652447874998850e45899e8cba49dcd2cc96b2d1f63be78e48201ab88a76cf1c3cb880abac57af07f2c65d673a1021ee1a577d0496c3a4b0c5dd

Author: fanquake

src/test/fuzz/util/net.cpp

@@ -182,54 +182,54 @@ ssize_t FuzzedSock::Recv(void* buf, size_t len, int flags) const
         EWOULDBLOCK,
     };
     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()) {
         const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
         if (r == -1) {
             SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
         }
         return r;
     }
-    std::vector<uint8_t> random_bytes;
-    bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()};
-    if (m_peek_data.has_value()) {
-        // `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`.
-        random_bytes = m_peek_data.value();
+
+    size_t copied_so_far{0};
+
+    if (!m_peek_data.empty()) {
+        // `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) {
-            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 (r == -1) {
-            SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
-        }
-        return r;
+
+    if (copied_so_far == len) {
+        return copied_so_far;
     }
-    // `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
-    // without MSG_PEEK.
-    size_t recv_len{std::min(random_bytes.size(), len)};
-    std::memcpy(buf, random_bytes.data(), recv_len);
-    if (pad_to_len_bytes) {
-        if (len > random_bytes.size()) {
-            std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size());
-        }
-        return len;
+
+    auto new_data = ConsumeRandomLengthByteVector(m_fuzzed_data_provider, len - copied_so_far);
+    if (new_data.empty()) return copied_so_far;
+
+    std::memcpy(reinterpret_cast<uint8_t*>(buf) + copied_so_far, new_data.data(), new_data.size());
+    copied_so_far += new_data.size();
+
+    if ((flags & MSG_PEEK) != 0) {
+        m_peek_data.insert(m_peek_data.end(), new_data.begin(), new_data.end());
     }
-    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

src/test/fuzz/util/net.h

@@ -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
      * `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