From 9219236ca759115f7007a403c3e7890942d45835 Mon Sep 17 00:00:00 2001 From: Pradip De Date: Wed, 5 Jun 2024 12:05:25 -0700 Subject: [PATCH] Changes for large Packetbuffer allocation to support TCP payloads (#33308) * 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. Define Max application payload size for large buffers Test modifications for chainedbuffer receives for TCP. - Add test for Buffer length greater than MRP max size. - Disable TCP on nrfconnect platform because of resource requirements. Stack allocations for large buffer with default size is exceeding limits. Disabling the Test file altogether for this platform would prevent all tests from running. Instead, only disabling TCP on nrfConnect for now, since it is unused. Fixes #31779. * Update src/system/SystemPacketBuffer.cpp Co-authored-by: Boris Zbarsky * Update src/transport/raw/TCP.cpp Co-authored-by: Boris Zbarsky * Restyle fix --------- Co-authored-by: Boris Zbarsky --- config/nrfconnect/chip-module/CMakeLists.txt | 2 +- src/inet/TCPEndPoint.h | 2 +- src/inet/TCPEndPointImplSockets.cpp | 5 +-- src/system/SystemConfig.h | 18 ++++++++ src/system/SystemPacketBuffer.cpp | 41 +++++++++++++----- src/system/SystemPacketBuffer.h | 24 ++++++++++- src/system/tests/TestSystemPacketBuffer.cpp | 18 ++++---- src/transport/SecureMessageCodec.cpp | 1 - src/transport/SessionManager.cpp | 9 ++++ src/transport/raw/MessageHeader.h | 11 ++++- src/transport/raw/TCP.cpp | 27 ++++++------ src/transport/raw/TCP.h | 6 +-- src/transport/raw/TCPConfig.h | 9 ---- src/transport/raw/tests/TestTCP.cpp | 44 ++++++++++++-------- 14 files changed, 145 insertions(+), 72 deletions(-) diff --git a/config/nrfconnect/chip-module/CMakeLists.txt b/config/nrfconnect/chip-module/CMakeLists.txt index 51ca4689de1ca1..29560db74f38cc 100644 --- a/config/nrfconnect/chip-module/CMakeLists.txt +++ b/config/nrfconnect/chip-module/CMakeLists.txt @@ -129,7 +129,7 @@ matter_add_gn_arg_bool ("chip_enable_nfc" CONFIG_CHIP_NF matter_add_gn_arg_bool ("chip_enable_ota_requestor" CONFIG_CHIP_OTA_REQUESTOR) matter_add_gn_arg_bool ("chip_persist_subscriptions" CONFIG_CHIP_PERSISTENT_SUBSCRIPTIONS) matter_add_gn_arg_bool ("chip_monolithic_tests" CONFIG_CHIP_BUILD_TESTS) -matter_add_gn_arg_bool ("chip_inet_config_enable_tcp_endpoint" CONFIG_CHIP_BUILD_TESTS) +matter_add_gn_arg_bool ("chip_inet_config_enable_tcp_endpoint" FALSE) matter_add_gn_arg_bool ("chip_error_logging" CONFIG_MATTER_LOG_LEVEL GREATER_EQUAL 1) matter_add_gn_arg_bool ("chip_progress_logging" CONFIG_MATTER_LOG_LEVEL GREATER_EQUAL 3) matter_add_gn_arg_bool ("chip_detail_logging" CONFIG_MATTER_LOG_LEVEL GREATER_EQUAL 4) diff --git a/src/inet/TCPEndPoint.h b/src/inet/TCPEndPoint.h index 8fc6a6338d4140..57652349cfa83a 100644 --- a/src/inet/TCPEndPoint.h +++ b/src/inet/TCPEndPoint.h @@ -522,7 +522,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis /** * Size of the largest TCP packet that can be received. */ - constexpr static size_t kMaxReceiveMessageSize = System::PacketBuffer::kMaxSizeWithoutReserve; + constexpr static size_t kMaxReceiveMessageSize = System::PacketBuffer::kMaxAllocSize; protected: friend class TCPTest; diff --git a/src/inet/TCPEndPointImplSockets.cpp b/src/inet/TCPEndPointImplSockets.cpp index d21c11b6a42865..6b8965b19d2b4b 100644 --- a/src/inet/TCPEndPointImplSockets.cpp +++ b/src/inet/TCPEndPointImplSockets.cpp @@ -947,10 +947,9 @@ void TCPEndPointImplSockets::ReceiveData() { VerifyOrDie(rcvLen > 0); size_t newDataLength = rcvBuf->DataLength() + static_cast(rcvLen); - VerifyOrDie(CanCastTo(newDataLength)); if (isNewBuf) { - rcvBuf->SetDataLength(static_cast(newDataLength)); + rcvBuf->SetDataLength(newDataLength); rcvBuf.RightSize(); if (mRcvQueue.IsNull()) { @@ -963,7 +962,7 @@ void TCPEndPointImplSockets::ReceiveData() } else { - rcvBuf->SetDataLength(static_cast(newDataLength), mRcvQueue); + rcvBuf->SetDataLength(newDataLength, mRcvQueue); } } } diff --git a/src/system/SystemConfig.h b/src/system/SystemConfig.h index e91f59298f683e..b37cfe555278dd 100644 --- a/src/system/SystemConfig.h +++ b/src/system/SystemConfig.h @@ -788,3 +788,21 @@ 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 + * + * This is the default maximum capacity (including both data and reserve + * space) of a large PacketBuffer(exceeding the IPv6 MTU of 1280 bytes). + * This shall be used over transports, such as TCP, that support large + * payload transfers. Fetching of large command responses or wildcard + * subscription responses may leverage this increased bandwidth transfer. + * Individual systems may override this size based on their requirements. + * Data transfers over MRP should not be using this size for allocating + * buffers as they are restricted by the IPv6 MTU. + */ +#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 index f8dda8b01ab84e..9f29d2755908cd 100644 --- a/src/system/SystemPacketBuffer.cpp +++ b/src/system/SystemPacketBuffer.cpp @@ -515,6 +515,21 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese // Setting a static upper bound on the maximum buffer size allocation for regular sized messages (not large). static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT16_MAX, "kMaxSizeWithoutReserve should not exceed UINT16_MAX."); +#if INET_CONFIG_ENABLE_TCP_ENDPOINT + // Setting a static upper bound on the maximum buffer size allocation for + // large messages. +#if CHIP_SYSTEM_CONFIG_USE_LWIP + // LwIP based systems are internally limited to using a u16_t type as the size of a buffer. + static_assert(PacketBuffer::kLargeBufMaxSizeWithoutReserve <= UINT16_MAX, + "In LwIP, max size for Large payload buffers cannot exceed UINT16_MAX!"); +#else + // Messages over TCP are framed using a length field that is 32 bits in + // length. + static_assert(PacketBuffer::kLargeBufMaxSizeWithoutReserve <= UINT32_MAX, + "Max size for Large payload buffers cannot exceed UINT32_MAX"); +#endif // CHIP_SYSTEM_CONFIG_USE_LWIP +#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT + // Ensure that aAvailableSize is bound within a max and is not big enough to cause overflow during // subsequent addition of all the sizes. if (aAvailableSize > UINT32_MAX) @@ -547,7 +562,14 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese #if CHIP_SYSTEM_CONFIG_USE_LWIP // LwIP based APIs have a maximum buffer size of UINT16_MAX. Ensure that // limit is met during allocation. - VerifyOrDieWithMsg(sumOfAvailAndReserved < UINT16_MAX, chipSystemLayer, "LwIP based systems can handle only up to UINT16_MAX!"); + if (sumOfAvailAndReserved > UINT16_MAX) + { + ChipLogError(chipSystemLayer, + "LwIP based systems require total buffer size to be less than UINT16_MAX!" + "Attempted allocation size = " ChipLogFormatX64, + ChipLogValueX64(sumOfAvailAndReserved)); + return PacketBufferHandle(); + } #endif // CHIP_SYSTEM_CONFIG_USE_LWIP // sumOfAvailAndReserved is no larger than sumOfSizes, which we checked can be cast to @@ -557,7 +579,7 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle()); - if (lAllocSize > PacketBuffer::kMaxSizeWithoutReserve) + if (lAllocSize > PacketBuffer::kMaxAllocSize) { ChipLogError(chipSystemLayer, "PacketBuffer: allocation exceeding buffer capacity limits."); return PacketBufferHandle(); @@ -621,11 +643,6 @@ 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); @@ -633,6 +650,8 @@ PacketBufferHandle PacketBufferHandle::NewWithData(const void * aData, size_t aD { memcpy(buffer.mBuffer->payload, aData, aDataSize); #if CHIP_SYSTEM_CONFIG_USE_LWIP + // Checks in the New() call catch buffer allocations greater + // than UINT16_MAX for LwIP based platforms. buffer.mBuffer->len = buffer.mBuffer->tot_len = static_cast(aDataSize); #else buffer.mBuffer->len = buffer.mBuffer->tot_len = aDataSize; @@ -755,18 +774,18 @@ PacketBufferHandle PacketBufferHandle::CloneData() const size_t originalDataSize = original->MaxDataLength(); uint16_t originalReservedSize = original->ReservedSize(); - if (originalDataSize + originalReservedSize > PacketBuffer::kMaxSizeWithoutReserve) + if (originalDataSize + originalReservedSize > PacketBuffer::kMaxAllocSize) { // 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() > PacketBuffer::kMaxAllocSize) { 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 <= PacketBuffer::kMaxAllocSize. + originalDataSize = PacketBuffer::kMaxAllocSize - originalReservedSize; } PacketBufferHandle clone = PacketBufferHandle::New(originalDataSize, originalReservedSize); diff --git a/src/system/SystemPacketBuffer.h b/src/system/SystemPacketBuffer.h index 41eaef6d9e305c..5068822c7f22a4 100644 --- a/src/system/SystemPacketBuffer.h +++ b/src/system/SystemPacketBuffer.h @@ -119,7 +119,7 @@ class DLL_EXPORT PacketBuffer : private pbuf public: /** - * The maximum size buffer an application can allocate with no protocol header reserve. + * The maximum size of a regular buffer an application can allocate with no protocol header reserve. */ #if CHIP_SYSTEM_CONFIG_USE_LWIP static constexpr size_t kMaxSizeWithoutReserve = LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE); @@ -134,10 +134,30 @@ class DLL_EXPORT PacketBuffer : private pbuf static constexpr uint16_t kDefaultHeaderReserve = CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE; /** - * The maximum size buffer an application can allocate with the default protocol header reserve. + * The maximum size of a regular buffer an application can allocate with the default protocol header reserve. */ static constexpr size_t kMaxSize = kMaxSizeWithoutReserve - kDefaultHeaderReserve; + /** + * The maximum size of a large buffer(> IPv6 MTU) that an application can allocate with no protocol header reserve. + */ + static constexpr size_t kLargeBufMaxSizeWithoutReserve = CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES; + + /** + * The maximum size of a large buffer(> IPv6 MTU) that an application can allocate with the default protocol header reserve. + */ + static constexpr size_t kLargeBufMaxSize = kLargeBufMaxSizeWithoutReserve - kDefaultHeaderReserve; + + /** + * Unified constant(both regular and large buffers) for the maximum size that an application can allocate with no + * protocol header reserve. + */ +#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 index bf1456487307a3..e445eed17a38e9 100644 --- a/src/system/tests/TestSystemPacketBuffer.cpp +++ b/src/system/tests/TestSystemPacketBuffer.cpp @@ -300,7 +300,7 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckNew) { const PacketBufferHandle buffer = PacketBufferHandle::New(0, config.reserved_size); - if (config.reserved_size > PacketBuffer::kMaxSizeWithoutReserve) + if (config.reserved_size > PacketBuffer::kMaxAllocSize) { EXPECT_TRUE(buffer.IsNull()); continue; @@ -1596,7 +1596,8 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleRightSize) TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData) { - uint8_t lPayload[2 * PacketBuffer::kMaxSizeWithoutReserve]; + uint8_t lPayload[2 * PacketBuffer::kMaxAllocSize]; + for (uint8_t & payload : lPayload) { payload = static_cast(random()); @@ -1675,7 +1676,7 @@ 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. - constexpr uint16_t kOversizeDataSize = PacketBuffer::kMaxSizeWithoutReserve + 99; + constexpr size_t kOversizeDataSize = PacketBuffer::kMaxAllocSize + 99; PacketBuffer * p = reinterpret_cast(chip::Platform::MemoryAlloc(kStructureSize + kOversizeDataSize)); ASSERT_NE(p, nullptr); @@ -1689,15 +1690,16 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData) PacketBufferHandle handle = PacketBufferHandle::Adopt(p); // Fill the buffer to maximum and verify that it can be cloned. + size_t maxSize = PacketBuffer::kMaxAllocSize; - 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/SecureMessageCodec.cpp b/src/transport/SecureMessageCodec.cpp index 3b70026c5681d6..263b4e5e127a5a 100644 --- a/src/transport/SecureMessageCodec.cpp +++ b/src/transport/SecureMessageCodec.cpp @@ -41,7 +41,6 @@ CHIP_ERROR Encrypt(const CryptoContext & context, CryptoContext::ConstNonceView { VerifyOrReturnError(!msgBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(!msgBuf->HasChainedBuffer(), CHIP_ERROR_INVALID_MESSAGE_LENGTH); - VerifyOrReturnError(msgBuf->TotalLength() <= kMaxAppMessageLen, CHIP_ERROR_MESSAGE_TOO_LONG); ReturnErrorOnFailure(payloadHeader.EncodeBeforeData(msgBuf)); diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index d49e66fb6f5116..00acc485d47d23 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -201,6 +201,15 @@ CHIP_ERROR SessionManager::PrepareMessage(const SessionHandle & sessionHandle, P packetHeader.SetSecureSessionControlMsg(true); } + if (sessionHandle->AllowsLargePayload()) + { + VerifyOrReturnError(message->TotalLength() <= kMaxLargeAppMessageLen, CHIP_ERROR_MESSAGE_TOO_LONG); + } + else + { + VerifyOrReturnError(message->TotalLength() <= kMaxAppMessageLen, CHIP_ERROR_MESSAGE_TOO_LONG); + } + #if CHIP_PROGRESS_LOGGING NodeId destination; FabricIndex fabricIndex; diff --git a/src/transport/raw/MessageHeader.h b/src/transport/raw/MessageHeader.h index ef4f86d3993793..2752a5c86c82c2 100644 --- a/src/transport/raw/MessageHeader.h +++ b/src/transport/raw/MessageHeader.h @@ -60,7 +60,6 @@ static constexpr size_t kMaxPacketBufferApplicationPayloadAndMICSizeBytes = Syst static constexpr size_t kMaxApplicationPayloadAndMICSizeBytes = min(kMaxPerSpecApplicationPayloadAndMICSizeBytes, kMaxPacketBufferApplicationPayloadAndMICSizeBytes); - } // namespace detail static constexpr size_t kMaxTagLen = 16; @@ -74,6 +73,16 @@ static constexpr size_t kMaxAppMessageLen = detail::kMaxApplicationPayloadAndMIC static constexpr uint16_t kMsgUnicastSessionIdUnsecured = 0x0000; +// Minimum header size of TCP + IPv6 without options. +static constexpr size_t kMaxTCPAndIPHeaderSizeBytes = 60; + +// Max space for the Application Payload and MIC for large packet buffers +// This is the size _excluding_ the header reserve. +static constexpr size_t kMaxLargeApplicationPayloadAndMICSizeBytes = + System::PacketBuffer::kLargeBufMaxSize - kMaxTCPAndIPHeaderSizeBytes; + +static constexpr size_t kMaxLargeAppMessageLen = kMaxLargeApplicationPayloadAndMICSizeBytes - kMaxTagLen; + typedef int PacketHeaderFlags; namespace Header { diff --git a/src/transport/raw/TCP.cpp b/src/transport/raw/TCP.cpp index e33590a63a6fdf..b928c3e91cccae 100644 --- a/src/transport/raw/TCP.cpp +++ b/src/transport/raw/TCP.cpp @@ -39,11 +39,15 @@ namespace { using namespace chip::Encoding; -// Packets start with a 16-bit size -constexpr size_t kPacketSizeBytes = 2; +// Packets start with a 32-bit size field. +constexpr size_t kPacketSizeBytes = 4; -// TODO: Actual limit may be lower (spec issue #2119) -constexpr uint16_t kMaxMessageSize = static_cast(System::PacketBuffer::kMaxSizeWithoutReserve - kPacketSizeBytes); +static_assert(System::PacketBuffer::kLargeBufMaxSizeWithoutReserve <= UINT32_MAX, "Cast below could truncate the value"); +static_assert(System::PacketBuffer::kLargeBufMaxSizeWithoutReserve >= kPacketSizeBytes, + "Large buffer allocation should be large enough to hold the length field"); + +constexpr uint32_t kMaxTCPMessageSize = + static_cast(System::PacketBuffer::kLargeBufMaxSizeWithoutReserve - kPacketSizeBytes); constexpr int kListenBacklogSize = 2; @@ -197,21 +201,21 @@ ActiveTCPConnectionState * TCPBase::FindInUseConnection(const Inet::TCPEndPoint CHIP_ERROR TCPBase::SendMessage(const Transport::PeerAddress & address, System::PacketBufferHandle && msgBuf) { // Sent buffer data format is: - // - packet size as a uint16_t + // - packet size as a uint32_t // - actual data VerifyOrReturnError(address.GetTransportType() == Type::kTcp, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(mState == TCPState::kInitialized, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(kPacketSizeBytes + msgBuf->DataLength() <= std::numeric_limits::max(), + VerifyOrReturnError(kPacketSizeBytes + msgBuf->DataLength() <= System::PacketBuffer::kLargeBufMaxSizeWithoutReserve, CHIP_ERROR_INVALID_ARGUMENT); - // The check above about kPacketSizeBytes + msgBuf->DataLength() means it definitely fits in uint16_t. + static_assert(kPacketSizeBytes <= UINT16_MAX); VerifyOrReturnError(msgBuf->EnsureReservedSize(static_cast(kPacketSizeBytes)), CHIP_ERROR_NO_MEMORY); msgBuf->SetStart(msgBuf->Start() - kPacketSizeBytes); uint8_t * output = msgBuf->Start(); - LittleEndian::Write16(output, static_cast(msgBuf->DataLength() - kPacketSizeBytes)); + LittleEndian::Write32(output, static_cast(msgBuf->DataLength() - kPacketSizeBytes)); // Reuse existing connection if one exists, otherwise a new one // will be established @@ -324,10 +328,9 @@ 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. return CHIP_ERROR_MESSAGE_TOO_LONG; } @@ -344,7 +347,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 index 783f3844d2d16d..1cb7aa9634ba16 100644 --- 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, @@ -306,10 +306,6 @@ class DLL_EXPORT TCPBase : public Base // giving up. uint32_t mConnectTimeout = CHIP_CONFIG_TCP_CONNECT_TIMEOUT_MSECS; - // 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; - // 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 index d54a9466b4d294..4f95978985fd63 100644 --- 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 * diff --git a/src/transport/raw/tests/TestTCP.cpp b/src/transport/raw/tests/TestTCP.cpp index f5e343f1a2ea79..dbce3b8ce5ef43 100644 --- a/src/transport/raw/tests/TestTCP.cpp +++ b/src/transport/raw/tests/TestTCP.cpp @@ -51,7 +51,7 @@ namespace { constexpr size_t kMaxTcpActiveConnectionCount = 4; constexpr size_t kMaxTcpPendingPackets = 4; -constexpr uint16_t kPacketSizeBytes = static_cast(sizeof(uint16_t)); +constexpr size_t kPacketSizeBytes = sizeof(uint32_t); uint16_t gChipTCPPort = static_cast(CHIP_PORT + chip::Crypto::GetRandU16() % 100); chip::Transport::AppTCPConnectionCallbackCtxt gAppTCPConnCbCtxt; chip::Transport::ActiveTCPConnectionState * gActiveTCPConnState = nullptr; @@ -298,7 +298,7 @@ struct TestData // the last buffer will be made larger. TestData() : mPayload(nullptr), mTotalLength(0), mMessageLength(0), mMessageOffset(0) {} ~TestData() { Free(); } - bool Init(const uint16_t sizes[]); + bool Init(const uint32_t sizes[]); void Free(); bool IsValid() { return !mHandle.IsNull() && (mPayload != nullptr); } @@ -309,7 +309,7 @@ struct TestData size_t mMessageOffset; }; -bool TestData::Init(const uint16_t sizes[]) +bool TestData::Init(const uint32_t sizes[]) { Free(); @@ -325,17 +325,17 @@ bool TestData::Init(const uint16_t sizes[]) mTotalLength += sizes[bufferCount]; } --bufferCount; - uint16_t additionalLength = 0; + uint32_t additionalLength = 0; if (headerLength + kPacketSizeBytes > mTotalLength) { - additionalLength = static_cast((headerLength + kPacketSizeBytes) - mTotalLength); + additionalLength = static_cast((headerLength + kPacketSizeBytes) - mTotalLength); mTotalLength += additionalLength; } - if (mTotalLength > UINT16_MAX) + if (mTotalLength > UINT32_MAX) { return false; } - uint16_t messageLength = static_cast(mTotalLength - kPacketSizeBytes); + uint32_t messageLength = static_cast(mTotalLength - kPacketSizeBytes); // Build the test payload. uint8_t * payload = static_cast(chip::Platform::MemoryCalloc(1, mTotalLength)); @@ -343,7 +343,7 @@ bool TestData::Init(const uint16_t sizes[]) { return false; } - chip::Encoding::LittleEndian::Put16(payload, messageLength); + chip::Encoding::LittleEndian::Put32(payload, messageLength); uint16_t headerSize; CHIP_ERROR err = header.Encode(payload + kPacketSizeBytes, messageLength, &headerSize); if (err != CHIP_NO_ERROR) @@ -363,10 +363,10 @@ bool TestData::Init(const uint16_t sizes[]) System::PacketBufferHandle head = chip::System::PacketBufferHandle::New(sizes[0], 0 /* reserve */); for (int i = 1; i <= bufferCount; ++i) { - uint16_t size = sizes[i]; + size_t size = sizes[i]; if (i == bufferCount) { - size = static_cast(size + additionalLength); + size = size + additionalLength; } chip::System::PacketBufferHandle buffer = chip::System::PacketBufferHandle::New(size, 0 /* reserve */); if (buffer.IsNull()) @@ -395,7 +395,7 @@ bool TestData::Init(const uint16_t sizes[]) if (lToWriteToCurrentBuf != 0) { memcpy(iterator->Start(), writePayload, lToWriteToCurrentBuf); - iterator->SetDataLength(static_cast(iterator->DataLength() + lToWriteToCurrentBuf), head); + iterator->SetDataLength(iterator->DataLength() + lToWriteToCurrentBuf, head); writePayload += lToWriteToCurrentBuf; writeLength -= lToWriteToCurrentBuf; } @@ -634,22 +634,22 @@ TEST_F(TestTCP, CheckProcessReceivedBuffer) // Test a single packet buffer. gMockTransportMgrDelegate.mReceiveHandlerCallCount = 0; - EXPECT_TRUE(testData[0].Init((const uint16_t[]){ 111, 0 })); + EXPECT_TRUE(testData[0].Init((const uint32_t[]){ 111, 0 })); err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(testData[0].mHandle)); EXPECT_EQ(err, CHIP_NO_ERROR); EXPECT_EQ(gMockTransportMgrDelegate.mReceiveHandlerCallCount, 1); // Test a message in a chain of three packet buffers. The message length is split across buffers. gMockTransportMgrDelegate.mReceiveHandlerCallCount = 0; - EXPECT_TRUE(testData[0].Init((const uint16_t[]){ 1, 122, 123, 0 })); + EXPECT_TRUE(testData[0].Init((const uint32_t[]){ 1, 122, 123, 0 })); err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(testData[0].mHandle)); EXPECT_EQ(err, CHIP_NO_ERROR); EXPECT_EQ(gMockTransportMgrDelegate.mReceiveHandlerCallCount, 1); // Test two messages in a chain. gMockTransportMgrDelegate.mReceiveHandlerCallCount = 0; - EXPECT_TRUE(testData[0].Init((const uint16_t[]){ 131, 0 })); - EXPECT_TRUE(testData[1].Init((const uint16_t[]){ 132, 0 })); + EXPECT_TRUE(testData[0].Init((const uint32_t[]){ 131, 0 })); + EXPECT_TRUE(testData[1].Init((const uint32_t[]){ 132, 0 })); testData[0].mHandle->AddToEnd(std::move(testData[1].mHandle)); err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(testData[0].mHandle)); EXPECT_EQ(err, CHIP_NO_ERROR); @@ -657,17 +657,25 @@ TEST_F(TestTCP, CheckProcessReceivedBuffer) // Test a chain of two messages, each a chain. gMockTransportMgrDelegate.mReceiveHandlerCallCount = 0; - EXPECT_TRUE(testData[0].Init((const uint16_t[]){ 141, 142, 0 })); - EXPECT_TRUE(testData[1].Init((const uint16_t[]){ 143, 144, 0 })); + EXPECT_TRUE(testData[0].Init((const uint32_t[]){ 141, 142, 0 })); + EXPECT_TRUE(testData[1].Init((const uint32_t[]){ 143, 144, 0 })); testData[0].mHandle->AddToEnd(std::move(testData[1].mHandle)); err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(testData[0].mHandle)); EXPECT_EQ(err, CHIP_NO_ERROR); EXPECT_EQ(gMockTransportMgrDelegate.mReceiveHandlerCallCount, 2); + // Test a single packet buffer that is larger than + // kMaxSizeWithoutReserve but less than CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES. + gMockTransportMgrDelegate.mReceiveHandlerCallCount = 0; + EXPECT_TRUE(testData[0].Init((const uint32_t[]){ System::PacketBuffer::kMaxSizeWithoutReserve + 1, 0 })); + err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(testData[0].mHandle)); + EXPECT_EQ(err, CHIP_NO_ERROR); + EXPECT_EQ(gMockTransportMgrDelegate.mReceiveHandlerCallCount, 1); + // Test a message that is too large to coalesce into a single packet buffer. gMockTransportMgrDelegate.mReceiveHandlerCallCount = 0; gMockTransportMgrDelegate.SetCallback(TestDataCallbackCheck, &testData[1]); - EXPECT_TRUE(testData[0].Init((const uint16_t[]){ 51, System::PacketBuffer::kMaxSizeWithoutReserve, 0 })); + EXPECT_TRUE(testData[0].Init((const uint32_t[]){ 51, CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES, 0 })); // Sending only the first buffer of the long chain. This should be enough to trigger the error. System::PacketBufferHandle head = testData[0].mHandle.PopHead(); err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(head));