fuzz: add targets for PCP and NAT-PMP port mapping requests #31676

pull darosior wants to merge 6 commits into bitcoin:master from darosior:2501_pcp_fuzzing changing 6 files +111 −6
  1. darosior commented at 8:06 pm on January 16, 2025: member

    Based on #31022, this introduces a fuzz target for PCPRequestPortMap and NATPMPRequestPortMap.

    Like in #31022 we set CreateSock to return a Sock which mocks the responses from the server and uses a mocked steady clock for the Waits. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.

    We reuse the existing FuzzedSock, so a preparatory commit is included that adds steady clock mocking to it. This may be useful for other harnesses as well.

  2. DrahtBot commented at 8:06 pm on January 16, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31676.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK laanwj, marcofleon, dergoegge

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28584 (Fuzz: extend CConnman tests by vasild)

    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.

  3. DrahtBot added the label Tests on Jan 16, 2025
  4. DrahtBot added the label CI failed on Jan 16, 2025
  5. DrahtBot commented at 11:51 pm on January 16, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35738346096

    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.

  6. laanwj requested review from laanwj on Jan 17, 2025
  7. fanquake commented at 11:24 am on January 17, 2025: member

    The ASAN failure is unrelated (#31675).

    https://cirrus-ci.com/task/6569144394448896?logs=ci#L3546:

     0[17:40:31.020] [#29185](/bitcoin-bitcoin/29185/)	NEW    cov: 1486 ft: 2800 corp: 248/5890b lim: 63 exec/s: 2653 rss: 125Mb L: 63/63 MS: 3 ShuffleBytes-InsertRepeatedBytes-InsertRepeatedBytes-
 1[17:40:31.020] /ci_container_base/src/common/pcp.cpp:331:13: runtime error: implicit conversion from type 'uint16_t' (aka 'unsigned short') of value 65535 (16-bit, unsigned) to type 'uint8_t' (aka 'unsigned char') changed the value to 255 (8-bit, unsigned)
 2[17:40:31.020]     [#0](/bitcoin-bitcoin/0/) 0x55b9257e023e in NATPMPRequestPortMap(CNetAddr const&, unsigned short, unsigned int, int, std::chrono::duration<long, std::ratio<1l, 1000l>>) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./common/pcp.cpp:331:13
 3[17:40:31.020]     [#1](/bitcoin-bitcoin/1/) 0x55b9253f3a3c in natpmp_request_port_map_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/./test/fuzz/pcp.cpp:221:20
 4[17:40:31.020]     [#2](/bitcoin-bitcoin/2/) 0x55b92573acbe in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
 5[17:40:31.020]     [#3](/bitcoin-bitcoin/3/) 0x55b92573acbe in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/util/./test/fuzz/fuzz.cpp:85:5
 6[17:40:31.020]     [#4](/bitcoin-bitcoin/4/) 0x55b92573aa27 in LLVMFuzzerTestOneInput /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/util/./test/fuzz/fuzz.cpp:223:5
 7[17:40:31.020]     [#5](/bitcoin-bitcoin/5/) 0x55b924db097a in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ca397a) (BuildId: 50d69ac9373512dbd1c6535b75b2d7bd7d794c1e)
 8[17:40:31.020]     [#6](/bitcoin-bitcoin/6/) 0x55b924daff89 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ca2f89) (BuildId: 50d69ac9373512dbd1c6535b75b2d7bd7d794c1e)
 9[17:40:31.020]     [#7](/bitcoin-bitcoin/7/) 0x55b924db1985 in fuzzer::Fuzzer::MutateAndTestOne() (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ca4985) (BuildId: 50d69ac9373512dbd1c6535b75b2d7bd7d794c1e)
10[17:40:31.020]     [#8](/bitcoin-bitcoin/8/) 0x55b924db2475 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ca5475) (BuildId: 50d69ac9373512dbd1c6535b75b2d7bd7d794c1e)
11[17:40:31.020]     [#9](/bitcoin-bitcoin/9/) 0x55b924d9e925 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c91925) (BuildId: 50d69ac9373512dbd1c6535b75b2d7bd7d794c1e)
12[17:40:31.020]     [#10](/bitcoin-bitcoin/10/) 0x55b924dcada6 in main (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1cbdda6) (BuildId: 50d69ac9373512dbd1c6535b75b2d7bd7d794c1e)
13[17:40:31.020]     [#11](/bitcoin-bitcoin/11/) 0x7fb2218571c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
14[17:40:31.020]     [#12](/bitcoin-bitcoin/12/) 0x7fb22185728a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
15[17:40:31.020]     [#13](/bitcoin-bitcoin/13/) 0x55b924d92da4 in _start (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c85da4) (BuildId: 50d69ac9373512dbd1c6535b75b2d7bd7d794c1e)
16[17:40:31.020] 
17[17:40:31.020] SUMMARY: UndefinedBehaviorSanitizer: implicit-unsigned-integer-truncation /ci_container_base/src/common/pcp.cpp:331:13 
18[17:40:31.020] MS: 3 CrossOver-ShuffleBytes-InsertRepeatedBytes-; base unit: 75aba7b3e37ee7e5d5428a9e36ed92db26b2aa77
19[17:40:31.020] 0x65,0x27,0x27,0xdf,0xd8,0xff,0xff,0xb,0x0,0x80,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x5a,0x29,0xff,0x2e,0xd0,0xff,0xff,0x3b,0xd0,0x0,0x2b,0x30,0x10,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x1,0x40,0x0,0x0,0x0,0x0,0x0,0x5a,
20[17:40:31.020] e''\337\330\377\377\013\000\200\377\377\377\377\377\377\377\377\377\377\377\377\377\377\000\000\000\000\000\000Z)\377.\320\377\377;\320\000+0\020\000\000\000\000\000\000\000\000\001@\000\000\000\000\000Z
21[17:40:31.020] artifact_prefix='./'; Test unit written to ./crash-0eaf33f4d4832389ed1ec1517b990f870bb6fd6f
22[17:40:31.020] Base64: ZScn39j//wsAgP//////////////////AAAAAAAAWin/LtD//zvQACswEAAAAAAAAAAAAUAAAAAAAFo=

    https://github.com/bitcoin/bitcoin/actions/runs/12816725670/job/35738347958?pr=31676#step:10:491:

    0   package_eval.cpp
1D:\a\bitcoin\bitcoin\src\test\fuzz\pcp.cpp(121,47): error C2220: the following warning is treated as an error [D:\a\bitcoin\bitcoin\build\src\test\fuzz\fuzz.vcxproj]
2D:\a\bitcoin\bitcoin\src\test\fuzz\pcp.cpp(121,47): warning C4305: 'return': truncation from 'int' to 'bool' [D:\a\bitcoin\bitcoin\build\src\test\fuzz\fuzz.vcxproj]
3  parse_hd_keypath.cpp
4  parse_iso8601.cpp
  8. darosior force-pushed on Jan 17, 2025
  9. darosior force-pushed on Jan 17, 2025
  10. darosior commented at 9:56 pm on January 17, 2025: member
    Pushed a new commit to avoid the implicit u16 -> u8 conversion, which for some reason i can’t reproduce (and didn’t hit) locally.
  11. DrahtBot removed the label CI failed on Jan 18, 2025
  12. dergoegge commented at 11:51 am on January 19, 2025: member 
     0
 1$ echo "Aw8DDwAAAABzAAgAAAAAAAH1/9UAAA==" | base64 --decode > pcp_request_port_map.crash
 2$ FUZZ="pcp_request_port_map" ./fuzz ./pcp_request_port_map.crash # MSan or valgrind required to repro
 3==3534==WARNING: MemorySanitizer: use-of-uninitialized-value
 4    [#0](/bitcoin-bitcoin/0/) 0x561cd8bb1125 in PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0::operator()(Span<unsigned char const>) const /workdir/bitcoin/build_fuzz/src/./common/pcp.cpp:470:62
 5    [#1](/bitcoin-bitcoin/1/) 0x561cd8bb1125 in decltype(std::declval<PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0&>()(std::declval<Span<unsigned char const>>())) std::__1::__invoke[abi:de190104]<PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0&, Span<unsigned char const>>(PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0&, Span<unsigned char const>&&) /libcxx_msan/include/c++/v1/__type_traits/invoke.h:149:25
 6    [#2](/bitcoin-bitcoin/2/) 0x561cd8bb1125 in bool std::__1::__invoke_void_return_wrapper<bool, false>::__call[abi:de190104]<PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0&, Span<unsigned char const>>(PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0&, Span<unsigned char const>&&) /libcxx_msan/include/c++/v1/__type_traits/invoke.h:216:12
 7    [#3](/bitcoin-bitcoin/3/) 0x561cd8bb1125 in std::__1::__function::__alloc_func<PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0, std::__1::allocator<PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0>, bool (Span<unsigned char const>)>::operator()[abi:de190104](Span<unsigned char const>&&) /libcxx_msan/include/c++/v1/__functional/function.h:171:12
 8    [#4](/bitcoin-bitcoin/4/) 0x561cd8bb1125 in std::__1::__function::__func<PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0, std::__1::allocator<PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0>, bool (Span<unsigned char const>)>::operator()(Span<unsigned char const>&&) /libcxx_msan/include/c++/v1/__functional/function.h:313:10
 9    [#5](/bitcoin-bitcoin/5/) 0x561cd8ba1719 in std::__1::__function::__value_func<bool (Span<unsigned char const>)>::operator()[abi:de190104](Span<unsigned char const>&&) const /libcxx_msan/include/c++/v1/__functional/function.h:430:12
10    [#6](/bitcoin-bitcoin/6/) 0x561cd8ba1719 in std::__1::function<bool (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /libcxx_msan/include/c++/v1/__functional/function.h:989:10
11    [#7](/bitcoin-bitcoin/7/) 0x561cd8ba1719 in (anonymous namespace)::PCPSendRecv(Sock&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, Span<unsigned char const>, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>, std::__1::function<bool (Span<unsigned char const>)>) /workdir/bitcoin/build_fuzz/src/./common/pcp.cpp:259:17
12    [#8](/bitcoin-bitcoin/8/) 0x561cd8ba5d3b in PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>) /workdir/bitcoin/build_fuzz/src/./common/pcp.cpp:459:21
13    [#9](/bitcoin-bitcoin/9/) 0x561cd86520ed in pcp_request_port_map_fuzz_target(std::__1::span<unsigned char const, 18446744073709551615ul>) /workdir/bitcoin/build_fuzz/src/test/fuzz/./test/fuzz/pcp.cpp:195:20
14    [#10](/bitcoin-bitcoin/10/) 0x561cd7f9e035 in decltype(std::declval<void (*&)(std::__1::span<unsigned char const, 18446744073709551615ul>)>()(std::declval<std::__1::span<unsigned char const, 18446744073709551615ul>>())) std::__1::__invoke[abi:de190104]<void (*&)(std::__1::span<unsigned char const, 18446744073709551615ul>), std::__1::span<unsigned char const, 18446744073709551615ul>>(void (*&)(std::__1::span<unsigned char const, 18446744073709551615ul>), std::__1::span<unsigned char const, 18446744073709551615ul>&&) /libcxx_msan/include/c++/v1/__type_traits/invoke.h:149:25
15    [#11](/bitcoin-bitcoin/11/) 0x561cd7f9e035 in void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:de190104]<void (*&)(std::__1::span<unsigned char const, 18446744073709551615ul>), std::__1::span<unsigned char const, 18446744073709551615ul>>(void (*&)(std::__1::span<unsigned char const, 18446744073709551615ul>), std::__1::span<unsigned char const, 18446744073709551615ul>&&) /libcxx_msan/include/c++/v1/__type_traits/invoke.h:224:5
16    [#12](/bitcoin-bitcoin/12/) 0x561cd7f9e035 in std::__1::__function::__alloc_func<void (*)(std::__1::span<unsigned char const, 18446744073709551615ul>), std::__1::allocator<void (*)(std::__1::span<unsigned char const, 18446744073709551615ul>)>, void (std::__1::span<unsigned char const, 18446744073709551615ul>)>::operator()[abi:de190104](std::__1::span<unsigned char const, 18446744073709551615ul>&&) /libcxx_msan/include/c++/v1/__functional/function.h:171:12
17    [#13](/bitcoin-bitcoin/13/) 0x561cd7f9e035 in std::__1::__function::__func<void (*)(std::__1::span<unsigned char const, 18446744073709551615ul>), std::__1::allocator<void (*)(std::__1::span<unsigned char const, 18446744073709551615ul>)>, void (std::__1::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::__1::span<unsigned char const, 18446744073709551615ul>&&) /libcxx_msan/include/c++/v1/__functional/function.h:313:10
18    [#14](/bitcoin-bitcoin/14/) 0x561cd8ac6394 in std::__1::__function::__value_func<void (std::__1::span<unsigned char const, 18446744073709551615ul>)>::operator()[abi:de190104](std::__1::span<unsigned char const, 18446744073709551615ul>&&) const /libcxx_msan/include/c++/v1/__functional/function.h:430:12
19    [#15](/bitcoin-bitcoin/15/) 0x561cd8ac6394 in std::__1::function<void (std::__1::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::__1::span<unsigned char const, 18446744073709551615ul>) const /libcxx_msan/include/c++/v1/__functional/function.h:989:10
20    [#16](/bitcoin-bitcoin/16/) 0x561cd8ac6394 in test_one_input(std::__1::span<unsigned char const, 18446744073709551615ul>) /workdir/bitcoin/build_fuzz/src/test/fuzz/util/./test/fuzz/fuzz.cpp:85:5
21    [#17](/bitcoin-bitcoin/17/) 0x561cd8ac6394 in LLVMFuzzerTestOneInput /workdir/bitcoin/build_fuzz/src/test/fuzz/util/./test/fuzz/fuzz.cpp:223:5
22    [#18](/bitcoin-bitcoin/18/) 0x561cd7e87d56 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
23    [#19](/bitcoin-bitcoin/19/) 0x561cd7e715f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:328:6
24    [#20](/bitcoin-bitcoin/20/) 0x561cd7e7750f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:863:9
25    [#21](/bitcoin-bitcoin/21/) 0x561cd7ea3932 in main /llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
26    [#22](/bitcoin-bitcoin/22/) 0x7f4fe2319d67  (/lib/x86_64-linux-gnu/libc.so.6+0x29d67) (BuildId: 3bc74dbb72522bb47e0d899e5615140b044a5b40)
27    [#23](/bitcoin-bitcoin/23/) 0x7f4fe2319e24 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e24) (BuildId: 3bc74dbb72522bb47e0d899e5615140b044a5b40)
28    [#24](/bitcoin-bitcoin/24/) 0x561cd7e69640 in _start (/workdir/out/libfuzzer_msan/fuzz+0xf18640)
29
30  Uninitialized value was created by an allocation of 'response' in the stack frame
31    [#0](/bitcoin-bitcoin/0/) 0x561cd8ba0dc2 in (anonymous namespace)::PCPSendRecv(Sock&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, Span<unsigned char const>, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>, std::__1::function<bool (Span<unsigned char const>)>) /workdir/bitcoin/build_fuzz/src/./common/pcp.cpp:224:5
32
33SUMMARY: MemorySanitizer: use-of-uninitialized-value /workdir/bitcoin/build_fuzz/src/./common/pcp.cpp:470:62 in PCPRequestPortMap(std::__1::array<unsigned char, 12ul> const&, CNetAddr const&, CNetAddr const&, unsigned short, unsigned int, int, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l>>)::$_0::operator()(Span<unsigned char const>) const
  13. laanwj commented at 10:49 am on January 20, 2025: member

    Code review ACK. The extra commits “pcp: make the ToString method const” and “pcp: make NAT-PMP error codes uint16_t” also LGTM.

    Have tried to investigate what is happening in @dergoegge ’s case but was unable to find the issue, how PCPSendRecv can return a response while the buffer is uninitialized. Would be interesting if this already found its first bug.

  14. in src/test/fuzz/pcp.cpp:77 in 33a4dc294a outdated 
    72+    {
73+        if (m_fuzz_data.ConsumeBool()) return -1;
74+        const auto consumed_bytes{m_fuzz_data.ConsumeIntegralInRange<size_t>(0, len)};
75+        const auto data{m_fuzz_data.ConsumeBytes<uint8_t>(consumed_bytes)};
76+        if (!data.empty()) std::memcpy(buf, data.data(), data.size());
77+        return consumed_bytes;
    dergoegge commented at 9:34 am on January 21, 2025:

    Here’s my theory for the MSan failure:

    data might hold less bytes than consumed_bytes indicated because the fuzzed data provider might not have enough bytes left. This means only data.size() bytes of buf will be initialized with the memcpy but consumed_bytes is returned as the number of bytes “received”.

    This should work:

    0        const auto data{ConsumeRandomLengthByteVector(fuzzed_data_provider, len)};
1        if (!data.empty()) std::memcpy(buf, data.data(), data.size());
2        return data.size();

    Or alternatively, is there a reason not use FuzzedSock?

    darosior commented at 2:59 pm on January 21, 2025:
    Mostly to mock the clock in Wait iirc. I’ll reproduce your fuzz finding now and look into this. Thanks!
    dergoegge commented at 3:03 pm on January 21, 2025:
    I see, perhaps you can just override Wait of FuzzedSock then? Or the time mocking could also just be a part of FuzzedSock (seems useful for all harnesses).
    darosior commented at 11:19 pm on January 21, 2025:
    Yeah, looks like i just reinvented (a poor man’s) FuzzedSock for my target. I’ll look into just using FuzzedSock instead tomorrow. Just confirmed your theory, returning data.size() instead of consumed_bytes fixed the crash.
    darosior commented at 8:54 pm on January 22, 2025:
    Done. It seems to have reduced the coverage somewhat. I’ll run it overnight again.
  15. darosior force-pushed on Jan 21, 2025
  16. darosior force-pushed on Jan 22, 2025
  17. darosior force-pushed on Jan 22, 2025
  18. DrahtBot added the label CI failed on Jan 22, 2025
  19. DrahtBot commented at 7:49 pm on January 22, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36015736208

    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.

  20. DrahtBot removed the label CI failed on Jan 22, 2025
  21. darosior commented at 5:07 pm on January 23, 2025: member
    Ok so the change to FuzzedSock seems to have made some parts unreachable for some reason. I’ll look into this, eventually…
  22. in src/test/fuzz/pcp.cpp:35 in 0262565f71 outdated 
    30+{
31+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
32+
33+    // Create a mocked socket between random (and potentially invalid) client and gateway addresses.
34+    CreateSock = [&](int domain, int type, int protocol) {
35+        if (domain == AF_INET && type == SOCK_DGRAM && protocol == IPPROTO_UDP) {
    marcofleon commented at 6:02 pm on February 6, 2025:
    Should the PCP target include IPv6 or no? Something like (domain == AF_INET || domain == AF_INET6) might improve the test.
  23. in src/test/fuzz/pcp.cpp:70 in 0262565f71 outdated 
    65+        return std::unique_ptr<FuzzedSock>();
66+    };
67+
68+    // Perform the port mapping request. The mocked socket will return fuzzer-provided data.
69+    const auto gateway_addr{ConsumeNetAddr(fuzzed_data_provider)};
70+    const auto local_addr{ConsumeNetAddr(fuzzed_data_provider)};
    marcofleon commented at 6:04 pm on February 6, 2025:
    I don’t see this used anywhere.
    darosior commented at 3:41 pm on February 12, 2025:
    Leftover from my Sock implem.
  24. in src/test/fuzz/util/net.cpp:362 in 0262565f71 outdated 
    356@@ -349,7 +357,9 @@ int FuzzedSock::GetSockName(sockaddr* name, socklen_t* name_len) const
357         SetFuzzedErrNo(m_fuzzed_data_provider, getsockname_errnos);
358         return -1;
359     }
360+    assert(name_len);
361     *name_len = m_fuzzed_data_provider.ConsumeData(name, *name_len);
362+    if (*name_len < (int)sizeof(sockaddr)) return -1;
    marcofleon commented at 6:21 pm on February 6, 2025:

    The ConsumeData here was the culprit for the bad coverage.

    Basically, this function needs to set name_len to 16 or 28 for the fuzzer to continue past SetSockAddr in PCPRequestPortMap. In the PCP case, name_len started here as 128 and then ConsumeData would look to return the min of 128 or the remaining bytes of the input. It would consume the rest of the input every time because the fuzzer was optimizing for the SetSockAddr check. So, there would always be nothing left in fuzzed_data_provider when Send was called later on.

    I think something like this in place of the ConsumeData would fix it and achieve the behavior we’re looking for:

    0auto bytes = ConsumeRandomLengthByteVector(m_fuzzed_data_provider, *name_len);
1std::memcpy(name, bytes.data(), bytes.size());
2*name_len = bytes.size();
    darosior commented at 4:20 pm on February 12, 2025:
    Good catch. Seems like this could also be useful for other harnesses.
  25. marcofleon commented at 6:24 pm on February 6, 2025: contributor

    I ran pcp_request_port_map with the patch I discuss below and looks like coverage is good to go now.

    https://marcofleon.github.io/coverage/pcp_request_port_map/coverage/root/bitcoin/src/common/pcp.cpp.html

  26. pcp: make the ToString method const 01906ce912
  27. pcp: make NAT-PMP error codes uint16_t 
    They are defined as being 16 bits in the RFC and correctly parsed in the code
which may result in an implicit conversion from uint16_t to uint8_t.
    6fe1c35c05
  28. fuzz: add steady clock mocking to FuzzedSock 39b7e2b590
  29. fuzz: never return an uninitialized sockaddr in FuzzedSock::GetSockName 
    The fuzz provider's `ConsumeData` may return less data than necessary
to fill the sockaddr struct and still return success. Fix this to avoid
the caller using uninitialized memory.
    0d472c1953
  30. fuzz: in FuzzedSock::GetSockName(), return a random-length name 
    ConsumeData() will always try to return a name as long as the requested size. It is more useful, and
closer to how `getsockname` would actually behave in reality, to return a random length name
instead.

This was hindering coverage in the PCP fuzz target as the addr len was set to the size of the
sockaddr_in struct and would exhaust all the provided data from the fuzzer.

Thanks to Marco Fleon for suggesting this.

Co-Authored-by: marcofleon <marleo23@proton.me>
    1695c8ab5b
  31. fuzz: implement targets for PCP and NAT-PMP port mapping requests c73b59d47f
  32. darosior force-pushed on Feb 12, 2025
  33. laanwj approved
  34. laanwj commented at 7:17 pm on February 12, 2025: member
    re-ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4
  35. darosior commented at 8:01 pm on February 12, 2025: member
    Rebased on master. Thanks @marcofleon for fixing the fuzz target for me, i addressed your comments. This now has pretty good coverage. There seems however to be non-deterministic slowdowns, as i get slow-unit in fuzzing that i cannot reproduce when re-running them.
  36. in src/test/fuzz/util/net.cpp:403 in c73b59d47f 
    399@@ -388,6 +400,7 @@ bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event*
400         // FuzzedDataProvider runs out of data.
401         *occurred = m_fuzzed_data_provider.ConsumeBool() ? 0 : requested;
402     }
403+    ElapseTime(timeout);
    marcofleon commented at 1:49 pm on February 20, 2025:
    To make this more accurate, we could have it advance by the full timeout only when the event fails to occur. And then consume from 0 to timeout for when the event succeeds. Same for WaitMany below.
  37. marcofleon commented at 1:53 pm on February 20, 2025: contributor

    ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4

    I’m not seeing any slow units with the corpuses I have and when I fuzz with afl++ stability looks very good (98-99%). It could be that I have to fuzz longer for it to appear. I think the new fuzz tests are good to go for now. If issues (timeout, etc.) come up, we can address them in a followup?

    Also, left one comment below to improve FuzzedSock a bit but it’s not a blocker.

  38. fanquake requested review from dergoegge on Feb 20, 2025
  39. Sjors commented at 7:54 am on February 21, 2025: member
    Given that #31916 plans to enable PCP for a subset of GUI users (those who had -upnp before), this PR should probably be marked v29.
  40. laanwj added this to the milestone 29.0 on Feb 21, 2025
  41. dergoegge approved
  42. dergoegge commented at 2:50 pm on February 21, 2025: member
    utACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4
  43. fanquake merged this on Feb 21, 2025
  44. fanquake closed this on Feb 21, 2025


darosior DrahtBot fanquake dergoegge laanwj marcofleon Sjors

Labels
Tests

Milestone
29.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-22 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me