From f7ce63daf6a53fdeb03b53b043e52e78cd0b8a18 Mon Sep 17 00:00:00 2001 From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com> Date: Tue, 7 Jun 2022 18:40:44 -0400 Subject: [PATCH] Revert "Fix alignment when using PacketBuffer reserve space (#19244)" This reverts commit fa43c74a58da21ccf05f9d1979583a5740219431. --- src/inet/UDPEndPointImplLwIP.cpp | 11 +- src/inet/UDPEndPointImplOpenThread.cpp | 21 ++- src/system/SystemPacketBuffer.cpp | 40 ----- src/system/SystemPacketBuffer.h | 20 +-- src/system/tests/TestSystemPacketBuffer.cpp | 156 +------------------- 5 files changed, 32 insertions(+), 216 deletions(-) diff --git a/src/inet/UDPEndPointImplLwIP.cpp b/src/inet/UDPEndPointImplLwIP.cpp index a423075f282baa..b3ce8a24705eb1 100644 --- a/src/inet/UDPEndPointImplLwIP.cpp +++ b/src/inet/UDPEndPointImplLwIP.cpp @@ -471,7 +471,16 @@ struct netif * UDPEndPointImplLwIP::FindNetifFromInterfaceId(InterfaceId aInterf IPPacketInfo * UDPEndPointImplLwIP::GetPacketInfo(const System::PacketBufferHandle & aBuffer) { - return aBuffer->GetReserve(); + if (!aBuffer->EnsureReservedSize(sizeof(IPPacketInfo) + 3)) + { + return nullptr; + } + + uintptr_t lStart = (uintptr_t) aBuffer->Start(); + uintptr_t lPacketInfoStart = lStart - sizeof(IPPacketInfo); + + // Align to a 4-byte boundary + return reinterpret_cast(lPacketInfoStart & ~(sizeof(uint32_t) - 1)); } } // namespace Inet diff --git a/src/inet/UDPEndPointImplOpenThread.cpp b/src/inet/UDPEndPointImplOpenThread.cpp index a9fb43794ad65a..02f11ba30bdf75 100644 --- a/src/inet/UDPEndPointImplOpenThread.cpp +++ b/src/inet/UDPEndPointImplOpenThread.cpp @@ -34,10 +34,12 @@ otInstance * globalOtInstance; namespace { // We want to reserve space for an IPPacketInfo in our buffer, but it needs to -// be suitably aligned. That might move it backward by up to -// kPacketInfoAlignmentBytes, so we need to make sure we allocate enough -// reserved space that this will still be within our buffer. -constexpr uint16_t kPacketInfoAlignmentBytes = alignof(IPPacketInfo) - 1; +// be 4-byte aligned. We ensure the alignment by masking off the low bits of +// the pointer that we get by doing `Start() - sizeof(IPPacketInfo)`. That +// might move it backward by up to kPacketInfoAlignmentBytes, so we need to make +// sure we allocate enough reserved space that this will still be within our +// buffer. +constexpr uint16_t kPacketInfoAlignmentBytes = sizeof(uint32_t) - 1; constexpr uint16_t kPacketInfoReservedSize = sizeof(IPPacketInfo) + kPacketInfoAlignmentBytes; } // namespace @@ -291,7 +293,16 @@ CHIP_ERROR UDPEndPointImplOT::IPv6JoinLeaveMulticastGroupImpl(InterfaceId aInter IPPacketInfo * UDPEndPointImplOT::GetPacketInfo(const System::PacketBufferHandle & aBuffer) { - return aBuffer->GetReserve(); + if (!aBuffer->EnsureReservedSize(kPacketInfoReservedSize)) + { + return nullptr; + } + + uintptr_t lStart = (uintptr_t) aBuffer->Start(); + uintptr_t lPacketInfoStart = lStart - sizeof(IPPacketInfo); + + // Align to a 4-byte boundary + return reinterpret_cast(lPacketInfoStart & ~kPacketInfoAlignmentBytes); } } // namespace Inet diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp index ce2bc65570aac5..3f6c136bcb152b 100644 --- a/src/system/SystemPacketBuffer.cpp +++ b/src/system/SystemPacketBuffer.cpp @@ -401,46 +401,6 @@ bool PacketBuffer::EnsureReservedSize(uint16_t aReservedSize) return true; } -uint8_t * PacketBuffer::GetReserve(uint16_t aSize, uint16_t aAlignmentMask) -{ - // This private utility method requires that `aAlignmentMask` be one less than a power-of-two alignment. This requirement - // is satisfied when aAlignmentMask=alignof(T)-1 for some type T, as passed by the public GetReserve(). - - // Computing `reserveStart` can't overflow because a valid PacketBuffer must be larger than kStructureSize. - const uintptr_t reserveStart = reinterpret_cast(this) + kStructureSize; - if (reserveStart + aSize < reserveStart) - { - // Overflow here means the requested size can't possibly fit. - return nullptr; - } - - const uintptr_t requestStart = (reserveStart + aAlignmentMask) & ~aAlignmentMask; - if (requestStart < reserveStart) - { - // Overflow here means the request can't be satisfied because the alignment is too large. - return nullptr; - } - - // This cast is safe because the difference is at most `aAlignmentMask`. - const uint16_t reserveAlignmentOffset = static_cast(requestStart - reserveStart); - - // This cast is not safe in itself, but the result is checked. - const uint16_t requestSize = static_cast(reserveAlignmentOffset + aSize); - if (requestSize < aSize) - { - // Overflow here means the requested size can't possibly fit. - return nullptr; - } - - if (!EnsureReservedSize(requestSize)) - { - // The requested size is too large for the available space. - return nullptr; - } - - return reinterpret_cast(requestStart); -} - bool PacketBuffer::AlignPayload(uint16_t aAlignBytes) { if (aAlignBytes == 0) diff --git a/src/system/SystemPacketBuffer.h b/src/system/SystemPacketBuffer.h index 61b07147ed3e8f..c61cbcd89bf119 100644 --- a/src/system/SystemPacketBuffer.h +++ b/src/system/SystemPacketBuffer.h @@ -116,7 +116,7 @@ class DLL_EXPORT PacketBuffer : private pbuf #if CHIP_SYSTEM_CONFIG_USE_LWIP static constexpr uint16_t kStructureSize = LWIP_MEM_ALIGN_SIZE(sizeof(struct ::pbuf)); #else // CHIP_SYSTEM_CONFIG_USE_LWIP - static constexpr uint16_t kStructureSize = CHIP_SYSTEM_ALIGN_SIZE(sizeof(::chip::System::pbuf), alignof(::chip::System::pbuf)); + static constexpr uint16_t kStructureSize = CHIP_SYSTEM_ALIGN_SIZE(sizeof(::chip::System::pbuf), 4u); #endif // CHIP_SYSTEM_CONFIG_USE_LWIP public: @@ -288,23 +288,6 @@ class DLL_EXPORT PacketBuffer : private pbuf */ CHECK_RETURN_VALUE bool EnsureReservedSize(uint16_t aReservedSize); - /** - * Get a pointer to reserved space. - * - * Returns a pointer to space in the packet buffer with size and alignment suitable for type T. Payload data is - * moved if necessary to increase the reserve. Due to alignment and padding, the returned space is not necessarily - * adjacent to either the packet buffer header or to the payload. - * - * @return \c pointer to the requested reserve if available, \c nullptr if there's not enough room in the buffer. - */ - template - T * GetReserve() - { - static_assert(sizeof(T) <= UINT16_MAX, "type too large"); - static_assert(alignof(T) <= UINT16_MAX, "alignment too large"); - return reinterpret_cast(GetReserve(static_cast(sizeof(T)), static_cast(alignof(T) - 1))); - } - /** * Align the buffer payload on the specified bytes boundary. * @@ -390,7 +373,6 @@ class DLL_EXPORT PacketBuffer : private pbuf static void InternalCheck(const PacketBuffer * buffer); #endif - uint8_t * GetReserve(uint16_t aSize, uint16_t aAlignmentMask); void AddRef(); bool HasSoleOwnership() const { return (this->ref == 1); } static void Free(PacketBuffer * aPacket); diff --git a/src/system/tests/TestSystemPacketBuffer.cpp b/src/system/tests/TestSystemPacketBuffer.cpp index f023fa372fe81b..6f0ba37d70ff28 100644 --- a/src/system/tests/TestSystemPacketBuffer.cpp +++ b/src/system/tests/TestSystemPacketBuffer.cpp @@ -27,7 +27,6 @@ #define __STDC_LIMIT_MACROS #endif -#include #include #include #include @@ -115,7 +114,6 @@ class PacketBufferTest static void CheckConsumeHead(nlTestSuite * inSuite, void * inContext); static void CheckConsume(nlTestSuite * inSuite, void * inContext); static void CheckEnsureReservedSize(nlTestSuite * inSuite, void * inContext); - static void CheckGetReserve(nlTestSuite * inSuite, void * inContext); static void CheckAlignPayload(nlTestSuite * inSuite, void * inContext); static void CheckNext(nlTestSuite * inSuite, void * inContext); static void CheckLast(nlTestSuite * inSuite, void * inContext); @@ -138,18 +136,8 @@ class PacketBufferTest static void PrintHandle(const char * tag, const PacketBuffer * buffer) { - if (buffer) - { - const uint16_t reserved_offset = PacketBuffer::kStructureSize; - const uint16_t payload_offset = - static_cast(reinterpret_cast(buffer->payload) - reinterpret_cast(buffer)); - printf("%s res@%-4u#%-4u pay@%-4u#%-4u %p next=%p ref=%u\n", tag, reserved_offset, payload_offset - reserved_offset, - payload_offset, buffer->len, buffer, buffer->next, buffer->ref); - } - else - { - printf("%s NULL\n", tag); - } + printf("%s %p ref=%u len=%-4u next=%p\n", tag, buffer, buffer ? buffer->ref : 0, buffer ? buffer->len : 0, + buffer ? buffer->next : nullptr); } static void PrintHandle(const char * tag, const PacketBufferHandle & handle) { PrintHandle(tag, handle.mBuffer); } @@ -174,9 +162,9 @@ class PacketBufferTest static void PrintHandle(const char * tag, const BufferConfiguration & config) { PrintHandle(tag, config.handle); } static void PrintConfig(const char * tag, const BufferConfiguration & config) { - printf("%s res@%-4u#%-4u pay@%-4zu#%-4u (config)\n", tag, PacketBuffer::kStructureSize, config.reserved_size, - config.payload_ptr - config.start_buffer, config.init_len); - PrintHandle(tag, config.handle); + printf("%s pay=%-4zu len=%-4u res=%-4u:", tag, config.payload_ptr - config.start_buffer, config.init_len, + config.reserved_size); + PrintHandle("", config.handle); } PacketBufferTest(TestContext * context); @@ -1154,139 +1142,6 @@ void PacketBufferTest::CheckEnsureReservedSize(nlTestSuite * inSuite, void * inC } } -/** - * Test PacketBuffer::GetReserve() function. - * - * Description: This tests the private implementation version of GetReserve(), - * since we can't iterate over types. The tested configurations are custom - * for this test, since the usual ones don't cover the relevant boundary conditions. - */ -void PacketBufferTest::CheckGetReserve(nlTestSuite * inSuite, void * inContext) -{ - struct TestContext * const theContext = static_cast(inContext); - PacketBufferTest * const test = theContext->test; - NL_TEST_ASSERT(inSuite, test->mContext == theContext); - - uint8_t payloads[2 * kBlockSize]; - for (size_t i = 1; i < sizeof(payloads); ++i) - { - payloads[i] = static_cast(random()); - } - - constexpr uint16_t kMax = PacketBuffer::kMaxSizeWithoutReserve; - - // clang-format off - const struct Instance - { - // PacketBuffer initialization: - struct { - uint16_t reserve_length; - uint16_t payload_length; - } init; - // GetReserve(): - struct { - uint16_t length; // Must be a multiple of alignment. - uint16_t alignment; // Must be a power of 2. - } request; - // Expected result: - struct { - bool success; - uint16_t motion; - } expect; - } instances[] = { - // PacketBuffer GetReserve - // Reserve Payload Length Align Expect - { { 1, 1}, { 1, 1}, { true, 0 } }, // Fits without moving payload. - { { 0, 1}, { 1, 1}, { true, 1 } }, // Fits by moving payload 1 byte. - { { 0, kMax}, { 1, 1}, { false, 0 } }, // No space to move payload. - { { 16, 1}, { 16, 8}, { true, 0 } }, // Fits without moving payload. - { { 9, 1}, { 8, 8}, { true, 0 } }, // Fits without moving payload. - { { 9, 1}, { 16, 8}, { true, 7 } }, // Fits by moving payload 7 bytes. - }; - // clang-format on - - for (auto & instance : instances) - { - BufferConfiguration config(instance.init.reserve_length); - test->PrepareTestBuffer(&config, kRecordHandle | kAllowHandleReuse); - - uint8_t * payload_start = config.handle->Start(); - memcpy(payload_start, payloads, instance.init.payload_length); - config.handle->SetDataLength(instance.init.payload_length); - uint16_t available_payload_length = config.handle->AvailableDataLength(); - - // Check that the packet was initialized correctly. - NL_TEST_ASSERT(inSuite, config.handle->ReservedSize() == instance.init.reserve_length); - NL_TEST_ASSERT(inSuite, config.handle->TotalLength() == instance.init.payload_length); - - const uint8_t * const reserve = - config.handle->GetReserve(instance.request.length, static_cast(instance.request.alignment - 1)); - if (instance.expect.success) - { - NL_TEST_ASSERT(inSuite, reserve != nullptr); - - // Verify that the payload is intact. - NL_TEST_ASSERT(inSuite, config.handle->TotalLength() == instance.init.payload_length); - NL_TEST_ASSERT(inSuite, memcmp(config.handle->Start(), payloads, instance.init.payload_length) == 0); - - // Verify that the payload was moved or not. - NL_TEST_ASSERT(inSuite, payload_start + instance.expect.motion == config.handle->Start()); - NL_TEST_ASSERT(inSuite, available_payload_length - instance.expect.motion == config.handle->AvailableDataLength()); - } - else - { - NL_TEST_ASSERT(inSuite, reserve == nullptr); - } - } - - // Check the case that the packet buffer starts so close to the end of memory that adding the requested length would overflow - // and result in a pointer that incorrectly appears to be before the payload start. - // - // NB: White-box test! This relies on knowing that the implementation of GetReserve() returns without dereferencing the - // pointer in this case. - // NB: Implementation-defined test! Merely creating an invalid pointer is allowed to fail (but is NOT ‘undefined behavior’), - // although it does not on any currently common platform. - PacketBuffer * badPointer = reinterpret_cast(static_cast(-PacketBuffer::kStructureSize - 1)); - NL_TEST_ASSERT(inSuite, badPointer->GetReserve(1, 1) == nullptr); - - // Check the case that the packet buffer starts so close to the end of memory that the requested alignment would overflow - // and result in a pointer that incorrectly appears to be before the payload start. - // - // NB: White-box test! This relies on knowing that the implementation of GetReserve() returns without dereferencing the - // pointer in this case. - // NB: Implementation-defined test! Merely creating an invalid pointer is allowed to fail (but is NOT ‘undefined behavior’), - // although it does not on any currently common platform. - badPointer = reinterpret_cast(static_cast(-PacketBuffer::kStructureSize - 2)); - NL_TEST_ASSERT(inSuite, badPointer->GetReserve(1, 2) == nullptr); - - // Check the case that the requested alignment is greater than PacketBuffer alignment, so that, depending on the actual - // location of the PacketBuffer, the returned pointer may or may not be able to be located immediately after the header. - // - // NB: White-box test! This can't happen with heap-allocated buffers, since heap allocation returns a maximally aligned - // pointer, so we need to allocate and set up the PacketBuffer internal state manually. We construct a pair of packet - // buffers pb[] such that one of the two is aligned to (2*alignof(PacketBuffer)) and the other is not. - constexpr size_t kHeadersPerBlock = (PacketBuffer::kBlockSize + sizeof(PacketBuffer) - 1) / sizeof(PacketBuffer); - constexpr size_t kOddHeadersPerBlock = (kHeadersPerBlock % 2) == 0 ? kHeadersPerBlock + 1 : kHeadersPerBlock; - NL_TEST_ASSERT(inSuite, kOddHeadersPerBlock > 2); - PacketBuffer holder[2 * kOddHeadersPerBlock + 1]; - PacketBuffer * pb[2] = { &holder[0], &holder[kOddHeadersPerBlock] }; - - constexpr size_t kBigAlignment = 2 * alignof(PacketBuffer); - for (int i = 0; i < 2; ++i) - { - pb[i]->next = nullptr; - pb[i]->payload = reinterpret_cast(pb[i]) + 2 * sizeof(PacketBuffer) + 1; - pb[i]->tot_len = pb[i]->len = 1; - pb[i]->ref = 0; -#if CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_HEAP - pb[i]->alloc_size = kHeadersPerBlock * sizeof(PacketBuffer); -#endif - const uint8_t * const reserve = pb[i]->GetReserve(1, kBigAlignment - 1); - NL_TEST_ASSERT(inSuite, reserve != nullptr); - NL_TEST_ASSERT(inSuite, (reinterpret_cast(reserve) % kBigAlignment) == 0); - } -} - /** * Test PacketBuffer::AlignPayload() function. * @@ -2132,7 +1987,6 @@ const nlTest sTests[] = NL_TEST_DEF("PacketBuffer::ConsumeHead", PacketBufferTest::CheckConsumeHead), NL_TEST_DEF("PacketBuffer::Consume", PacketBufferTest::CheckConsume), NL_TEST_DEF("PacketBuffer::EnsureReservedSize", PacketBufferTest::CheckEnsureReservedSize), - NL_TEST_DEF("PacketBuffer::GetReserve", PacketBufferTest::CheckGetReserve), NL_TEST_DEF("PacketBuffer::AlignPayload", PacketBufferTest::CheckAlignPayload), NL_TEST_DEF("PacketBuffer::Next", PacketBufferTest::CheckNext), NL_TEST_DEF("PacketBuffer::Last", PacketBufferTest::CheckLast),