From 62df7e9267974ffec5608555b9b0b0619a3f08e3 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Tue, 17 Sep 2024 14:06:27 -0700 Subject: [PATCH] [message] clarify partial read behavior of `Read()` vs `ReadByte()` (#10719) This commit updates and clarifies the behavior of `Message::ReadByte()` and `Message::Read()` overloads regarding partial reads. - `ReadByte()` will read the available bytes and return the actual number of bytes read if fewer bytes are available in the message than the requested read length. This behavior remains unchanged. The documentation is updated to emphasize this behavior. - `Read()` methods return `kErrorParse` if the requested length cannot be read. This is the existing behavior which remains unchanged. Previously, `Read()` methods would still perform a partial read and populate the buffer/object with as many bytes that could be read, even in case of failure and returning `kErrorParse`. This behavior has been changed in this commit so the method will skip reading/copying bytes if the full length cannot be read. This aligns the documentation and behavior with how the `Read()` methods are used and intended to be used within the OT stack. --- src/core/common/message.cpp | 10 ++++++++-- src/core/common/message.hpp | 24 ++++++++++-------------- tests/unit/test_message.cpp | 26 ++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/src/core/common/message.cpp b/src/core/common/message.cpp index 1b25109c5..07cfd1612 100644 --- a/src/core/common/message.cpp +++ b/src/core/common/message.cpp @@ -660,7 +660,13 @@ uint16_t Message::ReadBytes(const OffsetRange &aOffsetRange, void *aBuf) const Error Message::Read(uint16_t aOffset, void *aBuf, uint16_t aLength) const { - return (ReadBytes(aOffset, aBuf, aLength) == aLength) ? kErrorNone : kErrorParse; + Error error = kErrorNone; + + VerifyOrExit(aOffset + aLength <= GetLength(), error = kErrorParse); + ReadBytes(aOffset, aBuf, aLength); + +exit: + return error; } Error Message::Read(const OffsetRange &aOffsetRange, void *aBuf, uint16_t aLength) const @@ -668,7 +674,7 @@ Error Message::Read(const OffsetRange &aOffsetRange, void *aBuf, uint16_t aLengt Error error = kErrorNone; VerifyOrExit(aOffsetRange.Contains(aLength), error = kErrorParse); - VerifyOrExit(ReadBytes(aOffsetRange.GetOffset(), aBuf, aLength) == aLength, error = kErrorParse); + error = Read(aOffsetRange.GetOffset(), aBuf, aLength); exit: return error; diff --git a/src/core/common/message.hpp b/src/core/common/message.hpp index 64d1307b4..d53aa2a5c 100644 --- a/src/core/common/message.hpp +++ b/src/core/common/message.hpp @@ -762,6 +762,12 @@ public: /** * Reads bytes from the message. * + * The provided buffer @p aBuf MUST contain at least @p aLength bytes. + * + * If there are fewer bytes available in the message than the requested @p aLength, the available bytes are read + * and copied into @p aBuf. This method returns the actual number of bytes successfully read from the message and + * written into @p aBuf. + * * @param[in] aOffset Byte offset within the message to begin reading. * @param[out] aBuf A pointer to a data buffer to copy the read bytes into. * @param[in] aLength Number of bytes to read. @@ -774,6 +780,10 @@ public: /** * Reads bytes from the message. * + * If there are fewer bytes available in the message than the provided length in @p aOffsetRange, the available + * bytes are read and copied into @p aBuf. This method returns the actual number of bytes successfully read from + * the message and written into @p aBuf. + * * @param[in] aOffsetRange The offset range in the message to read bytes from. * @param[out] aBuf A pointer to a data buffer to copy the read bytes into. * @@ -785,9 +795,6 @@ public: /** * Reads a given number of bytes from the message. * - * If there are fewer bytes available in the message than the requested read length, the available bytes will be - * read and copied into @p aBuf. In this case `kErrorParse` will be returned. - * * @param[in] aOffset Byte offset within the message to begin reading. * @param[out] aBuf A pointer to a data buffer to copy the read bytes into. * @param[in] aLength Number of bytes to read. @@ -801,9 +808,6 @@ public: /** * Reads a given number of bytes from the message. * - * If there are fewer bytes available in the message or @p aOffsetRange than the requested @p aLength, the - * available bytes are read and copied into @p aBuf. In this case `kErrorParse` will be returned. - * * @param[in] aOffsetRange The offset range in the message to read from. * @param[out] aBuf A pointer to a data buffer to copy the read bytes into. * @param[in] aLength Number of bytes to read. @@ -817,10 +821,6 @@ public: /** * Reads an object from the message. * - * If there are fewer bytes available in the message than the requested object size, the available bytes will be - * read and copied into @p aObject (@p aObject will be read partially). In this case `kErrorParse` will - * be returned. - * * @tparam ObjectType The object type to read from the message. * * @param[in] aOffset Byte offset within the message to begin reading. @@ -840,10 +840,6 @@ public: /** * Reads an object from the message. * - * If there are fewer bytes available in the message or @p aOffsetRange than the requested object size, the - * available bytes will be read and copied into @p aObject (@p aObject will be read partially). In this case - * `kErrorParse` will be returned. - * * @tparam ObjectType The object type to read from the message. * * @param[in] aOffsetRange The offset range in the message to read from. diff --git a/tests/unit/test_message.cpp b/tests/unit/test_message.cpp index ac1320095..666548cc7 100644 --- a/tests/unit/test_message.cpp +++ b/tests/unit/test_message.cpp @@ -157,6 +157,32 @@ void TestMessage(void) VerifyOrQuit(!message->CompareBytes(offset, readBuffer, length)); VerifyOrQuit(message->CompareBytes(offset, readBuffer, readLength)); } + + // Verify `Read()` behavior when requested read length goes beyond available bytes in the message. + + for (uint16_t length = kMaxSize - offset + 1; length <= kMaxSize + 1; length++) + { + Error error; + + memset(readBuffer, 0, sizeof(readBuffer)); + + error = message->Read(offset, readBuffer, length); + + if (length < kMaxSize - offset) + { + uint16_t readLength = kMaxSize - offset; + + SuccessOrQuit(error); + VerifyOrQuit(memcmp(readBuffer, &writeBuffer[offset], readLength) == 0); + VerifyOrQuit(memcmp(&readBuffer[readLength], zeroBuffer, kMaxSize - readLength) == 0, + "read after length"); + } + else + { + VerifyOrQuit(error == kErrorParse); + VerifyOrQuit(memcmp(readBuffer, zeroBuffer, sizeof(readBuffer)) == 0, "Read() updated buffer on error"); + } + } } VerifyOrQuit(message->GetLength() == kMaxSize);