From 8c96008ab18075abca03bff6b3675643825a21ca Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Thu, 5 Nov 2020 05:05:32 -0500 Subject: [net] Don't return an optional from TransportDeserializer::GetMessage() Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This throws an error if COMMAND_OTHER doesn't exist, which should never happen. `find()` instead just accessed the last element, which could make debugging more difficult. Resolves review comments from PR19107: - https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478718436 - https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478714497 --- src/net.cpp | 45 ++++++++++++++------------- src/net.h | 4 +-- src/test/fuzz/p2p_transport_serialization.cpp | 20 ++++++------ 3 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index be419648d3..6aa2322e14 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -641,25 +641,26 @@ bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) if (m_deserializer->Complete()) { // decompose a transport agnostic CNetMessage from the deserializer - uint32_t out_err_raw_size{0}; - std::optional result{m_deserializer->GetMessage(time, out_err_raw_size)}; - if (!result) { + bool reject_message{false}; + CNetMessage msg = m_deserializer->GetMessage(time, reject_message); + if (reject_message) { // Message deserialization failed. Drop the message but don't disconnect the peer. // store the size of the corrupt message - mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER)->second += out_err_raw_size; + mapRecvBytesPerMsgCmd.at(NET_MESSAGE_COMMAND_OTHER) += msg.m_raw_message_size; continue; } - //store received bytes per message command - //to prevent a memory DOS, only allow valid commands - mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command); - if (i == mapRecvBytesPerMsgCmd.end()) + // Store received bytes per message command + // to prevent a memory DOS, only allow valid commands + auto i = mapRecvBytesPerMsgCmd.find(msg.m_command); + if (i == mapRecvBytesPerMsgCmd.end()) { i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); + } assert(i != mapRecvBytesPerMsgCmd.end()); - i->second += result->m_raw_message_size; + i->second += msg.m_raw_message_size; // push the message to the process queue, - vRecvMsg.push_back(std::move(*result)); + vRecvMsg.push_back(std::move(msg)); complete = true; } @@ -733,16 +734,18 @@ const uint256& V1TransportDeserializer::GetMessageHash() const return data_hash; } -std::optional V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, uint32_t& out_err_raw_size) +CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, bool& reject_message) { + // Initialize out parameter + reject_message = false; // decompose a single CNetMessage from the TransportDeserializer - std::optional msg(std::move(vRecv)); + CNetMessage msg(std::move(vRecv)); // store command string, time, and sizes - msg->m_command = hdr.GetCommand(); - msg->m_time = time; - msg->m_message_size = hdr.nMessageSize; - msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; + msg.m_command = hdr.GetCommand(); + msg.m_time = time; + msg.m_message_size = hdr.nMessageSize; + msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; uint256 hash = GetMessageHash(); @@ -752,17 +755,15 @@ std::optional V1TransportDeserializer::GetMessage(const std::chrono // Check checksum and header command string if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n", - SanitizeString(msg->m_command), msg->m_message_size, + SanitizeString(msg.m_command), msg.m_message_size, HexStr(Span(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE)), HexStr(hdr.pchChecksum), m_node_id); - out_err_raw_size = msg->m_raw_message_size; - msg = std::nullopt; + reject_message = true; } else if (!hdr.IsCommandValid()) { LogPrint(BCLog::NET, "Header error: Invalid message type (%s, %u bytes), peer=%d\n", - SanitizeString(hdr.GetCommand()), msg->m_message_size, m_node_id); - out_err_raw_size = msg->m_raw_message_size; - msg.reset(); + SanitizeString(hdr.GetCommand()), msg.m_message_size, m_node_id); + reject_message = true; } // Always reset the network deserializer (prepare for the next message) diff --git a/src/net.h b/src/net.h index 28cd635976..1f43433f9f 100644 --- a/src/net.h +++ b/src/net.h @@ -309,7 +309,7 @@ public: /** read and deserialize data, advances msg_bytes data pointer */ virtual int Read(Span& msg_bytes) = 0; // decomposes a message from the context - virtual std::optional GetMessage(std::chrono::microseconds time, uint32_t& out_err) = 0; + virtual CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) = 0; virtual ~TransportDeserializer() {} }; @@ -373,7 +373,7 @@ public: } return ret; } - std::optional GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size) override; + CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) override; }; /** The TransportSerializer prepares messages for the network transport diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index edee5aeef7..29b7223c90 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -68,18 +68,16 @@ FUZZ_TARGET_INIT(p2p_transport_serialization, initialize_p2p_transport_serializa } if (deserializer.Complete()) { const std::chrono::microseconds m_time{std::numeric_limits::max()}; - uint32_t out_err_raw_size{0}; - std::optional result{deserializer.GetMessage(m_time, out_err_raw_size)}; - if (result) { - assert(result->m_command.size() <= CMessageHeader::COMMAND_SIZE); - assert(result->m_raw_message_size <= mutable_msg_bytes.size()); - assert(result->m_raw_message_size == CMessageHeader::HEADER_SIZE + result->m_message_size); - assert(result->m_time == m_time); + bool reject_message{false}; + CNetMessage msg = deserializer.GetMessage(m_time, reject_message); + assert(msg.m_command.size() <= CMessageHeader::COMMAND_SIZE); + assert(msg.m_raw_message_size <= mutable_msg_bytes.size()); + assert(msg.m_raw_message_size == CMessageHeader::HEADER_SIZE + msg.m_message_size); + assert(msg.m_time == m_time); - std::vector header; - auto msg = CNetMsgMaker{result->m_recv.GetVersion()}.Make(result->m_command, MakeUCharSpan(result->m_recv)); - serializer.prepareForTransport(msg, header); - } + std::vector header; + auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_command, MakeUCharSpan(msg.m_recv)); + serializer.prepareForTransport(msg2, header); } } } -- cgit v1.2.3