Skip to content

Commit fa93f3d

Browse files
committed
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. Fixes Issues #31779, #33307.
1 parent 3718e99 commit fa93f3d

13 files changed

+122
-68
lines changed

src/inet/TCPEndPoint.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -522,7 +522,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
522522
/**
523523
* Size of the largest TCP packet that can be received.
524524
*/
525-
constexpr static size_t kMaxReceiveMessageSize = System::PacketBuffer::kMaxSizeWithoutReserve;
525+
constexpr static size_t kMaxReceiveMessageSize = System::PacketBuffer::kLargeBufMaxSizeWithoutReserve;
526526

527527
protected:
528528
friend class TCPTest;

src/inet/TCPEndPointImplSockets.cpp

+2-3
Original file line numberDiff line numberDiff line change
@@ -947,10 +947,9 @@ void TCPEndPointImplSockets::ReceiveData()
947947
{
948948
VerifyOrDie(rcvLen > 0);
949949
size_t newDataLength = rcvBuf->DataLength() + static_cast<size_t>(rcvLen);
950-
VerifyOrDie(CanCastTo<uint16_t>(newDataLength));
951950
if (isNewBuf)
952951
{
953-
rcvBuf->SetDataLength(static_cast<uint16_t>(newDataLength));
952+
rcvBuf->SetDataLength(newDataLength);
954953
rcvBuf.RightSize();
955954
if (mRcvQueue.IsNull())
956955
{
@@ -963,7 +962,7 @@ void TCPEndPointImplSockets::ReceiveData()
963962
}
964963
else
965964
{
966-
rcvBuf->SetDataLength(static_cast<uint16_t>(newDataLength), mRcvQueue);
965+
rcvBuf->SetDataLength(newDataLength, mRcvQueue);
967966
}
968967
}
969968
}

src/system/SystemConfig.h

+9
Original file line numberDiff line numberDiff line change
@@ -788,3 +788,12 @@ struct LwIPEvent;
788788
#define CHIP_SYSTEM_CONFIG_USE_ZEPHYR_EVENTFD 0
789789
#endif
790790
#endif // CHIP_SYSTEM_CONFIG_USE_ZEPHYR_EVENTFD
791+
792+
/**
793+
* @def CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES
794+
*
795+
* @brief Maximum buffer allocation size of a 'Large' message
796+
*/
797+
#ifndef CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES
798+
#define CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES (64000)
799+
#endif

src/system/SystemPacketBuffer.cpp

+24-10
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,21 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
515515
// Setting a static upper bound on the maximum buffer size allocation for regular sized messages (not large).
516516
static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT16_MAX, "kMaxSizeWithoutReserve should not exceed UINT16_MAX.");
517517

518+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
519+
// Setting a static upper bound on the maximum buffer size allocation for
520+
// large messages.
521+
#if CHIP_SYSTEM_CONFIG_USE_LWIP
522+
static_assert(PacketBuffer::kLargeBufMaxSizeWithoutReserve <= UINT16_MAX, "In LwIP, max size for Large payload buffers cannot exceed UINT16_MAX!");
523+
#else
524+
static_assert(PacketBuffer::kLargeBufMaxSizeWithoutReserve <= UINT32_MAX, "Max size for Large payload buffers cannot exceed UINT32_MAX");
525+
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP
526+
527+
// Ensure that the definition of the max buffer allocation for regular
528+
// messages is less than that for large ones.
529+
static_assert(PacketBuffer::kMaxSizeWithoutReserve < PacketBuffer::kLargeBufMaxSizeWithoutReserve,
530+
"Large buffer configuration should be greater than the conventional buffer limit");
531+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
532+
518533
// Ensure that aAvailableSize is bound within a max and is not big enough to cause overflow during
519534
// subsequent addition of all the sizes.
520535
if (aAvailableSize > UINT32_MAX)
@@ -557,7 +572,7 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
557572

558573
CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle());
559574

560-
if (lAllocSize > PacketBuffer::kMaxSizeWithoutReserve)
575+
if (lAllocSize > PacketBuffer::kMaxAllocSize)
561576
{
562577
ChipLogError(chipSystemLayer, "PacketBuffer: allocation exceeding buffer capacity limits.");
563578
return PacketBufferHandle();
@@ -621,18 +636,15 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
621636
PacketBufferHandle PacketBufferHandle::NewWithData(const void * aData, size_t aDataSize, size_t aAdditionalSize,
622637
uint16_t aReservedSize)
623638
{
624-
if (aDataSize > UINT16_MAX)
625-
{
626-
ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
627-
return PacketBufferHandle();
628-
}
629639
// Since `aDataSize` fits in uint16_t, the sum `aDataSize + aAdditionalSize` will not overflow.
630640
// `New()` will only return a non-null buffer if the total allocation size does not overflow.
631641
PacketBufferHandle buffer = New(aDataSize + aAdditionalSize, aReservedSize);
632642
if (buffer.mBuffer != nullptr)
633643
{
634644
memcpy(buffer.mBuffer->payload, aData, aDataSize);
635645
#if CHIP_SYSTEM_CONFIG_USE_LWIP
646+
// The VerifyOrDie() in the New() call catches buffer allocations greater
647+
// than UINT16_MAX for LwIP based platforms.
636648
buffer.mBuffer->len = buffer.mBuffer->tot_len = static_cast<uint16_t>(aDataSize);
637649
#else
638650
buffer.mBuffer->len = buffer.mBuffer->tot_len = aDataSize;
@@ -755,18 +767,20 @@ PacketBufferHandle PacketBufferHandle::CloneData() const
755767
size_t originalDataSize = original->MaxDataLength();
756768
uint16_t originalReservedSize = original->ReservedSize();
757769

758-
if (originalDataSize + originalReservedSize > PacketBuffer::kMaxSizeWithoutReserve)
770+
uint32_t maxSize = PacketBuffer::kMaxAllocSize;
771+
772+
if (originalDataSize + originalReservedSize > maxSize)
759773
{
760774
// The original memory allocation may have provided a larger block than requested (e.g. when using a shared pool),
761775
// and in particular may have provided a larger block than we are able to request from PackBufferHandle::New().
762776
// It is a genuine error if that extra space has been used.
763-
if (originalReservedSize + original->DataLength() > PacketBuffer::kMaxSizeWithoutReserve)
777+
if (originalReservedSize + original->DataLength() > maxSize)
764778
{
765779
return PacketBufferHandle();
766780
}
767781
// Otherwise, reduce the requested data size. This subtraction can not underflow because the above test
768-
// guarantees originalReservedSize <= PacketBuffer::kMaxSizeWithoutReserve.
769-
originalDataSize = PacketBuffer::kMaxSizeWithoutReserve - originalReservedSize;
782+
// guarantees originalReservedSize <= maxSize.
783+
originalDataSize = maxSize - originalReservedSize;
770784
}
771785

772786
PacketBufferHandle clone = PacketBufferHandle::New(originalDataSize, originalReservedSize);

src/system/SystemPacketBuffer.h

+12
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,18 @@ class DLL_EXPORT PacketBuffer : private pbuf
138138
*/
139139
static constexpr size_t kMaxSize = kMaxSizeWithoutReserve - kDefaultHeaderReserve;
140140

141+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
142+
static constexpr size_t kLargeBufMaxSizeWithoutReserve = CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES;
143+
144+
static constexpr size_t kLargeBufMaxSize = kLargeBufMaxSizeWithoutReserve - kDefaultHeaderReserve;
145+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
146+
147+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
148+
static constexpr size_t kMaxAllocSize = kLargeBufMaxSizeWithoutReserve;
149+
#else
150+
static constexpr size_t kMaxAllocSize = kMaxSizeWithoutReserve;
151+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
152+
141153
/**
142154
* Return the size of the allocation including the reserved and payload data spaces but not including space
143155
* allocated for the PacketBuffer structure.

src/system/tests/TestSystemPacketBuffer.cpp

+10-8
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckNew)
309309
{
310310
const PacketBufferHandle buffer = PacketBufferHandle::New(0, config.reserved_size);
311311

312-
if (config.reserved_size > PacketBuffer::kMaxSizeWithoutReserve)
312+
if (config.reserved_size > PacketBuffer::kMaxAllocSize)
313313
{
314314
EXPECT_TRUE(buffer.IsNull());
315315
continue;
@@ -1605,7 +1605,8 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleRightSize)
16051605

16061606
TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
16071607
{
1608-
uint8_t lPayload[2 * PacketBuffer::kMaxSizeWithoutReserve];
1608+
uint8_t lPayload[2 * PacketBuffer::kMaxAllocSize];
1609+
16091610
for (uint8_t & payload : lPayload)
16101611
{
16111612
payload = static_cast<uint8_t>(random());
@@ -1684,7 +1685,7 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
16841685
// This is only testable on heap allocation configurations, where pbuf records the allocation size and we can manually
16851686
// construct an oversize buffer.
16861687

1687-
constexpr uint16_t kOversizeDataSize = PacketBuffer::kMaxSizeWithoutReserve + 99;
1688+
constexpr size_t kOversizeDataSize = PacketBuffer::kMaxAllocSize + 99;
16881689
PacketBuffer * p = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(kStructureSize + kOversizeDataSize));
16891690
ASSERT_NE(p, nullptr);
16901691

@@ -1698,15 +1699,16 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
16981699
PacketBufferHandle handle = PacketBufferHandle::Adopt(p);
16991700

17001701
// Fill the buffer to maximum and verify that it can be cloned.
1702+
size_t maxSize = PacketBuffer::kMaxAllocSize;
17011703

1702-
memset(handle->Start(), 1, PacketBuffer::kMaxSizeWithoutReserve);
1703-
handle->SetDataLength(PacketBuffer::kMaxSizeWithoutReserve);
1704-
EXPECT_EQ(handle->DataLength(), PacketBuffer::kMaxSizeWithoutReserve);
1704+
memset(handle->Start(), 1, maxSize);
1705+
handle->SetDataLength(maxSize);
1706+
EXPECT_EQ(handle->DataLength(), maxSize);
17051707

17061708
PacketBufferHandle clone = handle.CloneData();
17071709
ASSERT_FALSE(clone.IsNull());
1708-
EXPECT_EQ(clone->DataLength(), PacketBuffer::kMaxSizeWithoutReserve);
1709-
EXPECT_EQ(memcmp(handle->Start(), clone->Start(), PacketBuffer::kMaxSizeWithoutReserve), 0);
1710+
EXPECT_EQ(clone->DataLength(), maxSize);
1711+
EXPECT_EQ(memcmp(handle->Start(), clone->Start(), maxSize), 0);
17101712

17111713
// Overfill the buffer and verify that it can not be cloned.
17121714
memset(handle->Start(), 2, kOversizeDataSize);

src/transport/SecureMessageCodec.cpp

-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ CHIP_ERROR Encrypt(const CryptoContext & context, CryptoContext::ConstNonceView
4141
{
4242
VerifyOrReturnError(!msgBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);
4343
VerifyOrReturnError(!msgBuf->HasChainedBuffer(), CHIP_ERROR_INVALID_MESSAGE_LENGTH);
44-
VerifyOrReturnError(msgBuf->TotalLength() <= kMaxAppMessageLen, CHIP_ERROR_MESSAGE_TOO_LONG);
4544

4645
ReturnErrorOnFailure(payloadHeader.EncodeBeforeData(msgBuf));
4746

src/transport/SessionManager.cpp

+11
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,17 @@ CHIP_ERROR SessionManager::PrepareMessage(const SessionHandle & sessionHandle, P
201201
packetHeader.SetSecureSessionControlMsg(true);
202202
}
203203

204+
if (sessionHandle->AllowsMRP())
205+
{
206+
VerifyOrReturnError(message->TotalLength() <= kMaxAppMessageLen, CHIP_ERROR_MESSAGE_TOO_LONG);
207+
}
208+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
209+
else if (sessionHandle->AllowsLargePayload())
210+
{
211+
VerifyOrReturnError(message->TotalLength() <= kMaxLargeAppMessageLen, CHIP_ERROR_MESSAGE_TOO_LONG);
212+
}
213+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
214+
204215
#if CHIP_PROGRESS_LOGGING
205216
NodeId destination;
206217
FabricIndex fabricIndex;

src/transport/raw/MessageHeader.h

+11-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@ static constexpr size_t kMaxPacketBufferApplicationPayloadAndMICSizeBytes = Syst
6060

6161
static constexpr size_t kMaxApplicationPayloadAndMICSizeBytes =
6262
min(kMaxPerSpecApplicationPayloadAndMICSizeBytes, kMaxPacketBufferApplicationPayloadAndMICSizeBytes);
63-
6463
} // namespace detail
6564

6665
static constexpr size_t kMaxTagLen = 16;
@@ -74,6 +73,17 @@ static constexpr size_t kMaxAppMessageLen = detail::kMaxApplicationPayloadAndMIC
7473

7574
static constexpr uint16_t kMsgUnicastSessionIdUnsecured = 0x0000;
7675

76+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
77+
// Minimum header size of TCP + IPv6 without options.
78+
static constexpr size_t kMaxTCPAndIPHeaderSizeBytes = 60;
79+
80+
// Max space for the Application Payload and MIC for large packet buffers
81+
// This is the size _excluding_ the header reserve.
82+
static constexpr size_t kMaxLargeApplicationPayloadAndMICSizeBytes = System::PacketBuffer::kLargeBufMaxSize - kMaxTCPAndIPHeaderSizeBytes;
83+
84+
static constexpr size_t kMaxLargeAppMessageLen = kMaxLargeApplicationPayloadAndMICSizeBytes - kMaxTagLen;
85+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
86+
7787
typedef int PacketHeaderFlags;
7888

7989
namespace Header {

src/transport/raw/TCP.cpp

+11-12
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,10 @@ namespace {
3939

4040
using namespace chip::Encoding;
4141

42-
// Packets start with a 16-bit size
43-
constexpr size_t kPacketSizeBytes = 2;
42+
// Packets start with a 32-bit size field.
43+
constexpr size_t kPacketSizeBytes = sizeof(uint32_t);
4444

45-
// TODO: Actual limit may be lower (spec issue #2119)
46-
constexpr uint16_t kMaxMessageSize = static_cast<uint16_t>(System::PacketBuffer::kMaxSizeWithoutReserve - kPacketSizeBytes);
45+
constexpr uint32_t kMaxTCPMessageSize = static_cast<uint32_t>(System::PacketBuffer::kLargeBufMaxSizeWithoutReserve - kPacketSizeBytes);
4746

4847
constexpr int kListenBacklogSize = 2;
4948

@@ -197,12 +196,12 @@ ActiveTCPConnectionState * TCPBase::FindInUseConnection(const Inet::TCPEndPoint
197196
CHIP_ERROR TCPBase::SendMessage(const Transport::PeerAddress & address, System::PacketBufferHandle && msgBuf)
198197
{
199198
// Sent buffer data format is:
200-
// - packet size as a uint16_t
199+
// - packet size as a uint32_t
201200
// - actual data
202201

203202
VerifyOrReturnError(address.GetTransportType() == Type::kTcp, CHIP_ERROR_INVALID_ARGUMENT);
204203
VerifyOrReturnError(mState == TCPState::kInitialized, CHIP_ERROR_INCORRECT_STATE);
205-
VerifyOrReturnError(kPacketSizeBytes + msgBuf->DataLength() <= std::numeric_limits<uint16_t>::max(),
204+
VerifyOrReturnError(kPacketSizeBytes + msgBuf->DataLength() <= System::PacketBuffer::kLargeBufMaxSizeWithoutReserve,
206205
CHIP_ERROR_INVALID_ARGUMENT);
207206

208207
// 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::
211210
msgBuf->SetStart(msgBuf->Start() - kPacketSizeBytes);
212211

213212
uint8_t * output = msgBuf->Start();
214-
LittleEndian::Write16(output, static_cast<uint16_t>(msgBuf->DataLength() - kPacketSizeBytes));
213+
LittleEndian::Write32(output, static_cast<uint32_t>(msgBuf->DataLength() - kPacketSizeBytes));
215214

216215
// Reuse existing connection if one exists, otherwise a new one
217216
// will be established
@@ -324,11 +323,11 @@ CHIP_ERROR TCPBase::ProcessReceivedBuffer(Inet::TCPEndPoint * endPoint, const Pe
324323
{
325324
return err;
326325
}
327-
uint16_t messageSize = LittleEndian::Get16(messageSizeBuf);
328-
if (messageSize >= kMaxMessageSize)
326+
uint32_t messageSize = LittleEndian::Get32(messageSizeBuf);
327+
if (messageSize >= kMaxTCPMessageSize)
329328
{
330-
331-
// This message is too long for upper layers.
329+
// Message is too big for node to process. Disconnect with peer.
330+
CloseConnectionInternal(state, CHIP_ERROR_MESSAGE_TOO_LONG, SuppressCallback::No);
332331
return CHIP_ERROR_MESSAGE_TOO_LONG;
333332
}
334333
// The subtraction will not underflow because we successfully read kPacketSizeBytes.
@@ -344,7 +343,7 @@ CHIP_ERROR TCPBase::ProcessReceivedBuffer(Inet::TCPEndPoint * endPoint, const Pe
344343
return CHIP_NO_ERROR;
345344
}
346345

347-
CHIP_ERROR TCPBase::ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, uint16_t messageSize)
346+
CHIP_ERROR TCPBase::ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, size_t messageSize)
348347
{
349348
// We enter with `state->mReceived` containing at least one full message, perhaps in a chain.
350349
// `state->mReceived->Start()` currently points to the message data.

src/transport/raw/TCP.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ class DLL_EXPORT TCPBase : public Base
258258
* is no other data).
259259
* @param[in] messageSize Size of the single message.
260260
*/
261-
CHIP_ERROR ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, uint16_t messageSize);
261+
CHIP_ERROR ProcessSingleMessage(const PeerAddress & peerAddress, ActiveTCPConnectionState * state, size_t messageSize);
262262

263263
/**
264264
* Initiate a connection to the given peer. On connection completion,
@@ -308,7 +308,7 @@ class DLL_EXPORT TCPBase : public Base
308308

309309
// The max payload size of data over a TCP connection that is transmissible
310310
// at a time.
311-
uint32_t mMaxTCPPayloadSize = CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES;
311+
uint32_t mMaxTCPSendAllocSize = System::PacketBuffer::kMaxAllocSize;
312312

313313
// Number of active and 'pending connection' endpoints
314314
size_t mUsedEndPointCount = 0;

src/transport/raw/TCPConfig.h

-9
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,6 @@ namespace chip {
6363
#define CHIP_CONFIG_MAX_TCP_PENDING_PACKETS 4
6464
#endif
6565

66-
/**
67-
* @def CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES
68-
*
69-
* @brief Maximum payload size of a message over a TCP connection
70-
*/
71-
#ifndef CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES
72-
#define CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES 1000000
73-
#endif
74-
7566
/**
7667
* @def CHIP_CONFIG_TCP_CONNECT_TIMEOUT_MSECS
7768
*

0 commit comments

Comments
 (0)