From 0a1b710d5f71bb1d8c0851e5a2d2164d54471337 Mon Sep 17 00:00:00 2001 From: Kevin Schoedel Date: Fri, 9 Jul 2021 12:29:07 -0400 Subject: [PATCH] Additional PacketBuffer size fixes. #### Problem 1. After #8226, `PacketBuffer::kMaxSizeWithoutReserve` was wrong on LwIP builds. 2. It is possible for a `PacketBufferHandle::New()` to return a larger buffer than requested (e.g. when using a shared pool allocator), and in particular for it to return a block that is larger than *can* be requested. Attempting `CloneData()` on such a buffer fails with an error message logged by `PacketBufferHandle::New()`. #### Change overview 1. Fix `PacketBuffer::kMaxSizeWithoutReserve`. 2. As long as the excess space is not actually occupied (which would be incorrect, since it exceeds the spec size limit), `CloneData()` copies correctly to a maximum-size buffer. #### Testing Added a unit test to verify that a buffer with excess unused space is clonable, and a buffer with oversize space in use is not. --- src/system/SystemPacketBuffer.cpp | 17 +++++++- src/system/SystemPacketBuffer.h | 2 +- src/system/tests/TestSystemPacketBuffer.cpp | 46 +++++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp index 4cc91b5dff326e..35f8eeaf956c17 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 = 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..fac5d1796b7a2c 100644 --- a/src/system/tests/TestSystemPacketBuffer.cpp +++ b/src/system/tests/TestSystemPacketBuffer.cpp @@ -1873,6 +1873,52 @@ 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; + uint8_t buf[PacketBuffer::kStructureSize + kOversizeDataSize]; + PacketBuffer * p = reinterpret_cast(buf); + + p->next = nullptr; + p->payload = buf + 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()); + + p = std::move(handle).UnsafeRelease(); + +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP } void PacketBufferTest::CheckPacketBufferWriter(nlTestSuite * inSuite, void * inContext)