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 ++++++++++++++++++++++++++++++++++---------------- src/net.h | 43 ++++++++++++++++++++++++---- src/net_processing.cpp | 32 +++++++++------------ 3 files changed, 106 insertions(+), 47 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index 63b7833822..bd183ce52b 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() diff --git a/src/net.h b/src/net.h index 44655abf80..a9ce33957d 100644 --- a/src/net.h +++ b/src/net.h @@ -609,8 +609,33 @@ public: - +/** Transport protocol agnostic message container. + * Ideally it should only contain receive time, payload, + * command and size. + */ class CNetMessage { +public: + CDataStream m_recv; // received message data + int64_t m_time = 0; // time (in microseconds) of message receipt. + bool m_valid_netmagic = false; + bool m_valid_header = false; + bool m_valid_checksum = false; + uint32_t m_message_size = 0; // size of the payload + std::string m_command; + + CNetMessage(const CDataStream& recv_in) : m_recv(std::move(recv_in)) {} + + void SetVersion(int nVersionIn) + { + m_recv.SetVersion(nVersionIn); + } +}; + +/** The TransportDeserializer takes care of holding and deserializing the + * network receive buffer. It can deserialize the network buffer into a + * transport protocol agnostic CNetMessage (command & payload) + */ +class TransportDeserializer { private: mutable CHash256 hasher; mutable uint256 data_hash; @@ -624,14 +649,19 @@ public: CDataStream vRecv; // received message data unsigned int nDataPos; - int64_t nTime; // time (in microseconds) of message receipt. + TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) { + Reset(); + } - CNetMessage(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) { + void Reset() { + vRecv.clear(); + hdrbuf.clear(); hdrbuf.resize(24); in_data = false; nHdrPos = 0; nDataPos = 0; - nTime = 0; + data_hash.SetNull(); + hasher.Reset(); } bool complete() const @@ -651,14 +681,17 @@ public: int readHeader(const char *pch, unsigned int nBytes); int readData(const char *pch, unsigned int nBytes); -}; + CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time); +}; /** Information about a peer */ class CNode { friend class CConnman; public: + std::unique_ptr m_deserializer; + // socket std::atomic nServices{NODE_NONE}; SOCKET hSocket GUARDED_BY(cs_hSocket); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b6839dcf21..c8ffbbbe9d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3260,41 +3260,37 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter return false; // Just take one message msgs.splice(msgs.begin(), pfrom->vProcessMsg, pfrom->vProcessMsg.begin()); - pfrom->nProcessQueueSize -= msgs.front().vRecv.size() + CMessageHeader::HEADER_SIZE; + pfrom->nProcessQueueSize -= msgs.front().m_recv.size() + CMessageHeader::HEADER_SIZE; pfrom->fPauseRecv = pfrom->nProcessQueueSize > connman->GetReceiveFloodSize(); fMoreWork = !pfrom->vProcessMsg.empty(); } CNetMessage& msg(msgs.front()); msg.SetVersion(pfrom->GetRecvVersion()); - // Scan for message start - if (memcmp(msg.hdr.pchMessageStart, chainparams.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) != 0) { - LogPrint(BCLog::NET, "PROCESSMESSAGE: INVALID MESSAGESTART %s peer=%d\n", SanitizeString(msg.hdr.GetCommand()), pfrom->GetId()); + // Check network magic + if (!msg.m_valid_netmagic) { + LogPrint(BCLog::NET, "PROCESSMESSAGE: INVALID MESSAGESTART %s peer=%d\n", SanitizeString(msg.m_command), pfrom->GetId()); pfrom->fDisconnect = true; return false; } - // Read header - CMessageHeader& hdr = msg.hdr; - if (!hdr.IsValid(chainparams.MessageStart())) + // Check header + if (!msg.m_valid_header) { - LogPrint(BCLog::NET, "PROCESSMESSAGE: ERRORS IN HEADER %s peer=%d\n", SanitizeString(hdr.GetCommand()), pfrom->GetId()); + LogPrint(BCLog::NET, "PROCESSMESSAGE: ERRORS IN HEADER %s peer=%d\n", SanitizeString(msg.m_command), pfrom->GetId()); return fMoreWork; } - std::string strCommand = hdr.GetCommand(); + const std::string& strCommand = msg.m_command; // Message size - unsigned int nMessageSize = hdr.nMessageSize; + unsigned int nMessageSize = msg.m_message_size; // Checksum - CDataStream& vRecv = msg.vRecv; - const uint256& hash = msg.GetMessageHash(); - if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) + CDataStream& vRecv = msg.m_recv; + if (!msg.m_valid_checksum) { - LogPrint(BCLog::NET, "%s(%s, %u bytes): CHECKSUM ERROR expected %s was %s\n", __func__, - SanitizeString(strCommand), nMessageSize, - HexStr(hash.begin(), hash.begin()+CMessageHeader::CHECKSUM_SIZE), - HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE)); + LogPrint(BCLog::NET, "%s(%s, %u bytes): CHECKSUM ERROR peer=%d\n", __func__, + SanitizeString(strCommand), nMessageSize, pfrom->GetId()); return fMoreWork; } @@ -3302,7 +3298,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter bool fRet = false; try { - fRet = ProcessMessage(pfrom, strCommand, vRecv, msg.nTime, chainparams, connman, interruptMsgProc); + fRet = ProcessMessage(pfrom, strCommand, vRecv, msg.m_time, chainparams, connman, interruptMsgProc); if (interruptMsgProc) return false; if (!pfrom->vRecvGetData.empty()) -- 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 +++-- src/net.h | 1 + src/net_processing.cpp | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index bd183ce52b..ee9d71786a 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); diff --git a/src/net.h b/src/net.h index a9ce33957d..15996438c9 100644 --- a/src/net.h +++ b/src/net.h @@ -621,6 +621,7 @@ public: bool m_valid_header = false; bool m_valid_checksum = false; uint32_t m_message_size = 0; // size of the payload + uint32_t m_raw_message_size = 0; // used wire size of the message (including header/checksum) std::string m_command; CNetMessage(const CDataStream& recv_in) : m_recv(std::move(recv_in)) {} diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c8ffbbbe9d..9496516c37 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3260,7 +3260,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter return false; // Just take one message msgs.splice(msgs.begin(), pfrom->vProcessMsg, pfrom->vProcessMsg.begin()); - pfrom->nProcessQueueSize -= msgs.front().m_recv.size() + CMessageHeader::HEADER_SIZE; + pfrom->nProcessQueueSize -= msgs.front().m_raw_message_size; pfrom->fPauseRecv = pfrom->nProcessQueueSize > connman->GetReceiveFloodSize(); fMoreWork = !pfrom->vProcessMsg.empty(); } -- 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 ++++++++++--------------- src/net.h | 46 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 42 insertions(+), 29 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index ee9d71786a..210258c25a 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() diff --git a/src/net.h b/src/net.h index 15996438c9..16435a3aa1 100644 --- a/src/net.h +++ b/src/net.h @@ -637,20 +637,40 @@ public: * transport protocol agnostic CNetMessage (command & payload) */ class TransportDeserializer { +public: + // prepare for next message + virtual void Reset() = 0; + // returns true if the current deserialization is complete + virtual bool Complete() const = 0; + // checks if the potential message in deserialization is oversized + virtual bool OversizedMessageDetected() const = 0; + // set the serialization context version + virtual void SetVersion(int version) = 0; + // read and deserialize data + virtual int Read(const char *data, unsigned int bytes) = 0; + // decomposes a message from the context + virtual CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) = 0; + virtual ~TransportDeserializer() {} +}; + +class V1TransportDeserializer : public TransportDeserializer +{ private: mutable CHash256 hasher; mutable uint256 data_hash; -public: bool in_data; // parsing header (false) or data (true) - CDataStream hdrbuf; // partially received header CMessageHeader hdr; // complete header - unsigned int nHdrPos; - CDataStream vRecv; // received message data + unsigned int nHdrPos; unsigned int nDataPos; - TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) { + const uint256& GetMessageHash() const; + int readHeader(const char *pch, unsigned int nBytes); + int readData(const char *pch, unsigned int nBytes); +public: + + V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) { Reset(); } @@ -664,25 +684,23 @@ public: data_hash.SetNull(); hasher.Reset(); } - - bool complete() const + bool Complete() const { if (!in_data) return false; return (hdr.nMessageSize == nDataPos); } - - const uint256& GetMessageHash() const; - void SetVersion(int nVersionIn) { hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - - int readHeader(const char *pch, unsigned int nBytes); - int readData(const char *pch, unsigned int nBytes); - + bool OversizedMessageDetected() const { + return (in_data && hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH); + } + int Read(const char *pch, unsigned int nBytes) { + return in_data ? readData(pch, nBytes) : readHeader(pch, nBytes); + } CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time); }; -- cgit v1.2.3 From b0e10ff4df3d4c70fb172ea8c3128c82e6e368bb Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 22 Oct 2019 15:28:32 +0200 Subject: Force CNetMessage::m_recv to use std::move --- src/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/net.h b/src/net.h index 16435a3aa1..6122508471 100644 --- a/src/net.h +++ b/src/net.h @@ -624,7 +624,7 @@ public: uint32_t m_raw_message_size = 0; // used wire size of the message (including header/checksum) std::string m_command; - CNetMessage(const CDataStream& recv_in) : m_recv(std::move(recv_in)) {} + CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {} void SetVersion(int nVersionIn) { -- 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 +++-------- src/net.h | 5 ----- 2 files changed, 3 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index 210258c25a..8b9ef1d0aa 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; diff --git a/src/net.h b/src/net.h index 6122508471..570ae83465 100644 --- a/src/net.h +++ b/src/net.h @@ -642,8 +642,6 @@ public: virtual void Reset() = 0; // returns true if the current deserialization is complete virtual bool Complete() const = 0; - // checks if the potential message in deserialization is oversized - virtual bool OversizedMessageDetected() const = 0; // set the serialization context version virtual void SetVersion(int version) = 0; // read and deserialize data @@ -695,9 +693,6 @@ public: hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - bool OversizedMessageDetected() const { - return (in_data && hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH); - } int Read(const char *pch, unsigned int nBytes) { return in_data ? readData(pch, nBytes) : readHeader(pch, nBytes); } -- 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 +---- src/net.h | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index 8b9ef1d0aa..be4427f793 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; diff --git a/src/net.h b/src/net.h index 570ae83465..f6a04f5166 100644 --- a/src/net.h +++ b/src/net.h @@ -638,8 +638,6 @@ public: */ class TransportDeserializer { public: - // prepare for next message - virtual void Reset() = 0; // returns true if the current deserialization is complete virtual bool Complete() const = 0; // set the serialization context version @@ -666,11 +664,6 @@ private: const uint256& GetMessageHash() const; int readHeader(const char *pch, unsigned int nBytes); int readData(const char *pch, unsigned int nBytes); -public: - - V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) { - Reset(); - } void Reset() { vRecv.clear(); @@ -682,6 +675,13 @@ public: data_hash.SetNull(); hasher.Reset(); } + +public: + + V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) { + Reset(); + } + bool Complete() const { if (!in_data) @@ -694,7 +694,9 @@ public: vRecv.SetVersion(nVersionIn); } int Read(const char *pch, unsigned int nBytes) { - return in_data ? readData(pch, nBytes) : readHeader(pch, nBytes); + int ret = in_data ? readData(pch, nBytes) : readHeader(pch, nBytes); + if (ret < 0) Reset(); + return ret; } CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time); }; -- cgit v1.2.3 From ed2dc5e48abed1cde6ab98025dc8212917d47d21 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 18 Oct 2019 12:09:48 -0700 Subject: Add override/final modifiers to V1TransportDeserializer --- src/net.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/net.h b/src/net.h index f6a04f5166..af73361c08 100644 --- a/src/net.h +++ b/src/net.h @@ -649,7 +649,7 @@ public: virtual ~TransportDeserializer() {} }; -class V1TransportDeserializer : public TransportDeserializer +class V1TransportDeserializer final : public TransportDeserializer { private: mutable CHash256 hasher; @@ -682,23 +682,23 @@ public: Reset(); } - bool Complete() const + bool Complete() const override { if (!in_data) return false; return (hdr.nMessageSize == nDataPos); } - void SetVersion(int nVersionIn) + void SetVersion(int nVersionIn) override { hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - int Read(const char *pch, unsigned int nBytes) { + int Read(const char *pch, unsigned int nBytes) override { int ret = in_data ? readData(pch, nBytes) : readHeader(pch, nBytes); if (ret < 0) Reset(); return ret; } - CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time); + CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) override; }; /** Information about a peer */ -- cgit v1.2.3