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

issue brunoerg openend this issue on September 24, 2024
  1. 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
    
  2. 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)),
    
  3. 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.
  4. 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?

  5. 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}
    
  6. 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!


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: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me