highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-11-12 15:09:59 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#22735: [net] Don't return an optional from TransportDeserializer::GetMessage()

Browse Source 
f3e451bebf [net] Replace GetID() with id in TransportDeserializer constructor (Troy Giorshev)
8c96008ab1 [net] Don't return an optional from TransportDeserializer::GetMessage() (Troy Giorshev)

Pull request description:

  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

ACKs for top commit:
  theStack:
    Code-review ACK f3e451bebf
  ryanofsky:
    Code review ACK f3e451bebf. Changes since last review in https://github.com/bitcoin/bitcoin/pull/20364#pullrequestreview-534369904 were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit.

Tree-SHA512: 37de4b25646116e45eba50206e82ed215b0d9942d4847a172c104da4ed76ea4cee29a6fb119f3c34106a9b384263c576cb8671d452965a468f358d4a3fa3c003
This commit is contained in:
MarcoFalke
2021-11-02 13:38:57 +01:00
parent 76886ce11e f3e451bebf
commit 9e3f7dcaa2
3 changed files with 35 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

47
src/net.cpp
View File

@@ -626,25 +626,26 @@ bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
if (m_deserializer->Complete()) { if (m_deserializer->Complete()) {
// decompose a transport agnostic CNetMessage from the deserializer // decompose a transport agnostic CNetMessage from the deserializer
uint32_t out_err_raw_size{0}; bool reject_message{false};
std::optional<CNetMessage> result{m_deserializer->GetMessage(time, out_err_raw_size)}; CNetMessage msg = m_deserializer->GetMessage(time, reject_message);
if (!result) { if (reject_message) {
// Message deserialization failed. Drop the message but don't disconnect the peer. // Message deserialization failed. Drop the message but don't disconnect the peer.
// store the size of the corrupt message // 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; continue;
} }
//store received bytes per message command // Store received bytes per message command
//to prevent a memory DOS, only allow valid commands // to prevent a memory DOS, only allow valid commands
mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command); auto i = mapRecvBytesPerMsgCmd.find(msg.m_command);
if (i == mapRecvBytesPerMsgCmd.end()) if (i == mapRecvBytesPerMsgCmd.end()) {
i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);
}
assert(i != mapRecvBytesPerMsgCmd.end()); 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, // push the message to the process queue,
vRecvMsg.push_back(std::move(*result)); vRecvMsg.push_back(std::move(msg));
complete = true; complete = true;
} }
@@ -718,16 +719,18 @@ const uint256& V1TransportDeserializer::GetMessageHash() const
return data_hash; return data_hash;
} }
std::optional<CNetMessage> 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 // decompose a single CNetMessage from the TransportDeserializer
std::optional<CNetMessage> msg(std::move(vRecv)); CNetMessage msg(std::move(vRecv));
// store command string, time, and sizes // store command string, time, and sizes
msg->m_command = hdr.GetCommand(); msg.m_command = hdr.GetCommand();
msg->m_time = time; msg.m_time = time;
msg->m_message_size = hdr.nMessageSize; msg.m_message_size = hdr.nMessageSize;
msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
uint256 hash = GetMessageHash(); uint256 hash = GetMessageHash();
@@ -737,17 +740,15 @@ std::optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono
// Check checksum and header command string // Check checksum and header command string
if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { 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", 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<uint8_t>(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE)), HexStr(Span<uint8_t>(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE)),
HexStr(hdr.pchChecksum), HexStr(hdr.pchChecksum),
m_node_id); m_node_id);
out_err_raw_size = msg->m_raw_message_size; reject_message = true;
msg = std::nullopt;
} else if (!hdr.IsCommandValid()) { } else if (!hdr.IsCommandValid()) {
LogPrint(BCLog::NET, "Header error: Invalid message type (%s, %u bytes), peer=%d\n", LogPrint(BCLog::NET, "Header error: Invalid message type (%s, %u bytes), peer=%d\n",
SanitizeString(hdr.GetCommand()), msg->m_message_size, m_node_id); SanitizeString(hdr.GetCommand()), msg.m_message_size, m_node_id);
out_err_raw_size = msg->m_raw_message_size; reject_message = true;
msg.reset();
} }
// Always reset the network deserializer (prepare for the next message) // Always reset the network deserializer (prepare for the next message)
@@ -2980,7 +2981,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
LogPrint(BCLog::NET, "Added connection peer=%d\n", id); LogPrint(BCLog::NET, "Added connection peer=%d\n", id);
} }
m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION)); m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION));
m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer()); m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
} }

4
src/net.h
View File

@@ -308,7 +308,7 @@ public:
/** read and deserialize data, advances msg_bytes data pointer */ /** read and deserialize data, advances msg_bytes data pointer */
virtual int Read(Span<const uint8_t>& msg_bytes) = 0; virtual int Read(Span<const uint8_t>& msg_bytes) = 0;
// decomposes a message from the context // decomposes a message from the context
virtual std::optional<CNetMessage> GetMessage(std::chrono::microseconds time, uint32_t& out_err) = 0; virtual CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) = 0;
virtual ~TransportDeserializer() {} virtual ~TransportDeserializer() {}
}; };
@@ -372,7 +372,7 @@ public:
} }
return ret; return ret;
} }
std::optional<CNetMessage> 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 /** The TransportSerializer prepares messages for the network transport

20
src/test/fuzz/p2p_transport_serialization.cpp
View File

@@ -68,18 +68,16 @@ FUZZ_TARGET_INIT(p2p_transport_serialization, initialize_p2p_transport_serializa
} }
if (deserializer.Complete()) { if (deserializer.Complete()) {
const std::chrono::microseconds m_time{std::numeric_limits<int64_t>::max()}; const std::chrono::microseconds m_time{std::numeric_limits<int64_t>::max()};
uint32_t out_err_raw_size{0}; bool reject_message{false};
std::optional<CNetMessage> result{deserializer.GetMessage(m_time, out_err_raw_size)}; CNetMessage msg = deserializer.GetMessage(m_time, reject_message);
if (result) { assert(msg.m_command.size() <= CMessageHeader::COMMAND_SIZE);
assert(result->m_command.size() <= CMessageHeader::COMMAND_SIZE); assert(msg.m_raw_message_size <= mutable_msg_bytes.size());
assert(result->m_raw_message_size <= mutable_msg_bytes.size()); assert(msg.m_raw_message_size == CMessageHeader::HEADER_SIZE + msg.m_message_size);
assert(result->m_raw_message_size == CMessageHeader::HEADER_SIZE + result->m_message_size); assert(msg.m_time == m_time);
assert(result->m_time == m_time);
std::vector<unsigned char> header; std::vector<unsigned char> header;
auto msg = CNetMsgMaker{result->m_recv.GetVersion()}.Make(result->m_command, MakeUCharSpan(result->m_recv)); auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_command, MakeUCharSpan(msg.m_recv));
serializer.prepareForTransport(msg, header); serializer.prepareForTransport(msg2, header);
}
} }
} }
} }