From c1b5ec6360a932413aaba73bbbbad4c4605d59fe Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 23 Nov 2020 10:25:56 +0100 Subject: [PATCH] Merge #20432: net: Treat raw message bytes as uint8_t fabecce71909c984504c21fa05f91d5f1b471e8c net: Treat raw message bytes as uint8_t (MarcoFalke) Pull request description: Using `uint8_t` from the beginning when messages are `recv`ed has two style benefits: * The signedness is clear from reading the code, as it does not depend on the architecture * When passing the bytes on, the need for static signedness casts is dropped, making the code a bit less verbose and more coherent ACKs for top commit: laanwj: Code review ACK fabecce71909c984504c21fa05f91d5f1b471e8c theStack: Code Review ACK fabecce71909c984504c21fa05f91d5f1b471e8c jonatack: Tested ACK fabecce71909c984504c21fa05f91d5f1b471e8c Tree-SHA512: e6d9803c78633fde3304faf592afa961ff9462a7912d1da97a24720265274aa10ab4168d71b6ec2756b7448dd42585321afee0e5c889e705be778ce9a330d145 --- src/net.cpp | 14 +++++++------- src/net.h | 10 +++++----- src/test/fuzz/net.cpp | 2 +- src/test/fuzz/p2p_transport_deserializer.cpp | 2 +- src/test/util/net.cpp | 8 ++++---- src/test/util/net.h | 2 +- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 3f63316140..71843e8ded 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -676,7 +676,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) } #undef X -bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) +bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) { complete = false; int64_t nTimeMicros = GetTimeMicros(); @@ -747,7 +747,7 @@ int CNode::GetSendVersion() const return nSendVersion; } -int V1TransportDeserializer::readHeader(Span msg_bytes) +int V1TransportDeserializer::readHeader(Span msg_bytes) { // copy data to temporary parsing buffer unsigned int nRemaining = 24 - nHdrPos; @@ -787,7 +787,7 @@ int V1TransportDeserializer::readHeader(Span msg_bytes) return nCopy; } -int V1TransportDeserializer::readData(Span msg_bytes) +int V1TransportDeserializer::readData(Span msg_bytes) { unsigned int nRemaining = hdr.nMessageSize - nDataPos; unsigned int nCopy = std::min(nRemaining, msg_bytes.size()); @@ -797,7 +797,7 @@ int V1TransportDeserializer::readData(Span msg_bytes) vRecv.resize(std::min(hdr.nMessageSize, nDataPos + nCopy + 256 * 1024)); } - hasher.Write(MakeUCharSpan(msg_bytes.first(nCopy))); + hasher.Write(msg_bytes.first(nCopy)); memcpy(&vRecv[nDataPos], msg_bytes.data(), nCopy); nDataPos += nCopy; @@ -1912,13 +1912,13 @@ void CConnman::SocketHandler() size_t CConnman::SocketRecvData(CNode *pnode) { // typical socket buffer is 8K-64K - char pchBuf[0x10000]; + uint8_t pchBuf[0x10000]; int nBytes = 0; { LOCK(pnode->cs_hSocket); if (pnode->hSocket == INVALID_SOCKET) return 0; - nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT); + nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT); if (nBytes < (int)sizeof(pchBuf)) { pnode->fHasRecvData = false; } @@ -1926,7 +1926,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) if (nBytes > 0) { bool notify = false; - if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify)) { + if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify)) { LOCK(cs_vNodes); pnode->CloseSocketDisconnect(this); } diff --git a/src/net.h b/src/net.h index 138a228621..359c00b795 100644 --- a/src/net.h +++ b/src/net.h @@ -934,7 +934,7 @@ public: // set the serialization context version virtual void SetVersion(int version) = 0; /** read and deserialize data, advances msg_bytes data pointer */ - virtual int Read(Span& msg_bytes) = 0; + virtual int Read(Span& msg_bytes) = 0; // decomposes a message from the context virtual std::optional GetMessage(int64_t time, uint32_t& out_err) = 0; virtual ~TransportDeserializer() {} @@ -955,8 +955,8 @@ private: unsigned int nDataPos; const uint256& GetMessageHash() const; - int readHeader(Span msg_bytes); - int readData(Span msg_bytes); + int readHeader(Span msg_bytes); + int readData(Span msg_bytes); void Reset() { vRecv.clear(); @@ -990,7 +990,7 @@ public: hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - int Read(Span& msg_bytes) override + int Read(Span& msg_bytes) override { int ret = in_data ? readData(msg_bytes) : readHeader(msg_bytes); if (ret < 0) { @@ -1285,7 +1285,7 @@ public: * @return True if the peer should stay connected, * False if the peer should be disconnected from. */ - bool ReceiveMsgBytes(Span msg_bytes, bool& complete); + bool ReceiveMsgBytes(Span msg_bytes, bool& complete); void SetRecvVersion(int nVersionIn) { diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 6df0b19cd7..56ecab8d51 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -129,7 +129,7 @@ FUZZ_TARGET_INIT(net, initialize_net) [&] { const std::vector b = ConsumeRandomLengthByteVector(fuzzed_data_provider); bool complete; - node.ReceiveMsgBytes({(const char*)b.data(), b.size()}, complete); + node.ReceiveMsgBytes(b, complete); }); } diff --git a/src/test/fuzz/p2p_transport_deserializer.cpp b/src/test/fuzz/p2p_transport_deserializer.cpp index ad96d896e3..599dc0d044 100644 --- a/src/test/fuzz/p2p_transport_deserializer.cpp +++ b/src/test/fuzz/p2p_transport_deserializer.cpp @@ -22,7 +22,7 @@ FUZZ_TARGET_INIT(p2p_transport_deserializer, initialize_p2p_transport_deserializ { // Construct deserializer, with a dummy NodeId V1TransportDeserializer deserializer{Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION}; - Span msg_bytes{(const char*)buffer.data(), buffer.size()}; + Span msg_bytes{buffer}; while (msg_bytes.size() > 0) { const int handled = deserializer.Read(msg_bytes); if (handled < 0) { diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 3b31ec4031..847a490e03 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -7,7 +7,7 @@ #include #include -void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const +void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const { assert(node.ReceiveMsgBytes(msg_bytes, complete)); if (complete) { @@ -29,11 +29,11 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const { - std::vector ser_msg_header; + std::vector ser_msg_header; node.m_serializer->prepareForTransport(ser_msg, ser_msg_header); bool complete; - NodeReceiveMsgBytes(node, {(const char*)ser_msg_header.data(), ser_msg_header.size()}, complete); - NodeReceiveMsgBytes(node, {(const char*)ser_msg.data.data(), ser_msg.data.size()}, complete); + NodeReceiveMsgBytes(node, ser_msg_header, complete); + NodeReceiveMsgBytes(node, ser_msg.data, complete); return complete; } diff --git a/src/test/util/net.h b/src/test/util/net.h index 607a754f60..0cdebe4678 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -31,7 +31,7 @@ struct ConnmanTestMsg : public CConnman { void ProcessMessagesOnce(CNode& node) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); } - void NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const; + void NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const; bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const; };