Skip to content

Commit 39cfea4

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. Test modifications for chainedbuffer receives for TCP. - Add test for Buffer length greater than MRP max size. Fixes Issues #31779, #33307.
1 parent 1b455b5 commit 39cfea4

9 files changed

+98
-62
lines changed

src/inet/TCPEndPointImplSockets.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -947,10 +947,10 @@ 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));
950+
VerifyOrDie(newDataLength <= UINT32_MAX);
951951
if (isNewBuf)
952952
{
953-
rcvBuf->SetDataLength(static_cast<uint16_t>(newDataLength));
953+
rcvBuf->SetDataLength(newDataLength);
954954
rcvBuf.RightSize();
955955
if (mRcvQueue.IsNull())
956956
{
@@ -963,7 +963,7 @@ void TCPEndPointImplSockets::ReceiveData()
963963
}
964964
else
965965
{
966-
rcvBuf->SetDataLength(static_cast<uint16_t>(newDataLength), mRcvQueue);
966+
rcvBuf->SetDataLength(newDataLength, mRcvQueue);
967967
}
968968
}
969969
}

src/system/SystemConfig.h

+9
Original file line numberDiff line numberDiff line change
@@ -771,3 +771,12 @@ struct LwIPEvent;
771771
#define CHIP_SYSTEM_CONFIG_USE_ZEPHYR_EVENTFD 0
772772
#endif
773773
#endif // CHIP_SYSTEM_CONFIG_USE_ZEPHYR_EVENTFD
774+
775+
/**
776+
* @def CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES
777+
*
778+
* @brief Maximum buffer allocation size of a 'Large' message
779+
*/
780+
#ifndef CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES
781+
#define CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES (64000)
782+
#endif

src/system/SystemPacketBuffer.cpp

+12-12
Original file line numberDiff line numberDiff line change
@@ -514,7 +514,9 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
514514
static_assert(PacketBuffer::kStructureSize == sizeof(PacketBuffer), "PacketBuffer size mismatch");
515515
static_assert(PacketBuffer::kStructureSize < UINT16_MAX, "Check for overflow more carefully");
516516
static_assert(SIZE_MAX >= INT_MAX, "Our additions might not fit in size_t");
517-
static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT32_MAX, "PacketBuffer may have size not fitting uint32_t");
517+
static_assert(CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES <= UINT32_MAX, "Max size for Large payload buffers");
518+
static_assert(PacketBuffer::kMaxSizeWithoutReserve < CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES,
519+
"Large buffer configuration should be greater than the conventional buffer limit");
518520
#if CHIP_SYSTEM_CONFIG_USE_LWIP
519521
// LwIP based APIs have a maximum buffer size of UINT16_MAX. Ensure that
520522
// limit is met during allocation.
@@ -530,8 +532,7 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
530532

531533
CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle());
532534

533-
// TODO: Change the max to a lower value
534-
if (aAvailableSize > UINT32_MAX || lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT32_MAX)
535+
if (lAllocSize > PacketBuffer::kMaxAllocSize)
535536
{
536537
ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
537538
return PacketBufferHandle();
@@ -593,18 +594,15 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
593594
PacketBufferHandle PacketBufferHandle::NewWithData(const void * aData, size_t aDataSize, size_t aAdditionalSize,
594595
uint16_t aReservedSize)
595596
{
596-
if (aDataSize > UINT16_MAX)
597-
{
598-
ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
599-
return PacketBufferHandle();
600-
}
601597
// Since `aDataSize` fits in uint16_t, the sum `aDataSize + aAdditionalSize` will not overflow.
602598
// `New()` will only return a non-null buffer if the total allocation size does not overflow.
603599
PacketBufferHandle buffer = New(aDataSize + aAdditionalSize, aReservedSize);
604600
if (buffer.mBuffer != nullptr)
605601
{
606602
memcpy(buffer.mBuffer->payload, aData, aDataSize);
607603
#if CHIP_SYSTEM_CONFIG_USE_LWIP
604+
// The VerifyOrDie() in the New() call catches buffer allocations greater
605+
// than UINT16_MAX for LwIP based platforms.
608606
buffer.mBuffer->len = buffer.mBuffer->tot_len = static_cast<uint16_t>(aDataSize);
609607
#else
610608
buffer.mBuffer->len = buffer.mBuffer->tot_len = aDataSize;
@@ -727,18 +725,20 @@ PacketBufferHandle PacketBufferHandle::CloneData() const
727725
size_t originalDataSize = original->MaxDataLength();
728726
uint16_t originalReservedSize = original->ReservedSize();
729727

730-
if (originalDataSize + originalReservedSize > PacketBuffer::kMaxSizeWithoutReserve)
728+
uint32_t maxSize = PacketBuffer::kMaxAllocSize;
729+
730+
if (originalDataSize + originalReservedSize > maxSize)
731731
{
732732
// The original memory allocation may have provided a larger block than requested (e.g. when using a shared pool),
733733
// and in particular may have provided a larger block than we are able to request from PackBufferHandle::New().
734734
// It is a genuine error if that extra space has been used.
735-
if (originalReservedSize + original->DataLength() > PacketBuffer::kMaxSizeWithoutReserve)
735+
if (originalReservedSize + original->DataLength() > maxSize)
736736
{
737737
return PacketBufferHandle();
738738
}
739739
// Otherwise, reduce the requested data size. This subtraction can not underflow because the above test
740-
// guarantees originalReservedSize <= PacketBuffer::kMaxSizeWithoutReserve.
741-
originalDataSize = PacketBuffer::kMaxSizeWithoutReserve - originalReservedSize;
740+
// guarantees originalReservedSize <= maxSize.
741+
originalDataSize = maxSize - originalReservedSize;
742742
}
743743

744744
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

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

312+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
313+
if (config.reserved_size > CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES)
314+
#else
312315
if (config.reserved_size > PacketBuffer::kMaxSizeWithoutReserve)
316+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
313317
{
314318
EXPECT_TRUE(buffer.IsNull());
315319
continue;
@@ -1605,7 +1609,11 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleRightSize)
16051609

16061610
TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
16071611
{
1612+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
1613+
uint8_t lPayload[2 * CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES];
1614+
#else
16081615
uint8_t lPayload[2 * PacketBuffer::kMaxSizeWithoutReserve];
1616+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
16091617
for (uint8_t & payload : lPayload)
16101618
{
16111619
payload = static_cast<uint8_t>(random());
@@ -1684,7 +1692,11 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
16841692
// This is only testable on heap allocation configurations, where pbuf records the allocation size and we can manually
16851693
// construct an oversize buffer.
16861694

1695+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
1696+
constexpr uint32_t kOversizeDataSize = CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES + 99;
1697+
#else
16871698
constexpr uint16_t kOversizeDataSize = PacketBuffer::kMaxSizeWithoutReserve + 99;
1699+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
16881700
PacketBuffer * p = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(kStructureSize + kOversizeDataSize));
16891701
ASSERT_NE(p, nullptr);
16901702

@@ -1698,15 +1710,20 @@ TEST_F_FROM_FIXTURE(TestSystemPacketBuffer, CheckHandleCloneData)
16981710
PacketBufferHandle handle = PacketBufferHandle::Adopt(p);
16991711

17001712
// Fill the buffer to maximum and verify that it can be cloned.
1713+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
1714+
uint32_t maxSize = CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES;
1715+
#else
1716+
uint32_t maxSize = PacketBuffer::kMaxSizeWithoutReserve;
1717+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
17011718

1702-
memset(handle->Start(), 1, PacketBuffer::kMaxSizeWithoutReserve);
1703-
handle->SetDataLength(PacketBuffer::kMaxSizeWithoutReserve);
1704-
EXPECT_EQ(handle->DataLength(), PacketBuffer::kMaxSizeWithoutReserve);
1719+
memset(handle->Start(), 1, maxSize);
1720+
handle->SetDataLength(maxSize);
1721+
EXPECT_EQ(handle->DataLength(), maxSize);
17051722

17061723
PacketBufferHandle clone = handle.CloneData();
17071724
ASSERT_FALSE(clone.IsNull());
1708-
EXPECT_EQ(clone->DataLength(), PacketBuffer::kMaxSizeWithoutReserve);
1709-
EXPECT_EQ(memcmp(handle->Start(), clone->Start(), PacketBuffer::kMaxSizeWithoutReserve), 0);
1725+
EXPECT_EQ(clone->DataLength(), maxSize);
1726+
EXPECT_EQ(memcmp(handle->Start(), clone->Start(), maxSize), 0);
17101727

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

src/transport/raw/TCP.cpp

+9-10
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,9 @@ namespace {
4040
using namespace chip::Encoding;
4141

4242
// Packets start with a 16-bit size
43-
constexpr size_t kPacketSizeBytes = 2;
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>(CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES - kPacketSizeBytes);
4746

4847
constexpr int kListenBacklogSize = 2;
4948

@@ -202,7 +201,7 @@ CHIP_ERROR TCPBase::SendMessage(const Transport::PeerAddress & address, System::
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() <= std::numeric_limits<uint32_t>::max(),
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)