diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp index 4cc91b5dff326e..84ec5c865239cb 100644 --- a/src/system/SystemPacketBuffer.cpp +++ b/src/system/SystemPacketBuffer.cpp @@ -645,7 +645,22 @@ PacketBufferHandle PacketBufferHandle::CloneData() const { uint16_t originalDataSize = original->MaxDataLength(); uint16_t originalReservedSize = original->ReservedSize(); - PacketBufferHandle clone = PacketBufferHandle::New(originalDataSize, originalReservedSize); + + if (originalDataSize + originalReservedSize > PacketBuffer::kMaxSizeWithoutReserve) + { + // 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) + { + return PacketBufferHandle(); + } + // Otherwise, reduce the requested data size. This subtraction can not underflow because the above test + // guarantees originalReservedSize <= PacketBuffer::kMaxSizeWithoutReserve. + originalDataSize = static_cast(PacketBuffer::kMaxSizeWithoutReserve - originalReservedSize); + } + + PacketBufferHandle clone = PacketBufferHandle::New(originalDataSize, originalReservedSize); if (clone.IsNull()) { return PacketBufferHandle(); diff --git a/src/system/SystemPacketBuffer.h b/src/system/SystemPacketBuffer.h index 88e695749346b3..44a3bc18b0d774 100644 --- a/src/system/SystemPacketBuffer.h +++ b/src/system/SystemPacketBuffer.h @@ -146,7 +146,7 @@ class DLL_EXPORT PacketBuffer : private pbuf * The maximum size buffer an application can allocate with no protocol header reserve. */ #if CHIP_SYSTEM_CONFIG_USE_LWIP - static constexpr uint16_t kMaxSizeWithoutReserve = (LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE) - PacketBuffer::kStructureSize); + static constexpr uint16_t kMaxSizeWithoutReserve = LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE); #else static constexpr uint16_t kMaxSizeWithoutReserve = CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX; #endif diff --git a/src/system/tests/TestSystemPacketBuffer.cpp b/src/system/tests/TestSystemPacketBuffer.cpp index aaa35b214ac463..a6200a41b3e203 100644 --- a/src/system/tests/TestSystemPacketBuffer.cpp +++ b/src/system/tests/TestSystemPacketBuffer.cpp @@ -1873,6 +1873,54 @@ void PacketBufferTest::CheckHandleCloneData(nlTestSuite * inSuite, void * inCont config_2.handle = nullptr; } } + +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP + + // It is possible for a packet buffer allocation to return a larger block than requested (e.g. when using a shared pool) + // and in particular to return a larger block than it is possible to request from PackBufferHandle::New(). + // In that case, (a) it is incorrect to actually use the extra space, and (b) if it is not used, the clone will + // be the maximum possible size. + // + // 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; + PacketBuffer * p = + reinterpret_cast(chip::Platform::MemoryAlloc(PacketBuffer::kStructureSize + kOversizeDataSize)); + NL_TEST_ASSERT(inSuite, p != nullptr); + + p->next = nullptr; + p->payload = reinterpret_cast(p) + PacketBuffer::kStructureSize; + p->tot_len = 0; + p->len = 0; + p->ref = 1; + p->alloc_size = kOversizeDataSize; + + PacketBufferHandle handle = PacketBufferHandle::Adopt(p); + + // Fill the buffer to maximum and verify that it can be cloned. + + memset(handle->Start(), 1, PacketBuffer::kMaxSizeWithoutReserve); + handle->SetDataLength(PacketBuffer::kMaxSizeWithoutReserve); + NL_TEST_ASSERT(inSuite, handle->DataLength() == PacketBuffer::kMaxSizeWithoutReserve); + + PacketBufferHandle clone = handle.CloneData(); + NL_TEST_ASSERT(inSuite, !clone.IsNull()); + NL_TEST_ASSERT(inSuite, clone->DataLength() == PacketBuffer::kMaxSizeWithoutReserve); + NL_TEST_ASSERT(inSuite, memcmp(handle->Start(), clone->Start(), PacketBuffer::kMaxSizeWithoutReserve) == 0); + + // Overfill the buffer and verify that it can not be cloned. + memset(handle->Start(), 2, kOversizeDataSize); + handle->SetDataLength(kOversizeDataSize); + NL_TEST_ASSERT(inSuite, handle->DataLength() == kOversizeDataSize); + + clone = handle.CloneData(); + NL_TEST_ASSERT(inSuite, clone.IsNull()); + + // Free the packet buffer memory ourselves, since we allocated it ourselves. + chip::Platform::MemoryFree(std::move(handle).UnsafeRelease()); + +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP } void PacketBufferTest::CheckPacketBufferWriter(nlTestSuite * inSuite, void * inContext)