Skip to content

Commit

Permalink
Changes for large Packetbuffer allocation to support TCP payloads
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pidarped committed May 15, 2024
1 parent 1b455b5 commit 39cfea4
Show file tree
Hide file tree
Showing 9 changed files with 98 additions and 62 deletions.
6 changes: 3 additions & 3 deletions src/inet/TCPEndPointImplSockets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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())
{
Expand All @@ -963,7 +963,7 @@ void TCPEndPointImplSockets::ReceiveData()
}
else
{
rcvBuf->SetDataLength(static_cast<uint16_t>(newDataLength), mRcvQueue);
rcvBuf->SetDataLength(newDataLength, mRcvQueue);
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions src/system/SystemConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
24 changes: 12 additions & 12 deletions src/system/SystemPacketBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES <= UINT32_MAX, "Max size for Large payload buffers");
static_assert(PacketBuffer::kMaxSizeWithoutReserve < CHIP_SYSTEM_CONFIG_MAX_LARGE_BUFFER_SIZE_BYTES,
"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.
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 12 additions & 0 deletions src/system/SystemPacketBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
27 changes: 22 additions & 5 deletions src/system/tests/TestSystemPacketBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand Down
19 changes: 9 additions & 10 deletions src/transport/raw/TCP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions src/transport/raw/TCP.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 0 additions & 9 deletions src/transport/raw/TCPConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down
Loading

0 comments on commit 39cfea4

Please sign in to comment.