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

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.

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.

DrahtBot added the label Tests on Jan 16, 2025

DrahtBot added the label CI failed on Jan 16, 2025

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.

laanwj requested review from laanwj on Jan 17, 2025

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

darosior force-pushed on Jan 17, 2025

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.

DrahtBot removed the label CI failed on Jan 18, 2025

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

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.

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.

darosior force-pushed on Jan 21, 2025

darosior force-pushed on Jan 22, 2025

DrahtBot added the label CI failed on Jan 22, 2025

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.

DrahtBot removed the label CI failed on Jan 22, 2025

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…

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.

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.

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.

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

pcp: make the ToString method const 01906ce912

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

fuzz: add steady clock mocking to FuzzedSock 39b7e2b590

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

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

fuzz: implement targets for PCP and NAT-PMP port mapping requests c73b59d47f

darosior force-pushed on Feb 12, 2025

laanwj approved

laanwj commented at 7:17 pm on February 12, 2025: member

re-ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4

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.

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.

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.

fanquake requested review from dergoegge on Feb 20, 2025

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.

laanwj added this to the milestone 29.0 on Feb 21, 2025

dergoegge approved

dergoegge commented at 2:50 pm on February 21, 2025: member

utACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4

fanquake merged this on Feb 21, 2025

fanquake closed this on Feb 21, 2025

darosior deleted the branch on Mar 3, 2025

TheCharlatan referenced this in commit 2cab7a09ea on Mar 16, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts