docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated

brunoerg commented at 1:18 pm on September 24, 2024: contributor

Similar to #28019. The following instruction is outdated and doesn’t work:

 0$ git apply << "EOF"
 1diff --git a/src/compat/compat.h b/src/compat/compat.h
 2index 8195bceaec..cce2b31ff0 100644
 3--- a/src/compat/compat.h
 4+++ b/src/compat/compat.h
 5@@ -90,8 +90,12 @@ typedef char* sockopt_arg_type;
 6 // building with a binutils < 2.36 is subject to this ld bug.
 7 #define MAIN_FUNCTION __declspec(dllexport) int main(int argc, char* argv[])
 8 #else
 9+#ifdef HFND_FUZZING_ENTRY_FUNCTION_CXX
10+#define MAIN_FUNCTION HFND_FUZZING_ENTRY_FUNCTION_CXX(int argc, char* argv[])
11+#else
12 #define MAIN_FUNCTION int main(int argc, char* argv[])
13 #endif
14+#endif
15
16 // Note these both should work with the current usage of poll, but best to be safe
17 // WIN32 poll is broken https://daniel.haxx.se/blog/2012/10/10/wsapoll-is-broken/
18diff --git a/src/net.cpp b/src/net.cpp
19index 7601a6ea84..702d0f56ce 100644
20--- a/src/net.cpp
21+++ b/src/net.cpp
22@@ -727,7 +727,7 @@ int V1TransportDeserializer::readHeader(Span<const uint8_t> msg_bytes)
23     }
24
25     // Check start string, network magic
26-    if (memcmp(hdr.pchMessageStart, m_chain_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) != 0) {
27+    if (false && memcmp(hdr.pchMessageStart, m_chain_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) != 0) { // skip network magic checking
28         LogDebug(BCLog::NET, "Header error: Wrong MessageStart %s received, peer=%d\n", HexStr(hdr.pchMessageStart), m_node_id);
29         return -1;
30     }
31@@ -788,7 +788,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
32     RandAddEvent(ReadLE32(hash.begin()));
33
34     // Check checksum and header message type string
35-    if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
36+    if (false && memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { // skip checksum checking
37         LogDebug(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
38                  SanitizeString(msg.m_type), msg.m_message_size,
39                  HexStr(Span{hash}.first(CMessageHeader::CHECKSUM_SIZE)),
40EOF

maflcko commented at 9:20 am on September 26, 2024: member

Should be trivial to rebase it, no?

 0diff --git a/src/compat/compat.h b/src/compat/compat.h
 1index 366c648ae7..1f3bf2b018 100644
 2--- a/src/compat/compat.h
 3+++ b/src/compat/compat.h
 4@@ -92,8 +92,12 @@ typedef char* sockopt_arg_type;
 5 // building with a binutils < 2.36 is subject to this ld bug.
 6 #define MAIN_FUNCTION __declspec(dllexport) int main(int argc, char* argv[])
 7 #else
 8+#ifdef HFND_FUZZING_ENTRY_FUNCTION_CXX
 9+#define MAIN_FUNCTION HFND_FUZZING_ENTRY_FUNCTION_CXX(int argc, char* argv[])
10+#else
11 #define MAIN_FUNCTION int main(int argc, char* argv[])
12 #endif
13+#endif
14 
15 // Note these both should work with the current usage of poll, but best to be safe
16 // WIN32 poll is broken https://daniel.haxx.se/blog/2012/10/10/wsapoll-is-broken/
17diff --git a/src/net.cpp b/src/net.cpp
18index d87adcc031..7e5f03ac67 100644
19--- a/src/net.cpp
20+++ b/src/net.cpp
21@@ -727,7 +727,7 @@ int V1Transport::readHeader(Span<const uint8_t> msg_bytes)
22     }
23 
24     // Check start string, network magic
25-    if (hdr.pchMessageStart != m_magic_bytes) {
26+    if (false && hdr.pchMessageStart != m_magic_bytes) { // skip network magic checking
27         LogDebug(BCLog::NET, "Header error: Wrong MessageStart %s received, peer=%d\n", HexStr(hdr.pchMessageStart), m_node_id);
28         return -1;
29     }
30@@ -792,7 +792,7 @@ CNetMessage V1Transport::GetReceivedMessage(const std::chrono::microseconds time
31     RandAddEvent(ReadLE32(hash.begin()));
32 
33     // Check checksum and header message type string
34-    if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
35+    if (false && memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { // skip checksum checking
36         LogDebug(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
37                  SanitizeString(msg.m_type), msg.m_message_size,
38                  HexStr(Span{hash}.first(CMessageHeader::CHECKSUM_SIZE)),

dergoegge commented at 9:30 am on September 30, 2024: member

How about using FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for these checks? Then we wouldn’t have to maintain a patch in the docs.

brunoerg commented at 1:20 pm on September 30, 2024: contributor

How about using FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for these checks? Then we wouldn’t have to maintain a patch in the docs.

Sounds good, I can address it if others agree. Just a question, couldn’t it affect other targets that also reaches it?

dergoegge commented at 2:00 pm on September 30, 2024: member

Just a question, couldn’t it affect other targets that also reaches it?

Yes I think so but similar to the PoW check it never makes sense to have this check in the way. The p2p_transport_serialization harness currently has a bunch of extra logic for reaching the unhappy/happy paths of these checks. We can achieve the same with the macro (without having to create valid checksum in the test), e.g.:

0#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
1if (hdr.pchChecksum[0]&1 != 0) {
2#else
3if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
4#endif
5
6...
7}

brunoerg commented at 5:14 pm on September 30, 2024: contributor

Yes I think so but similar to the PoW check it never makes sense to have this check in the way. The p2p_transport_serialization harness currently has a bunch of extra logic for reaching the unhappy/happy paths of these checks. We can achieve the same with the macro (without having to create valid checksum in the test), e.g.:

Looks good to me!

fanquake referenced this in commit 0ca1d1bf69 on Oct 15, 2024

fanquake closed this on Oct 15, 2024

docs: “Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver” is outdated #30957