diff options
author | fanquake <fanquake@gmail.com> | 2019-10-28 09:15:48 -0400 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2019-10-28 09:15:59 -0400 |
commit | badca85e2c0bf1720fca8a516adbf4b3f1d7e4d9 (patch) | |
tree | 06efc707f6771393cd65539938e0396c38c515dd | |
parent | f8cc2b967b9517af9f858da9170b7bea4bd7c7a3 (diff) | |
parent | ed2dc5e48abed1cde6ab98025dc8212917d47d21 (diff) | |
download | bitcoin-badca85e2c0bf1720fca8a516adbf4b3f1d7e4d9.tar.xz |
Merge #16202: p2p: Refactor network message deserialization
ed2dc5e48abed1cde6ab98025dc8212917d47d21 Add override/final modifiers to V1TransportDeserializer (Pieter Wuille)
f342a5e61a73e1edf389b662d265d20cf26a1d51 Make resetting implicit in TransportDeserializer::Read() (Pieter Wuille)
6a91499496d76c2b3e84489e9723b60514fb08db Remove oversized message detection from log and interface (Pieter Wuille)
b0e10ff4df3d4c70fb172ea8c3128c82e6e368bb Force CNetMessage::m_recv to use std::move (Jonas Schnelli)
efecb74677222f6c70adf7f860c315f430d39ec4 Use adapter pattern for the network deserializer (Jonas Schnelli)
1a5c656c3169ba525f84145d19ce8c64f2cf1efb Remove transport protocol knowhow from CNetMessage / net processing (Jonas Schnelli)
6294ecdb8bb4eb7049a18c721ee8cb4a53d80a06 Refactor: split network transport deserializing from message container (Jonas Schnelli)
Pull request description:
**This refactors the network message deserialization.**
* It transforms the `CNetMessage` into a transport protocol agnostic message container.
* A new class `TransportDeserializer` (unique pointer of `CNode`) is introduced, handling the network buffer reading and the decomposing to a `CNetMessage`
* **No behavioral changes** (in terms of disconnecting, punishing)
* Moves the checksum finalizing into the `SocketHandler` thread (finalizing was in `ProcessMessages` before)
The **optional last commit** makes the `TransportDeserializer` following an adapter pattern (polymorphic interface) to make it easier to later add a V2 transport protocol deserializer.
Intentionally not touching the sending part.
Pre-Requirement for BIP324 (v2 message transport protocol).
Replacement for #14046 and inspired by a [comment](https://github.com/bitcoin/bitcoin/pull/14046#issuecomment-431528330) from sipa
ACKs for top commit:
promag:
Code review ACK ed2dc5e48abed1cde6ab98025dc8212917d47d21.
marcinja:
Code review ACK ed2dc5e48abed1cde6ab98025dc8212917d47d21
ryanofsky:
Code review ACK ed2dc5e48abed1cde6ab98025dc8212917d47d21. 4 cleanup commits added since last review. Unaddressed comments:
ariard:
Code review and tested ACK ed2dc5e.
Tree-SHA512: bab8d87464e2e8742529e488ddcdc8650f0c2025c9130913df00a0b17ecdb9a525061cbbbd0de0251b76bf75a8edb72e3ad0dbf5b79e26f2ad05d61b4e4ded6d
-rw-r--r-- | src/net.cpp | 88 | ||||
-rw-r--r-- | src/net.h | 83 | ||||
-rw-r--r-- | src/net_processing.cpp | 32 | ||||
-rwxr-xr-x | test/functional/p2p_invalid_messages.py | 11 |
4 files changed, 138 insertions, 76 deletions
diff --git a/src/net.cpp b/src/net.cpp index c0b39925c6..7ae88b47a0 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -567,42 +567,28 @@ 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); - else - handled = msg.readData(pch, nBytes); - - if (handled < 0) - return false; - - if (msg.in_data && msg.hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { - LogPrint(BCLog::NET, "Oversized message from peer=%i, disconnecting\n", GetId()); - return false; - } + int handled = m_deserializer->Read(pch, nBytes); + if (handled < 0) 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(msg.m_command); 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 += msg.m_raw_message_size; + + // push the message to the process queue, + vRecvMsg.push_back(std::move(msg)); - msg.nTime = nTimeMicros; complete = true; } } @@ -636,8 +622,7 @@ int CNode::GetSendVersion() const return nSendVersion; } - -int CNetMessage::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; @@ -658,9 +643,10 @@ int CNetMessage::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; @@ -668,7 +654,7 @@ int CNetMessage::readHeader(const char *pch, unsigned int nBytes) return nCopy; } -int CNetMessage::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); @@ -685,14 +671,44 @@ int CNetMessage::readData(const char *pch, unsigned int nBytes) return nCopy; } -const uint256& CNetMessage::GetMessageHash() const +const uint256& V1TransportDeserializer::GetMessageHash() const { - assert(complete()); + assert(Complete()); if (data_hash.IsNull()) hasher.Finalize(data_hash.begin()); return data_hash; } +CNetMessage V1TransportDeserializer::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_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) { + 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(); @@ -1344,9 +1360,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_raw_message_size; } { LOCK(pnode->cs_vProcessMsg); @@ -2676,6 +2692,8 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn } else { LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } + + m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION)); } CNode::~CNode() @@ -609,56 +609,105 @@ 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 + uint32_t m_raw_message_size = 0; // used wire size of the message (including header/checksum) + std::string m_command; + + CNetMessage(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 { +public: + // returns true if the current deserialization is complete + virtual bool Complete() 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 final : 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; - int64_t nTime; // time (in microseconds) of message receipt. + const uint256& GetMessageHash() const; + int readHeader(const char *pch, unsigned int nBytes); + int readData(const char *pch, unsigned int nBytes); - 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 +public: + + V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) { + Reset(); + } + + bool Complete() const override { if (!in_data) return false; return (hdr.nMessageSize == nDataPos); } - - const uint256& GetMessageHash() const; - - void SetVersion(int nVersionIn) + void SetVersion(int nVersionIn) override { hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - - int readHeader(const char *pch, unsigned int nBytes); - int readData(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) override; }; - /** Information about a peer */ class CNode { friend class CConnman; public: + std::unique_ptr<TransportDeserializer> m_deserializer; + // socket std::atomic<ServiceFlags> nServices{NODE_NONE}; SOCKET hSocket GUARDED_BY(cs_hSocket); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4a3076e3c7..fd31c962c2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3272,41 +3272,37 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& 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_raw_message_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; } @@ -3314,7 +3310,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& 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()) diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 58c72f89d8..f0ceb8e6a3 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -101,11 +101,10 @@ class InvalidMessagesTest(BitcoinTestFramework): msg_over_size = msg_unrecognized(str_data="b" * (valid_data_limit + 1)) assert len(msg_over_size.serialize()) == (msg_limit + 1) - with node.assert_debug_log(["Oversized message from peer=4, disconnecting"]): - # An unknown message type (or *any* message type) over - # MAX_PROTOCOL_MESSAGE_LENGTH should result in a disconnect. - node.p2p.send_message(msg_over_size) - node.p2p.wait_for_disconnect(timeout=4) + # An unknown message type (or *any* message type) over + # MAX_PROTOCOL_MESSAGE_LENGTH should result in a disconnect. + node.p2p.send_message(msg_over_size) + node.p2p.wait_for_disconnect(timeout=4) node.disconnect_p2ps() conn = node.add_p2p_connection(P2PDataStore()) @@ -168,7 +167,7 @@ class InvalidMessagesTest(BitcoinTestFramework): def test_checksum(self): conn = self.nodes[0].add_p2p_connection(P2PDataStore()) - with self.nodes[0].assert_debug_log(['ProcessMessages(badmsg, 2 bytes): CHECKSUM ERROR expected 78df0a04 was ffffffff']): + with self.nodes[0].assert_debug_log(['CHECKSUM ERROR (badmsg, 2 bytes), expected 78df0a04 was ffffffff']): msg = conn.build_message(msg_unrecognized(str_data="d")) cut_len = ( 4 + # magic |