Merge #20432: net: Treat raw message bytes as uint8_t

laanwj · laanwj · commit 1b75f2542d15 · 2020-11-23T10:26:25.000+01:00
fabecce 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 fabecce theStack: Code Review ACK fabecce jonatack: Tested ACK fabecce Tree-SHA512: e6d9803c78633fde3304faf592afa961ff9462a7912d1da97a24720265274aa10ab4168d71b6ec2756b7448dd42585321afee0e5c889e705be778ce9a330d145
diff --git a/src/net.cpp b/src/net.cpp
@@ -629,7 +629,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
 }
 #undef X
 
-bool CNode::ReceiveMsgBytes(Span<const char> msg_bytes, bool& complete)
+bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
 {
     complete = false;
     const auto time = GetTime<std::chrono::microseconds>();
@@ -673,7 +673,7 @@ bool CNode::ReceiveMsgBytes(Span<const char> msg_bytes, bool& complete)
     return true;
 }
 
-int V1TransportDeserializer::readHeader(Span<const char> msg_bytes)
+int V1TransportDeserializer::readHeader(Span<const uint8_t> msg_bytes)
 {
     // copy data to temporary parsing buffer
     unsigned int nRemaining = CMessageHeader::HEADER_SIZE - nHdrPos;
@@ -713,7 +713,7 @@ int V1TransportDeserializer::readHeader(Span<const char> msg_bytes)
     return nCopy;
 }
 
-int V1TransportDeserializer::readData(Span<const char> msg_bytes)
+int V1TransportDeserializer::readData(Span<const uint8_t> msg_bytes)
 {
     unsigned int nRemaining = hdr.nMessageSize - nDataPos;
     unsigned int nCopy = std::min<unsigned int>(nRemaining, msg_bytes.size());
@@ -723,7 +723,7 @@ int V1TransportDeserializer::readData(Span<const char> 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;
 
@@ -1463,18 +1463,18 @@ void CConnman::SocketHandler()
         if (recvSet || errorSet)
         {
             // 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)
                     continue;
-                nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT);
+                nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT);
             }
             if (nBytes > 0)
             {
                 bool notify = false;
-                if (!pnode->ReceiveMsgBytes(Span<const char>(pchBuf, nBytes), notify))
+                if (!pnode->ReceiveMsgBytes(Span<const uint8_t>(pchBuf, nBytes), notify))
                     pnode->CloseSocketDisconnect();
                 RecordBytesRecv(nBytes);
                 if (notify) {
diff --git a/src/net.h b/src/net.h
@@ -758,7 +758,7 @@ class TransportDeserializer {
     // set the serialization context version
     virtual void SetVersion(int version) = 0;
     /** read and deserialize data, advances msg_bytes data pointer */
-    virtual int Read(Span<const char>& msg_bytes) = 0;
+    virtual int Read(Span<const uint8_t>& msg_bytes) = 0;
     // decomposes a message from the context
     virtual Optional<CNetMessage> GetMessage(std::chrono::microseconds time, uint32_t& out_err) = 0;
     virtual ~TransportDeserializer() {}
@@ -779,8 +779,8 @@ class V1TransportDeserializer final : public TransportDeserializer
     unsigned int nDataPos;
 
     const uint256& GetMessageHash() const;
-    int readHeader(Span<const char> msg_bytes);
-    int readData(Span<const char> msg_bytes);
+    int readHeader(Span<const uint8_t> msg_bytes);
+    int readData(Span<const uint8_t> msg_bytes);
 
     void Reset() {
         vRecv.clear();
@@ -814,7 +814,7 @@ class V1TransportDeserializer final : public TransportDeserializer
         hdrbuf.SetVersion(nVersionIn);
         vRecv.SetVersion(nVersionIn);
     }
-    int Read(Span<const char>& msg_bytes) override
+    int Read(Span<const uint8_t>& msg_bytes) override
     {
         int ret = in_data ? readData(msg_bytes) : readHeader(msg_bytes);
         if (ret < 0) {
@@ -1132,7 +1132,7 @@ class CNode
      * @return  True if the peer should stay connected,
      *          False if the peer should be disconnected from.
      */
-    bool ReceiveMsgBytes(Span<const char> msg_bytes, bool& complete);
+    bool ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete);
 
     void SetCommonVersion(int greatest_common_version)
     {
diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp
@@ -128,7 +128,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         case 11: {
             const std::vector<uint8_t> b = ConsumeRandomLengthByteVector(fuzzed_data_provider);
             bool complete;
-            node.ReceiveMsgBytes({(const char*)b.data(), b.size()}, complete);
+            node.ReceiveMsgBytes(b, complete);
             break;
         }
         }
diff --git a/src/test/fuzz/p2p_transport_deserializer.cpp b/src/test/fuzz/p2p_transport_deserializer.cpp
@@ -21,7 +21,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
 {
     // Construct deserializer, with a dummy NodeId
     V1TransportDeserializer deserializer{Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION};
-    Span<const char> msg_bytes{(const char*)buffer.data(), buffer.size()};
+    Span<const uint8_t> 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
@@ -7,7 +7,7 @@
 #include <chainparams.h>
 #include <net.h>
 
-void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const char> msg_bytes, bool& complete) const
+void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const
 {
     assert(node.ReceiveMsgBytes(msg_bytes, complete));
     if (complete) {
@@ -29,11 +29,11 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const char> msg_bytes
 
 bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const
 {
-    std::vector<unsigned char> ser_msg_header;
+    std::vector<uint8_t> 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
@@ -25,7 +25,7 @@ struct ConnmanTestMsg : public CConnman {
 
     void ProcessMessagesOnce(CNode& node) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); }
 
-    void NodeReceiveMsgBytes(CNode& node, Span<const char> msg_bytes, bool& complete) const;
+    void NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const;
 
     bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const;
 };

Original file line number	Diff line number	Diff line change
`@@ -629,7 +629,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)`
`629`	`629`	`}`
`630`	`630`	`#undef X`
`631`	`631`
`632`		`-bool CNode::ReceiveMsgBytes(Span<const char> msg_bytes, bool& complete)`
	`632`	`+bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)`
`633`	`633`	`{`
`634`	`634`	`complete = false;`
`635`	`635`	`const auto time = GetTime<std::chrono::microseconds>();`
`@@ -673,7 +673,7 @@ bool CNode::ReceiveMsgBytes(Span<const char> msg_bytes, bool& complete)`
`673`	`673`	`return true;`
`674`	`674`	`}`
`675`	`675`
`676`		`-int V1TransportDeserializer::readHeader(Span<const char> msg_bytes)`
	`676`	`+int V1TransportDeserializer::readHeader(Span<const uint8_t> msg_bytes)`
`677`	`677`	`{`
`678`	`678`	`// copy data to temporary parsing buffer`
`679`	`679`	`unsigned int nRemaining = CMessageHeader::HEADER_SIZE - nHdrPos;`
`@@ -713,7 +713,7 @@ int V1TransportDeserializer::readHeader(Span<const char> msg_bytes)`
`713`	`713`	`return nCopy;`
`714`	`714`	`}`
`715`	`715`
`716`		`-int V1TransportDeserializer::readData(Span<const char> msg_bytes)`
	`716`	`+int V1TransportDeserializer::readData(Span<const uint8_t> msg_bytes)`
`717`	`717`	`{`
`718`	`718`	`unsigned int nRemaining = hdr.nMessageSize - nDataPos;`
`719`	`719`	`unsigned int nCopy = std::min<unsigned int>(nRemaining, msg_bytes.size());`
`@@ -723,7 +723,7 @@ int V1TransportDeserializer::readData(Span<const char> msg_bytes)`
`723`	`723`	`vRecv.resize(std::min(hdr.nMessageSize, nDataPos + nCopy + 256 * 1024));`
`724`	`724`	`}`
`725`	`725`
`726`		`- hasher.Write(MakeUCharSpan(msg_bytes.first(nCopy)));`
	`726`	`+ hasher.Write(msg_bytes.first(nCopy));`
`727`	`727`	`memcpy(&vRecv[nDataPos], msg_bytes.data(), nCopy);`
`728`	`728`	`nDataPos += nCopy;`
`729`	`729`
`@@ -1463,18 +1463,18 @@ void CConnman::SocketHandler()`
`1463`	`1463`	`if (recvSet \|\| errorSet)`
`1464`	`1464`	`{`
`1465`	`1465`	`// typical socket buffer is 8K-64K`
`1466`		`- char pchBuf[0x10000];`
	`1466`	`+ uint8_t pchBuf[0x10000];`
`1467`	`1467`	`int nBytes = 0;`
`1468`	`1468`	`{`
`1469`	`1469`	`LOCK(pnode->cs_hSocket);`
`1470`	`1470`	`if (pnode->hSocket == INVALID_SOCKET)`
`1471`	`1471`	`continue;`
`1472`		`- nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT);`
	`1472`	`+ nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT);`
`1473`	`1473`	`}`
`1474`	`1474`	`if (nBytes > 0)`
`1475`	`1475`	`{`
`1476`	`1476`	`bool notify = false;`
`1477`		`- if (!pnode->ReceiveMsgBytes(Span<const char>(pchBuf, nBytes), notify))`
	`1477`	`+ if (!pnode->ReceiveMsgBytes(Span<const uint8_t>(pchBuf, nBytes), notify))`
`1478`	`1478`	`pnode->CloseSocketDisconnect();`
`1479`	`1479`	`RecordBytesRecv(nBytes);`
`1480`	`1480`	`if (notify) {`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)`
`128`	`128`	`case 11: {`
`129`	`129`	`const std::vector<uint8_t> b = ConsumeRandomLengthByteVector(fuzzed_data_provider);`
`130`	`130`	`bool complete;`
`131`		`- node.ReceiveMsgBytes({(const char*)b.data(), b.size()}, complete);`
	`131`	`+ node.ReceiveMsgBytes(b, complete);`
`132`	`132`	`break;`
`133`	`133`	`}`
`134`	`134`	`}`
Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)`
`21`	`21`	`{`
`22`	`22`	`// Construct deserializer, with a dummy NodeId`
`23`	`23`	`V1TransportDeserializer deserializer{Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION};`
`24`		`- Span<const char> msg_bytes{(const char*)buffer.data(), buffer.size()};`
	`24`	`+ Span<const uint8_t> msg_bytes{buffer};`
`25`	`25`	`while (msg_bytes.size() > 0) {`
`26`	`26`	`const int handled = deserializer.Read(msg_bytes);`
`27`	`27`	`if (handled < 0) {`