Changes for large Packetbuffer allocation to support TCP payloads

Changes to internal checks in SystemPacketBuffer. Update the length encoding for TCP payloads during send and receive. Max size config for large packetbuffer size limit in SystemPacketBuffer.h. Test modifications for chainedbuffer receives for TCP. - Add test for Buffer length greater than MRP max size. Fixes Issues project-chip#31779, project-chip#33307.
pidarped · May 5, 2024 · 6dd1c8e · 6dd1c8e
1 parent a8cfbac
commit 6dd1c8e
Show file tree

Hide file tree

Showing 8 changed files with 105 additions and 48 deletions.
diff --git a/src/inet/TCPEndPointImplSockets.cpp b/src/inet/TCPEndPointImplSockets.cpp
@@ -947,10 +947,10 @@ void TCPEndPointImplSockets::ReceiveData()
         {
             VerifyOrDie(rcvLen > 0);
             size_t newDataLength = rcvBuf->DataLength() + static_cast<size_t>(rcvLen);
-            VerifyOrDie(CanCastTo<uint16_t>(newDataLength));
+            VerifyOrDie(newDataLength <= UINT32_MAX);
             if (isNewBuf)
             {
-                rcvBuf->SetDataLength(static_cast<uint16_t>(newDataLength));
+                rcvBuf->SetDataLength(newDataLength);
                 rcvBuf.RightSize();
                 if (mRcvQueue.IsNull())
                 {
@@ -963,7 +963,7 @@ void TCPEndPointImplSockets::ReceiveData()
             }
             else
             {
-                rcvBuf->SetDataLength(static_cast<uint16_t>(newDataLength), mRcvQueue);
+                rcvBuf->SetDataLength(newDataLength, mRcvQueue);
             }
         }
     }

diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp
@@ -530,12 +530,19 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
 
     CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle());
 
-    // TODO: Change the max to a lower value
-    if (aAvailableSize > UINT32_MAX || lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT32_MAX)
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+    if (lAllocSize > CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES || lBlockSize > UINT32_MAX)
+    {
+        ChipLogError(chipSystemLayer, "PacketBuffer: allocation exceeds limit for large payload size.");
+        return PacketBufferHandle();
+    }
+#else
+    if (lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT16_MAX)
     {
         ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
         return PacketBufferHandle();
     }
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
 
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
 
@@ -593,7 +600,11 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
 PacketBufferHandle PacketBufferHandle::NewWithData(const void * aData, size_t aDataSize, size_t aAdditionalSize,
                                                    uint16_t aReservedSize)
 {
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+    if (aDataSize > UINT32_MAX)
+#else
     if (aDataSize > UINT16_MAX)
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
     {
         ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
         return PacketBufferHandle();
@@ -605,6 +616,8 @@ PacketBufferHandle PacketBufferHandle::NewWithData(const void * aData, size_t aD
     {
         memcpy(buffer.mBuffer->payload, aData, aDataSize);
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
+        // The VerifyOrDie() in the New() call catches buffer allocations greater
+        // than UINT16_MAX for LwIP based platforms.
         buffer.mBuffer->len = buffer.mBuffer->tot_len = static_cast<uint16_t>(aDataSize);
 #else
         buffer.mBuffer->len = buffer.mBuffer->tot_len = aDataSize;
@@ -727,18 +740,24 @@ PacketBufferHandle PacketBufferHandle::CloneData() const
         size_t originalDataSize       = original->MaxDataLength();
         uint16_t originalReservedSize = original->ReservedSize();
 
-        if (originalDataSize + originalReservedSize > PacketBuffer::kMaxSizeWithoutReserve)
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+        uint32_t maxSize = CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES;
+#else
+        uint32_t maxSize = PacketBuffer::kMaxSizeWithoutReserve;
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
+
+        if (originalDataSize + originalReservedSize > maxSize)
         {
             // The original memory allocation may have provided a larger block than requested (e.g. when using a shared pool),
             // and in particular may have provided a larger block than we are able to request from PackBufferHandle::New().
             // It is a genuine error if that extra space has been used.
-            if (originalReservedSize + original->DataLength() > PacketBuffer::kMaxSizeWithoutReserve)
+            if (originalReservedSize + original->DataLength() > maxSize)
             {
                 return PacketBufferHandle();
             }
             // Otherwise, reduce the requested data size. This subtraction can not underflow because the above test
-            // guarantees originalReservedSize <= PacketBuffer::kMaxSizeWithoutReserve.
-            originalDataSize = PacketBuffer::kMaxSizeWithoutReserve - originalReservedSize;
+            // guarantees originalReservedSize <= maxSize.
+            originalDataSize = maxSize - originalReservedSize;
         }
 
         PacketBufferHandle clone = PacketBufferHandle::New(originalDataSize, originalReservedSize);

diff --git a/src/system/SystemPacketBuffer.h b/src/system/SystemPacketBuffer.h
@@ -44,6 +44,19 @@
 #include <lwip/pbuf.h>
 #endif // CHIP_SYSTEM_CONFIG_USE_LWIP
 
+/**
+ * @def CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES
+ *
+ * @brief Maximum payload size of a 'Large' message
+ */
+#ifndef CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES
+#define CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES (64000)
+#endif
+
+#if CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES > UINT32_MAX
+#error "The maximum Large payload size cannot exceed UINT32_MAX"
+#endif
+
 namespace chip {
 namespace System {
 

diff --git a/src/system/tests/TestSystemPacketBuffer.cpp b/src/system/tests/TestSystemPacketBuffer.cpp
@@ -309,7 +309,11 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckNew)
     {
         const PacketBufferHandle buffer = PacketBufferHandle::New(0, config.reserved_size);
 
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+        if (config.reserved_size > CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES)
+#else
         if (config.reserved_size > PacketBuffer::kMaxSizeWithoutReserve)
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
         {
             EXPECT_TRUE(buffer.IsNull());
             continue;
@@ -1605,7 +1609,11 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleRightSize)
 
 TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
 {
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+    uint8_t lPayload[2 * CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES];
+#else
     uint8_t lPayload[2 * PacketBuffer::kMaxSizeWithoutReserve];
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
     for (uint8_t & payload : lPayload)
     {
         payload = static_cast<uint8_t>(random());
@@ -1684,7 +1692,11 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
     // This is only testable on heap allocation configurations, where pbuf records the allocation size and we can manually
     // construct an oversize buffer.
 
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+    constexpr uint32_t kOversizeDataSize = CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES + 99;
+#else
     constexpr uint16_t kOversizeDataSize = PacketBuffer::kMaxSizeWithoutReserve + 99;
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
     PacketBuffer * p = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(kStructureSize + kOversizeDataSize));
     ASSERT_NE(p, nullptr);
 
@@ -1698,15 +1710,20 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
     PacketBufferHandle handle = PacketBufferHandle::Adopt(p);
 
     // Fill the buffer to maximum and verify that it can be cloned.
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+    uint32_t maxSize = CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES;
+#else
+    uint32_t maxSize = PacketBuffer::kMaxSizeWithoutReserve;
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
 
-    memset(handle->Start(), 1, PacketBuffer::kMaxSizeWithoutReserve);
-    handle->SetDataLength(PacketBuffer::kMaxSizeWithoutReserve);
-    EXPECT_EQ(handle->DataLength(), PacketBuffer::kMaxSizeWithoutReserve);
+    memset(handle->Start(), 1, maxSize);
+    handle->SetDataLength(maxSize);
+    EXPECT_EQ(handle->DataLength(), maxSize);
 
     PacketBufferHandle clone = handle.CloneData();
     ASSERT_FALSE(clone.IsNull());
-    EXPECT_EQ(clone->DataLength(), PacketBuffer::kMaxSizeWithoutReserve);
-    EXPECT_EQ(memcmp(handle->Start(), clone->Start(), PacketBuffer::kMaxSizeWithoutReserve), 0);
+    EXPECT_EQ(clone->DataLength(), maxSize);
+    EXPECT_EQ(memcmp(handle->Start(), clone->Start(), maxSize), 0);
 
     // Overfill the buffer and verify that it can not be cloned.
     memset(handle->Start(), 2, kOversizeDataSize);

diff --git a/src/transport/raw/TCP.cpp b/src/transport/raw/TCP.cpp
@@ -40,10 +40,10 @@ namespace {
 using namespace chip::Encoding;
 
 // Packets start with a 16-bit size
-constexpr size_t kPacketSizeBytes = 2;
+constexpr size_t kPacketSizeBytes = sizeof(uint32_t);
 
 // TODO: Actual limit may be lower (spec issue #2119)
-constexpr uint16_t kMaxMessageSize = static_cast<uint16_t>(System::PacketBuffer::kMaxSizeWithoutReserve - kPacketSizeBytes);
+constexpr uint32_t kMaxTCPMessageSize = static_cast<uint32_t>(CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES - kPacketSizeBytes);
 
 constexpr int kListenBacklogSize = 2;
 
@@ -202,7 +202,7 @@ CHIP_ERROR TCPBase::SendMessage(const Transport::PeerAddress & address, System::
 
     VerifyOrReturnError(address.GetTransportType() == Type::kTcp, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(mState == TCPState::kInitialized, CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrReturnError(kPacketSizeBytes + msgBuf->DataLength() <= std::numeric_limits<uint16_t>::max(),
+    VerifyOrReturnError(kPacketSizeBytes + msgBuf->DataLength() <= std::numeric_limits<uint32_t>::max(),
                         CHIP_ERROR_INVALID_ARGUMENT);
 
     // The check above about kPacketSizeBytes + msgBuf->DataLength() means it definitely fits in uint16_t.
@@ -211,7 +211,7 @@ CHIP_ERROR TCPBase::SendMessage(const Transport::PeerAddress & address, System::
     msgBuf->SetStart(msgBuf->Start() - kPacketSizeBytes);
 
     uint8_t * output = msgBuf->Start();
-    LittleEndian::Write16(output, static_cast<uint16_t>(msgBuf->DataLength() - kPacketSizeBytes));
+    LittleEndian::Write32(output, static_cast<uint32_t>(msgBuf->DataLength() - kPacketSizeBytes));
 
     // Reuse existing connection if one exists, otherwise a new one
     // will be established
@@ -324,11 +324,11 @@ CHIP_ERROR TCPBase::ProcessReceivedBuffer(Inet::TCPEndPoint * endPoint, const Pe
         {
             return err;
         }
-        uint16_t messageSize = LittleEndian::Get16(messageSizeBuf);
-        if (messageSize >= kMaxMessageSize)
+        uint32_t messageSize = LittleEndian::Get32(messageSizeBuf);
+        if (messageSize >= kMaxTCPMessageSize)
         {
-
-            // This message is too long for upper layers.
+            // Message is too big for node to process. Disconnect with peer.
+            CloseConnectionInternal(state, CHIP_ERROR_MESSAGE_TOO_LONG, SuppressCallback::No);
             return CHIP_ERROR_MESSAGE_TOO_LONG;
         }
         // The subtraction will not underflow because we successfully read kPacketSizeBytes.
@@ -344,7 +344,7 @@ CHIP_ERROR TCPBase::ProcessReceivedBuffer(Inet::TCPEndPoint * endPoint, const Pe
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR TCPBase::ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, uint16_t messageSize)
+CHIP_ERROR TCPBase::ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, size_t messageSize)
 {
     // We enter with `state->mReceived` containing at least one full message, perhaps in a chain.
     // `state->mReceived->Start()` currently points to the message data.

diff --git a/src/transport/raw/TCP.h b/src/transport/raw/TCP.h
@@ -252,7 +252,7 @@ class DLL_EXPORT TCPBase : public Base
      *                              is no other data).
      * @param[in]     messageSize   Size of the single message.
      */
-    CHIP_ERROR ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, uint16_t messageSize);
+    CHIP_ERROR ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, size_t messageSize);
 
     /**
      * Initiate a connection to the given peer. On connection completion,

diff --git a/src/transport/raw/TCPConfig.h b/src/transport/raw/TCPConfig.h
@@ -69,7 +69,7 @@ namespace chip {
  * @brief Maximum payload size of a message over a TCP connection
  */
 #ifndef CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES
-#define CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES 1000000
+#define CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES (CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES)
 #endif
 
 /**