Skip to content

Commit 9e3f7dc

Browse files
author
MarcoFalke
committed
Merge bitcoin/bitcoin#22735: [net] Don't return an optional from TransportDeserializer::GetMessage()
f3e451b [net] Replace GetID() with id in TransportDeserializer constructor (Troy Giorshev) 8c96008 [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: - bitcoin/bitcoin#19107 (comment) - bitcoin/bitcoin#19107 (comment) ACKs for top commit: theStack: Code-review ACK f3e451b ryanofsky: Code review ACK f3e451b. Changes since last review in bitcoin/bitcoin#20364 (review) were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit. Tree-SHA512: 37de4b25646116e45eba50206e82ed215b0d9942d4847a172c104da4ed76ea4cee29a6fb119f3c34106a9b384263c576cb8671d452965a468f358d4a3fa3c003
2 parents 76886ce + f3e451b commit 9e3f7dc

File tree

3 files changed

+35
-36
lines changed

3 files changed

+35
-36
lines changed

‎src/net.cpp

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -626,25 +626,26 @@ bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
626626

627627
if (m_deserializer->Complete()) {
628628
// decompose a transport agnostic CNetMessage from the deserializer
629-
uint32_t out_err_raw_size{0};
630-
std::optional<CNetMessage> result{m_deserializer->GetMessage(time, out_err_raw_size)};
631-
if (!result) {
629+
bool reject_message{false};
630+
CNetMessage msg = m_deserializer->GetMessage(time, reject_message);
631+
if (reject_message) {
632632
// Message deserialization failed. Drop the message but don't disconnect the peer.
633633
// store the size of the corrupt message
634-
mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER)->second += out_err_raw_size;
634+
mapRecvBytesPerMsgCmd.at(NET_MESSAGE_COMMAND_OTHER) += msg.m_raw_message_size;
635635
continue;
636636
}
637637

638-
//store received bytes per message command
639-
//to prevent a memory DOS, only allow valid commands
640-
mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command);
641-
if (i == mapRecvBytesPerMsgCmd.end())
638+
// Store received bytes per message command
639+
// to prevent a memory DOS, only allow valid commands
640+
auto i = mapRecvBytesPerMsgCmd.find(msg.m_command);
641+
if (i == mapRecvBytesPerMsgCmd.end()) {
642642
i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);
643+
}
643644
assert(i != mapRecvBytesPerMsgCmd.end());
644-
i->second += result->m_raw_message_size;
645+
i->second += msg.m_raw_message_size;
645646

646647
// push the message to the process queue,
647-
vRecvMsg.push_back(std::move(*result));
648+
vRecvMsg.push_back(std::move(msg));
648649

649650
complete = true;
650651
}
@@ -718,16 +719,18 @@ const uint256& V1TransportDeserializer::GetMessageHash() const
718719
return data_hash;
719720
}
720721

721-
std::optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, uint32_t& out_err_raw_size)
722+
CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, bool& reject_message)
722723
{
724+
// Initialize out parameter
725+
reject_message = false;
723726
// decompose a single CNetMessage from the TransportDeserializer
724-
std::optional<CNetMessage> msg(std::move(vRecv));
727+
CNetMessage msg(std::move(vRecv));
725728

726729
// store command string, time, and sizes
727-
msg->m_command = hdr.GetCommand();
728-
msg->m_time = time;
729-
msg->m_message_size = hdr.nMessageSize;
730-
msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
730+
msg.m_command = hdr.GetCommand();
731+
msg.m_time = time;
732+
msg.m_message_size = hdr.nMessageSize;
733+
msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
731734

732735
uint256 hash = GetMessageHash();
733736

@@ -737,17 +740,15 @@ std::optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono
737740
// Check checksum and header command string
738741
if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
739742
LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
740-
SanitizeString(msg->m_command), msg->m_message_size,
743+
SanitizeString(msg.m_command), msg.m_message_size,
741744
HexStr(Span<uint8_t>(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE)),
742745
HexStr(hdr.pchChecksum),
743746
m_node_id);
744-
out_err_raw_size = msg->m_raw_message_size;
745-
msg = std::nullopt;
747+
reject_message = true;
746748
} else if (!hdr.IsCommandValid()) {
747749
LogPrint(BCLog::NET, "Header error: Invalid message type (%s, %u bytes), peer=%d\n",
748-
SanitizeString(hdr.GetCommand()), msg->m_message_size, m_node_id);
749-
out_err_raw_size = msg->m_raw_message_size;
750-
msg.reset();
750+
SanitizeString(hdr.GetCommand()), msg.m_message_size, m_node_id);
751+
reject_message = true;
751752
}
752753

753754
// Always reset the network deserializer (prepare for the next message)
@@ -2980,7 +2981,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
29802981
LogPrint(BCLog::NET, "Added connection peer=%d\n", id);
29812982
}
29822983

2983-
m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION));
2984+
m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION));
29842985
m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
29852986
}
29862987

‎src/net.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ class TransportDeserializer {
308308
/** read and deserialize data, advances msg_bytes data pointer */
309309
virtual int Read(Span<const uint8_t>& msg_bytes) = 0;
310310
// decomposes a message from the context
311-
virtual std::optional<CNetMessage> GetMessage(std::chrono::microseconds time, uint32_t& out_err) = 0;
311+
virtual CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) = 0;
312312
virtual ~TransportDeserializer() {}
313313
};
314314

@@ -372,7 +372,7 @@ class V1TransportDeserializer final : public TransportDeserializer
372372
}
373373
return ret;
374374
}
375-
std::optional<CNetMessage> GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size) override;
375+
CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) override;
376376
};
377377

378378
/** The TransportSerializer prepares messages for the network transport

‎src/test/fuzz/p2p_transport_serialization.cpp

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -68,18 +68,16 @@ FUZZ_TARGET_INIT(p2p_transport_serialization, initialize_p2p_transport_serializa
6868
}
6969
if (deserializer.Complete()) {
7070
const std::chrono::microseconds m_time{std::numeric_limits<int64_t>::max()};
71-
uint32_t out_err_raw_size{0};
72-
std::optional<CNetMessage> result{deserializer.GetMessage(m_time, out_err_raw_size)};
73-
if (result) {
74-
assert(result->m_command.size() <= CMessageHeader::COMMAND_SIZE);
75-
assert(result->m_raw_message_size <= mutable_msg_bytes.size());
76-
assert(result->m_raw_message_size == CMessageHeader::HEADER_SIZE + result->m_message_size);
77-
assert(result->m_time == m_time);
71+
bool reject_message{false};
72+
CNetMessage msg = deserializer.GetMessage(m_time, reject_message);
73+
assert(msg.m_command.size() <= CMessageHeader::COMMAND_SIZE);
74+
assert(msg.m_raw_message_size <= mutable_msg_bytes.size());
75+
assert(msg.m_raw_message_size == CMessageHeader::HEADER_SIZE + msg.m_message_size);
76+
assert(msg.m_time == m_time);
7877

79-
std::vector<unsigned char> header;
80-
auto msg = CNetMsgMaker{result->m_recv.GetVersion()}.Make(result->m_command, MakeUCharSpan(result->m_recv));
81-
serializer.prepareForTransport(msg, header);
82-
}
78+
std::vector<unsigned char> header;
79+
auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_command, MakeUCharSpan(msg.m_recv));
80+
serializer.prepareForTransport(msg2, header);
8381
}
8482
}
8583
}

0 commit comments

Comments
 (0)