fuzz: FuzzedSock::Recv() don’t lose bytes from MSG_PEEK read #30273

1. 
   vasild commented at 2:00 pm on June 12, 2024: contributor

   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 #30211, more specifically:

   #30211 (review) #30211 (review)

2. 
   DrahtBot commented at 2:00 pm on June 12, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	marcofleon, dergoegge, brunoerg

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Jun 12, 2024
4. 
   brunoerg commented at 8:08 pm on June 12, 2024: contributor

   From CI:

    0SUMMARY: UndefinedBehaviorSanitizer: invalid-null-argument test/fuzz/util/net.cpp:219:66 
    1MS: 0 ; base unit: 0000000000000000000000000000000000000000
    20x5c,0x57,0x5c,0x57,0x5c,0xfb,0xff,0x29,0xa5,0x6,0x6,0xff,0x29,0x35,0xda,
    3\\W\\W\\\373\377)\245\006\006\377)5\332
    4artifact_prefix='./'; Test unit written to ./crash-c88fdfee4792d18945ffb11a7a520d9f0fb5e56b
    5Base64: XFdcV1z7/ymlBgb/KTXa
    6
    7Traceback (most recent call last):
    8  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 411, in <module>
    9    main()
   10  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 199, in main
   11    run_once(
   12  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 376, in run_once
   13    assert len(done_stat) == 1
   

5. DrahtBot added the label CI failed on Jun 12, 2024
6. vasild force-pushed on Jun 13, 2024
7. 
   vasild commented at 10:10 am on June 13, 2024: contributor

   3ecd1c632f...f4793257cd: The UB sanitizer did not like the call to memcpy(..., nullptr, 0); which happens when the fuzzer input is exhausted. Added an explicit check about that.
8. DrahtBot removed the label CI failed on Jun 13, 2024
9. fuzz: simplify FuzzedSock::m_peek_data

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

   b51d75ea97
10. fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read

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

    4d81b4de33
11. in src/test/fuzz/util/net.cpp:215 in f4793257cd outdated

    242-        if (len > random_bytes.size()) {
    243-            std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size());
    244-        }
    245-        return len;
    246+
    247+    assert(m_peek_data.empty());
    

    ---

    ---

    

    marcofleon commented at 12:04 pm on June 14, 2024:

    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.

    ---

    

    vasild commented at 12:57 pm on June 14, 2024:

    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.

    ---

    

    marcofleon commented at 1:16 pm on June 14, 2024:

    Nice, I’ll retest.

    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.

    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.

    ---

    

    vasild commented at 4:32 pm on June 14, 2024:

    Did you run it with the I2P dictionary?

    No. How do I do that? Do you have a Base64 of the crash unit?

    ---

    

    marcofleon commented at 10:06 am on June 17, 2024:

    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.
12. vasild force-pushed on Jun 14, 2024
13. 
    vasild commented at 12:57 pm on June 14, 2024: contributor

    f4793257cd...4d81b4de33: rebase and resolve #30273 (review)
14. 
    marcofleon commented at 3:22 pm on June 17, 2024: contributor

    ACK 4d81b4de339efbbb68c9785203b699e6e12ecd83. 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.
15. in src/test/fuzz/util/net.cpp:188 in 4d81b4de33

    181@@ -182,54 +182,54 @@ ssize_t FuzzedSock::Recv(void* buf, size_t len, int flags) const
    182         EWOULDBLOCK,
    183     };
    184     assert(buf != nullptr || len == 0);
    185+
    186+    // Do the latency before any of the "return" statements.
    187+    if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) {
    188+        std::this_thread::sleep_for(std::chrono::milliseconds{2});
    

    ---

    ---

    

    dergoegge commented at 4:01 pm on June 20, 2024:

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

16. dergoegge approved
17. 
    dergoegge commented at 4:02 pm on June 20, 2024: member

    Code review ACK 4d81b4de339efbbb68c9785203b699e6e12ecd83
18. DrahtBot added the label CI failed on Jun 28, 2024
19. 
    dergoegge commented at 9:17 am on July 1, 2024: member

    CI failure looks unrelated to me
20. brunoerg approved
21. 
    brunoerg commented at 9:28 am on July 1, 2024: contributor

    utACK 4d81b4de339efbbb68c9785203b699e6e12ecd83
22. fanquake merged this on Jul 1, 2024
23. fanquake closed this on Jul 1, 2024

    ---

24. vasild deleted the branch on Jul 1, 2024

Contributors
vasild DrahtBot brunoerg marcofleon dergoegge

Labels
Tests CI failed