From 6294ecdb8bb4eb7049a18c721ee8cb4a53d80a06 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 13 Jun 2019 10:39:44 +0200 Subject: Refactor: split network transport deserializing from message container --- src/net.cpp | 78 ++++++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 54 insertions(+), 24 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index 63b783382..bd183ce52 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -570,42 +570,42 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete nLastRecv = nTimeMicros / 1000000; nRecvBytes += nBytes; while (nBytes > 0) { - - // get current incomplete message, or create a new one - if (vRecvMsg.empty() || - vRecvMsg.back().complete()) - vRecvMsg.push_back(CNetMessage(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION)); - - CNetMessage& msg = vRecvMsg.back(); - // absorb network data int handled; - if (!msg.in_data) - handled = msg.readHeader(pch, nBytes); + if (!m_deserializer->in_data) + handled = m_deserializer->readHeader(pch, nBytes); else - handled = msg.readData(pch, nBytes); + handled = m_deserializer->readData(pch, nBytes); - if (handled < 0) + if (handled < 0) { + m_deserializer->Reset(); return false; + } - if (msg.in_data && msg.hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { + if (m_deserializer->in_data && m_deserializer->hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { LogPrint(BCLog::NET, "Oversized message from peer=%i, disconnecting\n", GetId()); + m_deserializer->Reset(); return false; } pch += handled; nBytes -= handled; - if (msg.complete()) { + if (m_deserializer->complete()) { + // decompose a transport agnostic CNetMessage from the deserializer + CNetMessage msg = m_deserializer->GetMessage(Params().MessageStart(), nTimeMicros); + //store received bytes per message command //to prevent a memory DOS, only allow valid commands - mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.hdr.pchCommand); + mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(m_deserializer->hdr.pchCommand); if (i == mapRecvBytesPerMsgCmd.end()) i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); assert(i != mapRecvBytesPerMsgCmd.end()); - i->second += msg.hdr.nMessageSize + CMessageHeader::HEADER_SIZE; + i->second += m_deserializer->hdr.nMessageSize + CMessageHeader::HEADER_SIZE; + + // push the message to the process queue, + vRecvMsg.push_back(std::move(msg)); - msg.nTime = nTimeMicros; complete = true; } } @@ -639,8 +639,7 @@ int CNode::GetSendVersion() const return nSendVersion; } - -int CNetMessage::readHeader(const char *pch, unsigned int nBytes) +int TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) { // copy data to temporary parsing buffer unsigned int nRemaining = 24 - nHdrPos; @@ -671,7 +670,7 @@ int CNetMessage::readHeader(const char *pch, unsigned int nBytes) return nCopy; } -int CNetMessage::readData(const char *pch, unsigned int nBytes) +int TransportDeserializer::readData(const char *pch, unsigned int nBytes) { unsigned int nRemaining = hdr.nMessageSize - nDataPos; unsigned int nCopy = std::min(nRemaining, nBytes); @@ -688,7 +687,7 @@ int CNetMessage::readData(const char *pch, unsigned int nBytes) return nCopy; } -const uint256& CNetMessage::GetMessageHash() const +const uint256& TransportDeserializer::GetMessageHash() const { assert(complete()); if (data_hash.IsNull()) @@ -696,6 +695,35 @@ const uint256& CNetMessage::GetMessageHash() const return data_hash; } +CNetMessage TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) { + // decompose a single CNetMessage from the TransportDeserializer + CNetMessage msg(std::move(vRecv)); + + // store state about valid header, netmagic and checksum + msg.m_valid_header = hdr.IsValid(message_start); + msg.m_valid_netmagic = (memcmp(hdr.pchMessageStart, message_start, CMessageHeader::MESSAGE_START_SIZE) == 0); + uint256 hash = GetMessageHash(); + + // store command string, payload size + msg.m_command = hdr.GetCommand(); + msg.m_message_size = hdr.nMessageSize; + + msg.m_valid_checksum = (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0); + if (!msg.m_valid_checksum) { + LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s\n", + SanitizeString(msg.m_command), msg.m_message_size, + HexStr(hash.begin(), hash.begin()+CMessageHeader::CHECKSUM_SIZE), + HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE)); + } + + // store receive time + msg.m_time = time; + + // reset the network deserializer (prepare for the next message) + Reset(); + return msg; +} + size_t CConnman::SocketSendData(CNode *pnode) const EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_vSend) { auto it = pnode->vSendMsg.begin(); @@ -1347,9 +1375,9 @@ void CConnman::SocketHandler() size_t nSizeAdded = 0; auto it(pnode->vRecvMsg.begin()); for (; it != pnode->vRecvMsg.end(); ++it) { - if (!it->complete()) - break; - nSizeAdded += it->vRecv.size() + CMessageHeader::HEADER_SIZE; + // vRecvMsg contains only completed CNetMessage + // the single possible partially deserialized message are held by TransportDeserializer + nSizeAdded += it->m_recv.size() + CMessageHeader::HEADER_SIZE; } { LOCK(pnode->cs_vProcessMsg); @@ -2678,6 +2706,8 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn } else { LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } + + m_deserializer = MakeUnique(TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION)); } CNode::~CNode() -- cgit v1.2.3 From 1a5c656c3169ba525f84145d19ce8c64f2cf1efb Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 13 Jun 2019 11:07:50 +0200 Subject: Remove transport protocol knowhow from CNetMessage / net processing --- src/net.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index bd183ce52..ee9d71786 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -601,7 +601,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete if (i == mapRecvBytesPerMsgCmd.end()) i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); assert(i != mapRecvBytesPerMsgCmd.end()); - i->second += m_deserializer->hdr.nMessageSize + CMessageHeader::HEADER_SIZE; + i->second += msg.m_raw_message_size; // push the message to the process queue, vRecvMsg.push_back(std::move(msg)); @@ -707,6 +707,7 @@ CNetMessage TransportDeserializer::GetMessage(const CMessageHeader::MessageStart // store command string, payload size msg.m_command = hdr.GetCommand(); msg.m_message_size = hdr.nMessageSize; + msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; msg.m_valid_checksum = (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0); if (!msg.m_valid_checksum) { @@ -1377,7 +1378,7 @@ void CConnman::SocketHandler() for (; it != pnode->vRecvMsg.end(); ++it) { // vRecvMsg contains only completed CNetMessage // the single possible partially deserialized message are held by TransportDeserializer - nSizeAdded += it->m_recv.size() + CMessageHeader::HEADER_SIZE; + nSizeAdded += it->m_raw_message_size; } { LOCK(pnode->cs_vProcessMsg); -- cgit v1.2.3 From efecb74677222f6c70adf7f860c315f430d39ec4 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 13 Jun 2019 11:25:54 +0200 Subject: Use adapter pattern for the network deserializer --- src/net.cpp | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index ee9d71786..210258c25 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -571,18 +571,13 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete nRecvBytes += nBytes; while (nBytes > 0) { // absorb network data - int handled; - if (!m_deserializer->in_data) - handled = m_deserializer->readHeader(pch, nBytes); - else - handled = m_deserializer->readData(pch, nBytes); - + int handled = m_deserializer->Read(pch, nBytes); if (handled < 0) { m_deserializer->Reset(); return false; } - if (m_deserializer->in_data && m_deserializer->hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { + if (m_deserializer->OversizedMessageDetected()) { LogPrint(BCLog::NET, "Oversized message from peer=%i, disconnecting\n", GetId()); m_deserializer->Reset(); return false; @@ -591,13 +586,13 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete pch += handled; nBytes -= handled; - if (m_deserializer->complete()) { + if (m_deserializer->Complete()) { // decompose a transport agnostic CNetMessage from the deserializer CNetMessage msg = m_deserializer->GetMessage(Params().MessageStart(), nTimeMicros); //store received bytes per message command //to prevent a memory DOS, only allow valid commands - mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(m_deserializer->hdr.pchCommand); + mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command); if (i == mapRecvBytesPerMsgCmd.end()) i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); assert(i != mapRecvBytesPerMsgCmd.end()); @@ -639,7 +634,7 @@ int CNode::GetSendVersion() const return nSendVersion; } -int TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) +int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) { // copy data to temporary parsing buffer unsigned int nRemaining = 24 - nHdrPos; @@ -670,7 +665,7 @@ int TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) return nCopy; } -int TransportDeserializer::readData(const char *pch, unsigned int nBytes) +int V1TransportDeserializer::readData(const char *pch, unsigned int nBytes) { unsigned int nRemaining = hdr.nMessageSize - nDataPos; unsigned int nCopy = std::min(nRemaining, nBytes); @@ -687,15 +682,15 @@ int TransportDeserializer::readData(const char *pch, unsigned int nBytes) return nCopy; } -const uint256& TransportDeserializer::GetMessageHash() const +const uint256& V1TransportDeserializer::GetMessageHash() const { - assert(complete()); + assert(Complete()); if (data_hash.IsNull()) hasher.Finalize(data_hash.begin()); return data_hash; } -CNetMessage TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) { +CNetMessage V1TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) { // decompose a single CNetMessage from the TransportDeserializer CNetMessage msg(std::move(vRecv)); @@ -2708,7 +2703,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } - m_deserializer = MakeUnique(TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION)); + m_deserializer = MakeUnique(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION)); } CNode::~CNode() -- cgit v1.2.3 From 6a91499496d76c2b3e84489e9723b60514fb08db Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 18 Oct 2019 11:57:10 -0700 Subject: Remove oversized message detection from log and interface --- src/net.cpp | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index 210258c25..8b9ef1d0a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -577,12 +577,6 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete return false; } - if (m_deserializer->OversizedMessageDetected()) { - LogPrint(BCLog::NET, "Oversized message from peer=%i, disconnecting\n", GetId()); - m_deserializer->Reset(); - return false; - } - pch += handled; nBytes -= handled; @@ -655,9 +649,10 @@ int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) return -1; } - // reject messages larger than MAX_SIZE - if (hdr.nMessageSize > MAX_SIZE) + // reject messages larger than MAX_SIZE or MAX_PROTOCOL_MESSAGE_LENGTH + if (hdr.nMessageSize > MAX_SIZE || hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { return -1; + } // switch state to reading message data in_data = true; -- cgit v1.2.3 From f342a5e61a73e1edf389b662d265d20cf26a1d51 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 18 Oct 2019 12:03:13 -0700 Subject: Make resetting implicit in TransportDeserializer::Read() --- src/net.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index 8b9ef1d0a..be4427f79 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -572,10 +572,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete while (nBytes > 0) { // absorb network data int handled = m_deserializer->Read(pch, nBytes); - if (handled < 0) { - m_deserializer->Reset(); - return false; - } + if (handled < 0) return false; pch += handled; nBytes -= handled; -- cgit v1.2.3