Skip to content

Commit

Permalink
Revert "Fix alignment when using PacketBuffer reserve space (#19244)"
Browse files Browse the repository at this point in the history 
This reverts commit fa43c74.
  • Loading branch information
@kpschoedel
kpschoedel authored Jun 7, 2022
1 parent c868f81 commit f7ce63d
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 216 deletions.
11 changes: 10 additions & 1 deletion src/inet/UDPEndPointImplLwIP.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,16 @@ struct netif * UDPEndPointImplLwIP::FindNetifFromInterfaceId(InterfaceId aInterf

IPPacketInfo * UDPEndPointImplLwIP::GetPacketInfo(const System::PacketBufferHandle & aBuffer)
{
return aBuffer->GetReserve<IPPacketInfo>();
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<IPPacketInfo *>(lPacketInfoStart & ~(sizeof(uint32_t) - 1));
}

} // namespace Inet
Expand Down
21 changes: 16 additions & 5 deletions src/inet/UDPEndPointImplOpenThread.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -291,7 +293,16 @@ CHIP_ERROR UDPEndPointImplOT::IPv6JoinLeaveMulticastGroupImpl(InterfaceId aInter

IPPacketInfo * UDPEndPointImplOT::GetPacketInfo(const System::PacketBufferHandle & aBuffer)
{
return aBuffer->GetReserve<IPPacketInfo>();
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<IPPacketInfo *>(lPacketInfoStart & ~kPacketInfoAlignmentBytes);
}

} // namespace Inet
Expand Down
40 changes: 0 additions & 40 deletions src/system/SystemPacketBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>().

// Computing `reserveStart` can't overflow because a valid PacketBuffer must be larger than kStructureSize.
const uintptr_t reserveStart = reinterpret_cast<uintptr_t>(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<uint16_t>(requestStart - reserveStart);

// This cast is not safe in itself, but the result is checked.
const uint16_t requestSize = static_cast<uint16_t>(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<uint8_t *>(requestStart);
}

bool PacketBuffer::AlignPayload(uint16_t aAlignBytes)
{
if (aAlignBytes == 0)
Expand Down
20 changes: 1 addition & 19 deletions src/system/SystemPacketBuffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 <typename T>
T * GetReserve()
{
static_assert(sizeof(T) <= UINT16_MAX, "type too large");
static_assert(alignof(T) <= UINT16_MAX, "alignment too large");
return reinterpret_cast<T *>(GetReserve(static_cast<uint16_t>(sizeof(T)), static_cast<uint16_t>(alignof(T) - 1)));
}

/**
* Align the buffer payload on the specified bytes boundary.
*
Expand Down Expand Up @@ -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);
Expand Down
156 changes: 5 additions & 151 deletions src/system/tests/TestSystemPacketBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#define __STDC_LIMIT_MACROS
#endif

#include <algorithm>
#include <errno.h>
#include <stdint.h>
#include <stdlib.h>
Expand Down Expand Up @@ -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);
Expand All @@ -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<uint16_t>(reinterpret_cast<const char *>(buffer->payload) - reinterpret_cast<const char *>(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); }

Expand All @@ -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);
Expand Down Expand Up @@ -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<struct TestContext *>(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<uint8_t>(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<uint16_t>(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<PacketBuffer *>(static_cast<uintptr_t>(-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<PacketBuffer *>(static_cast<uintptr_t>(-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<uint8_t *>(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<uintptr_t>(reserve) % kBigAlignment) == 0);
}
}

/**
* Test PacketBuffer::AlignPayload() function.
*
Expand Down Expand Up @@ -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),
Expand Down

0 comments on commit f7ce63d

Please sign in to comment.