From 88b3367402a4e83c98343aeddd01d922d3ab7193 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Thu, 2 Feb 2023 11:01:24 -0800 Subject: [PATCH] [message] add `WriteBytesFromMessage()` and `Remove/InsertHeader()` (#8708) This commit updates `Message` class adding new methods. A new method `WriteBytesFromMessage()` is added which writes bytes read from another or potentially the same message to the message at a given offset. This method replaces the `CopyTo()` and unlike `CopyTo()` it can be used to copy bytes within the same message in either forward or backward directions. This commit adds `RemoveHeader()` which removes header bytes from the message at a given offset and length. It shrinks the message and copies existing header bytes before the removed segment forward to replace the removed bytes. This commit also adds `InsertHeader()` method which grows the message to make space for new header bytes at a given offset. The existing header bytes are copied backward to make room for the new header bytes. Unit test `test_message` is updated to validated all the newly added methods. --- src/core/common/message.cpp | 121 ++++++++++++++++++++++++++------ src/core/common/message.hpp | 73 +++++++++++++++----- src/core/net/ip6.cpp | 39 +++-------- tests/unit/test_message.cpp | 134 +++++++++++++++++++++++++++--------- 4 files changed, 268 insertions(+), 99 deletions(-) diff --git a/src/core/common/message.cpp b/src/core/common/message.cpp index 5986a73d0..4db1499fc 100644 --- a/src/core/common/message.cpp +++ b/src/core/common/message.cpp @@ -482,6 +482,67 @@ void Message::RemoveHeader(uint16_t aLength) } } +void Message::RemoveHeader(uint16_t aOffset, uint16_t aLength) +{ + // To shrink the header, we copy the header byte before `aOffset` + // forward. Starting at offset `aLength`, we write bytes we read + // from offset `0` onward and copy a total of `aOffset` bytes. + // Then remove the first `aLength` bytes from message. + // + // + // 0 aOffset aOffset + aLength + // +-----------------------+---------+------------------------+ + // | / / / / / / / / / / / | x x x x | | + // +-----------------------+---------+------------------------+ + // + // 0 aLength aOffset + aLength + // +---------+-----------------------+------------------------+ + // | | / / / / / / / / / / / | | + // +---------+-----------------------+------------------------+ + // + // 0 aOffset + // +-----------------------+------------------------+ + // | / / / / / / / / / / / | | + // +-----------------------+------------------------+ + // + + WriteBytesFromMessage(/* aWriteOffset */ aLength, *this, /* aReadOffset */ 0, /* aLength */ aOffset); + RemoveHeader(aLength); +} + +Error Message::InsertHeader(uint16_t aOffset, uint16_t aLength) +{ + Error error; + + // To make space in header at `aOffset`, we first prepend + // `aLength` bytes at front. Then copy the existing bytes + // backwards. Starting at offset `0`, we write bytes we read + // from offset `aLength` onward and copy a total of `aOffset` + // bytes. + // + // 0 aOffset + // +-----------------------+------------------------+ + // | / / / / / / / / / / / | | + // +-----------------------+------------------------+ + // + // 0 aLength aOffset + aLength + // +---------+-----------------------+------------------------+ + // | | / / / / / / / / / / / | | + // +---------+-----------------------+------------------------+ + // + // 0 aOffset aOffset + aLength + // +-----------------------+---------+------------------------+ + // | / / / / / / / / / / / | N E W | | + // +-----------------------+---------+------------------------+ + // + + SuccessOrExit(error = PrependBytes(nullptr, aLength)); + WriteBytesFromMessage(/* aWriteOffset */ 0, *this, /* aReadOffset */ aLength, /* aLength */ aOffset); + +exit: + return error; +} + void Message::GetFirstChunk(uint16_t aOffset, uint16_t &aLength, Chunk &aChunk) const { // This method gets the first message chunk (contiguous data @@ -647,29 +708,49 @@ void Message::WriteBytes(uint16_t aOffset, const void *aBuf, uint16_t aLength) } } -uint16_t Message::CopyTo(uint16_t aSourceOffset, uint16_t aDestinationOffset, uint16_t aLength, Message &aMessage) const +void Message::WriteBytesFromMessage(uint16_t aWriteOffset, + const Message &aMessage, + uint16_t aReadOffset, + uint16_t aLength) { - uint16_t bytesCopied = 0; - Chunk chunk; - - // This implementing can potentially overwrite the data when bytes are - // being copied forward within the same message, i.e., source and - // destination messages are the same, and source offset is smaller than - // the destination offset. We assert not allowing such a use. - - OT_ASSERT((&aMessage != this) || (aSourceOffset >= aDestinationOffset)); - - GetFirstChunk(aSourceOffset, aLength, chunk); - - while (chunk.GetLength() > 0) + if ((&aMessage != this) || (aReadOffset >= aWriteOffset)) { - aMessage.WriteBytes(aDestinationOffset, chunk.GetBytes(), chunk.GetLength()); - aDestinationOffset += chunk.GetLength(); - bytesCopied += chunk.GetLength(); - GetNextChunk(aLength, chunk); - } + Chunk chunk; - return bytesCopied; + aMessage.GetFirstChunk(aReadOffset, aLength, chunk); + + while (chunk.GetLength() > 0) + { + WriteBytes(aWriteOffset, chunk.GetBytes(), chunk.GetLength()); + aWriteOffset += chunk.GetLength(); + aMessage.GetNextChunk(aLength, chunk); + } + } + else + { + // We are copying bytes within the same message forward. + // To ensure copy forward works, we read and write from + // end of range and move backwards. + + static constexpr uint16_t kBufSize = 32; + + uint8_t buf[kBufSize]; + + aWriteOffset += aLength; + aReadOffset += aLength; + + while (aLength > 0) + { + uint16_t copyLength = Min(kBufSize, aLength); + + aLength -= copyLength; + aReadOffset -= copyLength; + aWriteOffset -= copyLength; + + ReadBytes(aReadOffset, buf, copyLength); + WriteBytes(aWriteOffset, buf, copyLength); + } + } } Message *Message::Clone(uint16_t aLength) const diff --git a/src/core/common/message.hpp b/src/core/common/message.hpp index f194fe5ab..fd70251e9 100644 --- a/src/core/common/message.hpp +++ b/src/core/common/message.hpp @@ -608,13 +608,48 @@ public: } /** - * This method removes header bytes from the message. + * This method removes header bytes from the message at start of message. * - * @param[in] aLength Number of header bytes to remove. + * The caller MUST ensure that message contains the bytes to be removed, i.e. `aOffset` is smaller than the message + * length. + * + * @param[in] aLength Number of header bytes to remove from start of `Message`. * */ void RemoveHeader(uint16_t aLength); + /** + * This method removes header bytes from the message at a given offset. + * + * This method shrinks the message. The existing header bytes before @p aOffset are copied forward and replace the + * removed bytes. + * + * The caller MUST ensure that message contains the bytes to be removed, i.e. `aOffset + aLength` is smaller than + * the message length. + * + * @param[in] aOffset The offset to start removing. + * @param[in] aLength Number of header bytes to remove. + * + */ + void RemoveHeader(uint16_t aOffset, uint16_t aLength); + + /** + * This method grows the message to make space for new header bytes at a given offset. + * + * This method grows the message header (similar to `PrependBytes()`). The existing header bytes from start to + * `aOffset + aLength` are then copied backward to make room for the new header bytes. Note that this method does + * not change the bytes from @p aOffset up @p aLength (the new inserted header range). Caller can write to this + * range to update the bytes after successful return from this method. + * + * @param[in] aOffset The offset at which to insert the header bytes + * @param[in] aLength Number of header bytes to insert. + * + * @retval kErrorNone Successfully grown the message and copied the existing header bytes. + * @retval kErrorNoBufs Insufficient available buffers to grow the message. + * + */ + Error InsertHeader(uint16_t aOffset, uint16_t aLength); + /** * This method appends bytes to the end of the message. * @@ -809,6 +844,23 @@ public: */ void WriteBytes(uint16_t aOffset, const void *aBuf, uint16_t aLength); + /** + * This method writes bytes read from another or potentially the same message to the message at a given offset. + * + * This method will not resize the message. The bytes to write (with @p aLength) MUST fit within the existing + * message buffer (from the given @p aWriteOffset up to the message's length). + * + * This method can be used to copy bytes within the same message in either direction, i.e., copy forward where + * `aWriteOffset > aReadOffset` or copy backward where `aWriteOffset < aReadOffset`. + * + * @param[in] aWriteOffset Byte offset within this message to begin writing. + * @param[in] aMessage The message to read the bytes from. + * @param[in] aReadOffset The offset in @p aMessage to start reading the bytes from. + * @param[in] aLength The number of bytes to read from @p aMessage and write. + * + */ + void WriteBytesFromMessage(uint16_t aWriteOffset, const Message &aMessage, uint16_t aReadOffset, uint16_t aLength); + /** * This methods writes an object to the message. * @@ -845,23 +897,6 @@ public: WriteBytes(aOffset, aData.GetBytes(), aData.GetLength()); } - /** - * This method copies bytes from one message to another. - * - * If source and destination messages are the same, `CopyTo()` can be used to perform a backward copy, but - * it MUST not be used to forward copy within the same message (i.e., when source and destination messages are the - * same and source offset is smaller than the destination offset). - * - * @param[in] aSourceOffset Byte offset within the source message to begin reading. - * @param[in] aDestinationOffset Byte offset within the destination message to begin writing. - * @param[in] aLength Number of bytes to copy. - * @param[in] aMessage Message to copy to. - * - * @returns The number of bytes copied. - * - */ - uint16_t CopyTo(uint16_t aSourceOffset, uint16_t aDestinationOffset, uint16_t aLength, Message &aMessage) const; - /** * This method creates a copy of the message. * diff --git a/src/core/net/ip6.cpp b/src/core/net/ip6.cpp index 3819c4780..ac1dd9b9c 100644 --- a/src/core/net/ip6.cpp +++ b/src/core/net/ip6.cpp @@ -268,8 +268,7 @@ Error Ip6::InsertMplOption(Message &aMessage, Header &aHeader) aMessage.Write(0, hbh); // make space for MPL Option + padding by shifting hop-by-hop option header - SuccessOrExit(error = aMessage.PrependBytes(nullptr, 8)); - aMessage.CopyTo(/* aSourceOffset */ 8, /* aDestOffset */ 0, hbhSize, aMessage); + SuccessOrExit(error = aMessage.InsertHeader(hbh.GetSize(), 8)); // insert MPL Option mMpl.InitOption(mplOption, aHeader.GetSource()); @@ -396,18 +395,7 @@ Error Ip6::RemoveMplOption(Message &aMessage) if (remove) { // last IPv6 Option, shrink HBH Option header - uint8_t buf[8]; - - offset = endOffset - sizeof(buf); - - while (offset >= sizeof(buf)) - { - IgnoreError(aMessage.Read(offset - sizeof(buf), buf)); - aMessage.Write(offset, buf); - offset -= sizeof(buf); - } - - aMessage.RemoveHeader(sizeof(buf)); + aMessage.RemoveHeader(endOffset - 8, 8); if (mplOffset == sizeof(ip6Header) + sizeof(hbh)) { @@ -421,7 +409,7 @@ Error Ip6::RemoveMplOption(Message &aMessage) aMessage.Write(sizeof(ip6Header), hbh); } - ip6Header.SetPayloadLength(ip6Header.GetPayloadLength() - sizeof(buf)); + ip6Header.SetPayloadLength(ip6Header.GetPayloadLength() - 8); aMessage.Write(0, ip6Header); } else if (mplOffset != 0) @@ -649,10 +637,10 @@ Error Ip6::FragmentDatagram(Message &aMessage, uint8_t aIpProto) fragment->SetOffset(aMessage.GetOffset()); fragment->Write(aMessage.GetOffset(), fragmentHeader); - VerifyOrExit(aMessage.CopyTo(aMessage.GetOffset() + FragmentHeader::FragmentOffsetToBytes(offset), - aMessage.GetOffset() + sizeof(fragmentHeader), payloadFragment, - *fragment) == static_cast(payloadFragment), - error = kErrorNoBufs); + fragment->WriteBytesFromMessage( + /* aWriteOffset */ aMessage.GetOffset() + sizeof(fragmentHeader), aMessage, + /* aReadOffset */ aMessage.GetOffset() + FragmentHeader::FragmentOffsetToBytes(offset), + /* aLength */ payloadFragment); EnqueueDatagram(*fragment); @@ -683,11 +671,8 @@ Error Ip6::HandleFragment(Message &aMessage, MessageOrigin aOrigin, MessageInfo Message *message = nullptr; uint16_t offset = 0; uint16_t payloadFragment = 0; - int assertValue = 0; bool isFragmented = true; - OT_UNUSED_VARIABLE(assertValue); - SuccessOrExit(error = aMessage.Read(0, header)); SuccessOrExit(error = aMessage.Read(aMessage.GetOffset(), fragmentHeader)); @@ -727,15 +712,13 @@ Error Ip6::HandleFragment(Message &aMessage, MessageOrigin aOrigin, MessageInfo LogDebg("start reassembly"); VerifyOrExit((message = NewMessage(0)) != nullptr, error = kErrorNoBufs); mReassemblyList.Enqueue(*message); - SuccessOrExit(error = message->SetLength(aMessage.GetOffset())); message->SetTimestampToNow(); message->SetOffset(0); message->SetDatagramTag(fragmentHeader.GetIdentification()); // copying the non-fragmentable header to the fragmentation buffer - assertValue = aMessage.CopyTo(0, 0, aMessage.GetOffset(), *message); - OT_ASSERT(assertValue == aMessage.GetOffset()); + SuccessOrExit(error = message->AppendBytesFromMessage(aMessage, 0, aMessage.GetOffset())); Get().RegisterReceiver(TimeTicker::kIp6FragmentReassembler); } @@ -747,9 +730,9 @@ Error Ip6::HandleFragment(Message &aMessage, MessageOrigin aOrigin, MessageInfo } // copy the fragment payload into the message buffer - assertValue = aMessage.CopyTo(aMessage.GetOffset() + sizeof(fragmentHeader), aMessage.GetOffset() + offset, - payloadFragment, *message); - OT_ASSERT(assertValue == static_cast(payloadFragment)); + message->WriteBytesFromMessage( + /* aWriteOffset */ aMessage.GetOffset() + offset, aMessage, + /* aReadOffset */ aMessage.GetOffset() + sizeof(fragmentHeader), /* aLength */ payloadFragment); // check if it is the last frame if (!fragmentHeader.IsMoreFlagSet()) diff --git a/tests/unit/test_message.cpp b/tests/unit/test_message.cpp index 425a55b8d..d6aa9d330 100644 --- a/tests/unit/test_message.cpp +++ b/tests/unit/test_message.cpp @@ -134,63 +134,90 @@ void TestMessage(void) VerifyOrQuit(message->GetLength() == kMaxSize); - // Test `Message::CopyTo()` behavior. + // Test `WriteBytesFromMessage()` behavior copying between different + // messages. VerifyOrQuit((message2 = messagePool->Allocate(Message::kTypeIp6)) != nullptr); SuccessOrQuit(message2->SetLength(kMaxSize)); - for (uint16_t srcOffset = 0; srcOffset < kMaxSize; srcOffset += kOffsetStep) + for (uint16_t readOffset = 0; readOffset < kMaxSize; readOffset += kOffsetStep) { - for (uint16_t dstOffset = 0; dstOffset < kMaxSize; dstOffset += kOffsetStep) + for (uint16_t writeOffset = 0; writeOffset < kMaxSize; writeOffset += kOffsetStep) { - for (uint16_t length = 0; length <= kMaxSize - dstOffset; length += kLengthStep) + for (uint16_t length = 0; length <= kMaxSize - Max(writeOffset, readOffset); length += kLengthStep) { - uint16_t bytesCopied; - message2->WriteBytes(0, zeroBuffer, kMaxSize); - bytesCopied = message->CopyTo(srcOffset, dstOffset, length, *message2); - - if (srcOffset + length <= kMaxSize) - { - VerifyOrQuit(bytesCopied == length, "CopyTo() failed"); - } - else - { - VerifyOrQuit(bytesCopied == kMaxSize - srcOffset, "CopyTo() failed"); - } + message2->WriteBytesFromMessage(writeOffset, *message, readOffset, length); SuccessOrQuit(message2->Read(0, readBuffer, kMaxSize)); - VerifyOrQuit(memcmp(&readBuffer[0], zeroBuffer, dstOffset) == 0, "read before length"); - VerifyOrQuit(memcmp(&readBuffer[dstOffset], &writeBuffer[srcOffset], bytesCopied) == 0); - VerifyOrQuit( - memcmp(&readBuffer[dstOffset + bytesCopied], zeroBuffer, kMaxSize - bytesCopied - dstOffset) == 0, - "read after length"); + VerifyOrQuit(memcmp(&readBuffer[0], zeroBuffer, writeOffset) == 0); + VerifyOrQuit(memcmp(&readBuffer[writeOffset], &writeBuffer[readOffset], length) == 0); + VerifyOrQuit(memcmp(&readBuffer[writeOffset + length], zeroBuffer, kMaxSize - length - writeOffset) == + 0); - VerifyOrQuit(message->CompareBytes(srcOffset, *message2, dstOffset, bytesCopied)); - VerifyOrQuit(message2->CompareBytes(dstOffset, *message, srcOffset, bytesCopied)); + VerifyOrQuit(message->CompareBytes(readOffset, *message2, writeOffset, length)); + VerifyOrQuit(message2->CompareBytes(writeOffset, *message, readOffset, length)); } } } - // Verify `CopyTo()` with same source and destination message and a backward copy. + // Verify `WriteBytesFromMessage()` behavior copying backwards within + // same message. - for (uint16_t srcOffset = 0; srcOffset < kMaxSize; srcOffset++) + for (uint16_t readOffset = 0; readOffset < kMaxSize; readOffset++) { - uint16_t bytesCopied; + uint16_t length = kMaxSize - readOffset; message->WriteBytes(0, writeBuffer, kMaxSize); - bytesCopied = message->CopyTo(srcOffset, 0, kMaxSize, *message); - VerifyOrQuit(bytesCopied == kMaxSize - srcOffset, "CopyTo() failed"); + message->WriteBytesFromMessage(0, *message, readOffset, length); SuccessOrQuit(message->Read(0, readBuffer, kMaxSize)); - VerifyOrQuit(memcmp(&readBuffer[0], &writeBuffer[srcOffset], bytesCopied) == 0, - "CopyTo() changed before srcOffset"); - VerifyOrQuit(memcmp(&readBuffer[bytesCopied], &writeBuffer[bytesCopied], kMaxSize - bytesCopied) == 0, - "CopyTo() write error"); + VerifyOrQuit(memcmp(&readBuffer[0], &writeBuffer[readOffset], length) == 0); + VerifyOrQuit(memcmp(&readBuffer[length], &writeBuffer[length], kMaxSize - length) == 0); + } + + // Verify `WriteBytesFromMessage()` behavior copying forward within + // same message. + + for (uint16_t writeOffset = 0; writeOffset < kMaxSize; writeOffset++) + { + uint16_t length = kMaxSize - writeOffset; + + message->WriteBytes(0, writeBuffer, kMaxSize); + + message->WriteBytesFromMessage(writeOffset, *message, 0, length); + + SuccessOrQuit(message->Read(0, readBuffer, kMaxSize)); + + VerifyOrQuit(memcmp(&readBuffer[0], &writeBuffer[0], writeOffset) == 0); + VerifyOrQuit(memcmp(&readBuffer[writeOffset], &writeBuffer[0], length) == 0); + } + + // Test `WriteBytesFromMessage()` behavior copying within same + // message at different read and write offsets and lengths. + + for (uint16_t readOffset = 0; readOffset < kMaxSize; readOffset += kOffsetStep) + { + for (uint16_t writeOffset = 0; writeOffset < kMaxSize; writeOffset += kOffsetStep) + { + for (uint16_t length = 0; length <= kMaxSize - Max(writeOffset, readOffset); length += kLengthStep) + { + message->WriteBytes(0, writeBuffer, kMaxSize); + + message->WriteBytesFromMessage(writeOffset, *message, readOffset, length); + + SuccessOrQuit(message->Read(0, readBuffer, kMaxSize)); + + VerifyOrQuit(memcmp(&readBuffer[0], writeBuffer, writeOffset) == 0); + VerifyOrQuit(memcmp(&readBuffer[writeOffset], &writeBuffer[readOffset], length) == 0); + VerifyOrQuit(memcmp(&readBuffer[writeOffset + length], &writeBuffer[writeOffset + length], + kMaxSize - length - writeOffset) == 0); + } + } } // Verify `AppendBytesFromMessage()` with two different messages as source and destination. @@ -236,6 +263,49 @@ void TestMessage(void) message->Free(); message2->Free(); + // Verify `RemoveHeader()` + + for (uint16_t offset = 0; offset < kMaxSize; offset += kOffsetStep) + { + for (uint16_t length = 0; length <= kMaxSize - offset; length += kLengthStep) + { + VerifyOrQuit((message = messagePool->Allocate(Message::kTypeIp6)) != nullptr); + SuccessOrQuit(message->AppendBytes(writeBuffer, kMaxSize)); + + message->RemoveHeader(offset, length); + + VerifyOrQuit(message->GetLength() == kMaxSize - length); + + SuccessOrQuit(message->Read(0, readBuffer, kMaxSize - length)); + + VerifyOrQuit(memcmp(&readBuffer[0], &writeBuffer[0], offset) == 0); + VerifyOrQuit(memcmp(&readBuffer[offset], &writeBuffer[offset + length], kMaxSize - length - offset) == 0); + message->Free(); + } + } + + // Verify `InsertHeader()` + + for (uint16_t offset = 0; offset < kMaxSize; offset += kOffsetStep) + { + for (uint16_t length = 0; length <= kMaxSize; length += kLengthStep) + { + VerifyOrQuit((message = messagePool->Allocate(Message::kTypeIp6)) != nullptr); + SuccessOrQuit(message->AppendBytes(writeBuffer, kMaxSize)); + + SuccessOrQuit(message->InsertHeader(offset, length)); + + VerifyOrQuit(message->GetLength() == kMaxSize + length); + + SuccessOrQuit(message->Read(0, readBuffer, offset)); + VerifyOrQuit(memcmp(&readBuffer[0], &writeBuffer[0], offset) == 0); + + SuccessOrQuit(message->Read(offset + length, readBuffer, kMaxSize - offset)); + VerifyOrQuit(memcmp(&readBuffer[0], &writeBuffer[offset], kMaxSize - offset) == 0); + message->Free(); + } + } + testFreeInstance(instance); }