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 15, 2024 · 61a4a0d · 61a4a0d
1 parent 1b455b5
commit 61a4a0d
Show file tree

Hide file tree

Showing 9 changed files with 98 additions and 62 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 <= System::PacketBuffer::kLargeBufMaxSize);
             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/SystemConfig.h b/src/system/SystemConfig.h
@@ -771,3 +771,12 @@ struct LwIPEvent;
 #define CHIP_SYSTEM_CONFIG_USE_ZEPHYR_EVENTFD 0
 #endif
 #endif // CHIP_SYSTEM_CONFIG_USE_ZEPHYR_EVENTFD
+
+/**
+ * @def CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES
+ *
+ * @brief Maximum buffer allocation size of a 'Large' message
+ */
+#ifndef CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES
+#define CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES (64000)
+#endif
diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp
@@ -514,7 +514,9 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
     static_assert(PacketBuffer::kStructureSize == sizeof(PacketBuffer), "PacketBuffer size mismatch");
     static_assert(PacketBuffer::kStructureSize < UINT16_MAX, "Check for overflow more carefully");
     static_assert(SIZE_MAX >= INT_MAX, "Our additions might not fit in size_t");
-    static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT32_MAX, "PacketBuffer may have size not fitting uint32_t");
+    static_assert(PacketBuffer::kLargeBufMaxSizeWithoutReserve <= UINT32_MAX, "Max size for Large payload buffers");
+    static_assert(PacketBuffer::kMaxSizeWithoutReserve < PacketBuffer::kLargeBufMaxSizeWithoutReserve,
+                  "Large buffer configuration should be greater than the conventional buffer limit");
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
     // LwIP based APIs have a maximum buffer size of UINT16_MAX. Ensure that
     // limit is met during allocation.
@@ -530,8 +532,7 @@ 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 (lAllocSize > PacketBuffer::kMaxAllocSize)
     {
         ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
         return PacketBufferHandle();
@@ -593,18 +594,15 @@ 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 (aDataSize > UINT16_MAX)
-    {
-        ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
-        return PacketBufferHandle();
-    }
     // Since `aDataSize` fits in uint16_t, the sum `aDataSize + aAdditionalSize` will not overflow.
     // `New()` will only return a non-null buffer if the total allocation size does not overflow.
     PacketBufferHandle buffer = New(aDataSize + aAdditionalSize, aReservedSize);
     if (buffer.mBuffer != nullptr)
     {
         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 +725,20 @@ PacketBufferHandle PacketBufferHandle::CloneData() const
         size_t originalDataSize       = original->MaxDataLength();
         uint16_t originalReservedSize = original->ReservedSize();
 
-        if (originalDataSize + originalReservedSize > PacketBuffer::kMaxSizeWithoutReserve)
+        uint32_t maxSize              = PacketBuffer::kMaxAllocSize;
+
+        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
@@ -138,6 +138,18 @@ class DLL_EXPORT PacketBuffer : private pbuf
      */
     static constexpr size_t kMaxSize = kMaxSizeWithoutReserve - kDefaultHeaderReserve;
 
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+    static constexpr size_t kLargeBufMaxSizeWithoutReserve = CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES;
+
+    static constexpr size_t kLargeBufMaxSize = kLargeBufMaxSizeWithoutReserve - kDefaultHeaderReserve;
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
+
+#if INET_CONFIG_ENABLE_TCP_ENDPOINT
+    static constexpr size_t kMaxAllocSize = kLargeBufMaxSizeWithoutReserve;
+#else
+    static constexpr size_t kMaxAllocSize = kMaxSizeWithoutReserve;
+#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
+
     /**
      * Return the size of the allocation including the reserved and payload data spaces but not including space
      * allocated for the PacketBuffer structure.

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_SYSTEM_CONFIG_MAX_LARGE_BUFFER_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_SYSTEM_CONFIG_MAX_LARGE_BUFFER_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_SYSTEM_CONFIG_MAX_LARGE_BUFFER_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_SYSTEM_CONFIG_MAX_LARGE_BUFFER_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,9 @@ 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_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES - kPacketSizeBytes);
 
 constexpr int kListenBacklogSize = 2;
 
@@ -202,7 +201,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 +210,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 +323,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 +343,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
@@ -258,7 +258,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,
@@ -308,7 +308,7 @@ class DLL_EXPORT TCPBase : public Base
 
     // The max payload size of data over a TCP connection that is transmissible
     // at a time.
-    uint32_t mMaxTCPPayloadSize = CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES;
+    uint32_t mMaxTCPSendAllocSize = System::PacketBuffer::kMaxAllocSize;
 
     // Number of active and 'pending connection' endpoints
     size_t mUsedEndPointCount = 0;

diff --git a/src/transport/raw/TCPConfig.h b/src/transport/raw/TCPConfig.h
@@ -63,15 +63,6 @@ namespace chip {
 #define CHIP_CONFIG_MAX_TCP_PENDING_PACKETS 4
 #endif
 
-/**
- * @def CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES
- *
- * @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
-#endif
-
 /**
  *  @def CHIP_CONFIG_TCP_CONNECT_TIMEOUT_MSECS
  *