From 924ec3846c07486811ae0b6c1a08bc14890b212b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 22 Oct 2024 11:58:46 -0400 Subject: [PATCH 1/6] Add a big endian reader since we seem to potentially want this --- src/lib/support/BufferReader.cpp | 85 +++++-- src/lib/support/BufferReader.h | 246 +++++++++++++++------ src/lib/support/tests/TestBufferReader.cpp | 49 ++-- 3 files changed, 281 insertions(+), 99 deletions(-) diff --git a/src/lib/support/BufferReader.cpp b/src/lib/support/BufferReader.cpp index 02bce8f3fe0615..141280ec4c2230 100644 --- a/src/lib/support/BufferReader.cpp +++ b/src/lib/support/BufferReader.cpp @@ -16,7 +16,9 @@ */ #include "BufferReader.h" +#include "lib/core/CHIPError.h" +#include #include #include @@ -24,6 +26,26 @@ namespace chip { namespace Encoding { + +BufferReader & BufferReader::ReadBytes(uint8_t * dest, size_t size) +{ + static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing"); + + if ((size > UINT16_MAX) || (mAvailable < size)) + { + mStatus = CHIP_ERROR_BUFFER_TOO_SMALL; + // Ensure that future reads all fail. + mAvailable = 0; + return *this; + } + + memcpy(dest, mReadPtr, size); + + mReadPtr += size; + mAvailable = static_cast(mAvailable - size); + return *this; +} + namespace LittleEndian { namespace { @@ -72,25 +94,6 @@ void Reader::RawReadLowLevelBeCareful(T * retval) mAvailable = static_cast(mAvailable - data_size); } -Reader & Reader::ReadBytes(uint8_t * dest, size_t size) -{ - static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing"); - - if ((size > UINT16_MAX) || (mAvailable < size)) - { - mStatus = CHIP_ERROR_BUFFER_TOO_SMALL; - // Ensure that future reads all fail. - mAvailable = 0; - return *this; - } - - memcpy(dest, mReadPtr, size); - - mReadPtr += size; - mAvailable = static_cast(mAvailable - size); - return *this; -} - // Explicit Read instantiations for the data types we want to support. template void Reader::RawReadLowLevelBeCareful(char *); template void Reader::RawReadLowLevelBeCareful(bool *); @@ -104,5 +107,49 @@ template void Reader::RawReadLowLevelBeCareful(uint32_t *); template void Reader::RawReadLowLevelBeCareful(uint64_t *); } // namespace LittleEndian + +namespace BigEndian { + +Reader & Reader::Read16(uint16_t * dest) +{ + if (!EnsureAvailable(sizeof(uint16_t))) + { + return *this; + } + + static_assert(sizeof(*dest) == 2); + + *dest = mReadPtr[0]; + *dest <<= 8; + *dest += mReadPtr[1]; + + mReadPtr += 2; + mAvailable -= 2; + return *this; +} + +Reader & Reader::Read32(uint32_t * dest) +{ + if (!EnsureAvailable(sizeof(uint32_t))) + { + return *this; + } + + static_assert(sizeof(*dest) == 4); + + *dest = 0; + for (unsigned i = 0; i < sizeof(uint32_t); i++) + { + *dest <<= 8; + *dest += mReadPtr[i]; + } + + mReadPtr += sizeof(uint32_t); + mAvailable -= sizeof(uint32_t); + return *this; +} + +} // namespace BigEndian + } // namespace Encoding } // namespace chip diff --git a/src/lib/support/BufferReader.h b/src/lib/support/BufferReader.h index ac9c7145359335..c80e7ffb64de1e 100644 --- a/src/lib/support/BufferReader.h +++ b/src/lib/support/BufferReader.h @@ -31,26 +31,11 @@ namespace chip { namespace Encoding { -namespace LittleEndian { -/** - * @class Reader - * - * Simple reader for reading little-endian things out of buffers. - */ -class Reader +class BufferReader { public: - /** - * Create a buffer reader from a given buffer and length. - * - * @param buffer The octet buffer from which to read. The caller must ensure - * (most simply by allocating the reader on the stack) that - * the buffer outlives the reader. If `buffer` is nullptr, - * length is automatically overridden to zero, to avoid accesses. - * @param buf_len The number of octets in the buffer. - */ - Reader(const uint8_t * buffer, size_t buf_len) : mBufStart(buffer), mReadPtr(buffer), mAvailable(buf_len) + BufferReader(const uint8_t * buffer, size_t buf_len) : mBufStart(buffer), mReadPtr(buffer), mAvailable(buf_len) { if (mBufStart == nullptr) { @@ -58,15 +43,6 @@ class Reader } } - /** - * Create a buffer reader from a given byte span. - * - * @param buffer The octet buffer byte span from which to read. The caller must ensure - * that the buffer outlives the reader. The buffer's ByteSpan .data() pointer - * is is nullptr, length is automatically overridden to zero, to avoid accesses. - */ - Reader(const ByteSpan & buffer) : Reader(buffer.data(), buffer.size()) {} - /** * Number of octets we have read so far. */ @@ -95,6 +71,122 @@ class Reader */ bool IsSuccess() const { return StatusCode() == CHIP_NO_ERROR; } + /** + * Read a byte string from the BufferReader + * + * @param [out] dest Where the bytes read + * @param [in] size How many bytes to read + * + * @note The read can put the reader in a failed-status state if there are + * not enough octets available. Callers must either continue to do + * more reads on the return value or check its status to see whether + * the sequence of reads that has been performed succeeded. + */ + CHECK_RETURN_VALUE + BufferReader & ReadBytes(uint8_t * dest, size_t size); + + /** + * Access bytes of size length, useful for in-place processing of strings + * + * data_ptr MUST NOT be null and will contain the data pointer with `len` bytes available + * if this call is successful + * + * If len is greater than the number of available bytes, the object enters in a failed status. + */ + CHECK_RETURN_VALUE + BufferReader & ZeroCopyProcessBytes(size_t len, const uint8_t ** data_ptr) + { + if (len > mAvailable) + { + *data_ptr = nullptr; + mStatus = CHIP_ERROR_BUFFER_TOO_SMALL; + // Ensure that future reads all fail. + mAvailable = 0; + } + else + { + *data_ptr = mReadPtr; + mReadPtr += len; + mAvailable -= len; + } + return *this; + } + + /** + * Advance the Reader forward by the specified number of octets. + * + * @param len The number of octets to skip. + * + * @note If the len argument is greater than the number of available octets + * remaining, the Reader will advance to the end of the buffer + * without entering a failed-status state. + */ + BufferReader & Skip(size_t len) + { + len = std::min(len, mAvailable); + mReadPtr += len; + mAvailable = static_cast(mAvailable - len); + return *this; + } + +protected: + /// Our buffer start. + const uint8_t * const mBufStart; + + /// Our current read point. + const uint8_t * mReadPtr; + + /// The number of octets we can still read starting at mReadPtr. + size_t mAvailable; + + /// Our current status. + CHIP_ERROR mStatus = CHIP_NO_ERROR; + + + /// Make sure we have at least the given number of bytes available (does not consume them) + bool EnsureAvailable(size_t size) + { + if (mAvailable < size) + { + mStatus = CHIP_ERROR_BUFFER_TOO_SMALL; + // Ensure that future reads all fail. + mAvailable = 0; + return false; + } + return true; + } +}; + +namespace LittleEndian { + +/** + * @class Reader + * + * Simple reader for reading little-endian things out of buffers. + */ +class Reader : public BufferReader +{ +public: + /** + * Create a buffer reader from a given buffer and length. + * + * @param buffer The octet buffer from which to read. The caller must ensure + * (most simply by allocating the reader on the stack) that + * the buffer outlives the reader. If `buffer` is nullptr, + * length is automatically overridden to zero, to avoid accesses. + * @param buf_len The number of octets in the buffer. + */ + Reader(const uint8_t * buffer, size_t buf_len) : BufferReader(buffer, buf_len) {} + + /** + * Create a buffer reader from a given byte span. + * + * @param buffer The octet buffer byte span from which to read. The caller must ensure + * that the buffer outlives the reader. The buffer's ByteSpan .data() pointer + * is is nullptr, length is automatically overridden to zero, to avoid accesses. + */ + Reader(const ByteSpan & buffer) : Reader(buffer.data(), buffer.size()) {} + /** * Read a bool, assuming single byte storage. * @@ -267,20 +359,6 @@ class Reader return *this; } - /** - * Read a byte string from the BufferReader - * - * @param [out] dest Where the bytes read - * @param [in] size How many bytes to read - * - * @note The read can put the reader in a failed-status state if there are - * not enough octets available. Callers must either continue to do - * more reads on the return value or check its status to see whether - * the sequence of reads that has been performed succeeded. - */ - CHECK_RETURN_VALUE - Reader & ReadBytes(uint8_t * dest, size_t size); - /** * Helper for our various APIs so we don't have to write out various logic * multiple times. This is public so that consumers that want to read into @@ -290,46 +368,80 @@ class Reader */ template void RawReadLowLevelBeCareful(T * retval); +}; + +} // namespace LittleEndian + +namespace BigEndian { +/** + * @class Reader + * + * Simple reader for reading big-endian things out of buffers. + */ +class Reader : public BufferReader +{ +public: /** - * Advance the Reader forward by the specified number of octets. - * - * @param len The number of octets to skip. + * Create a buffer reader from a given buffer and length. * - * @note If the len argument is greater than the number of available octets - * remaining, the Reader will advance to the end of the buffer - * without entering a failed-status state. + * @param buffer The octet buffer from which to read. The caller must ensure + * (most simply by allocating the reader on the stack) that + * the buffer outlives the reader. If `buffer` is nullptr, + * length is automatically overridden to zero, to avoid accesses. + * @param buf_len The number of octets in the buffer. */ - Reader & Skip(size_t len) - { - len = std::min(len, mAvailable); - mReadPtr += len; - mAvailable = static_cast(mAvailable - len); - return *this; - } + Reader(const uint8_t * buffer, size_t buf_len) : BufferReader(buffer, buf_len) {} -private: /** - * Our buffer start. + * Create a buffer reader from a given byte span. + * + * @param buffer The octet buffer byte span from which to read. The caller must ensure + * that the buffer outlives the reader. The buffer's ByteSpan .data() pointer + * is is nullptr, length is automatically overridden to zero, to avoid accesses. */ - const uint8_t * const mBufStart; + Reader(const ByteSpan & buffer) : Reader(buffer.data(), buffer.size()) {} /** - * Our current read point. + * Read a single 8-bit unsigned integer. + * + * @param [out] dest Where the 8-bit integer goes. + * + * @note The read can put the reader in a failed-status state if there are + * not enough octets available. Callers must either continue to do + * more reads on the return value or check its status to see whether + * the sequence of reads that has been performed succeeded. */ - const uint8_t * mReadPtr; + CHECK_RETURN_VALUE + Reader & Read8(uint8_t * dest) + { + (void) ReadBytes(dest, 1); + return *this; + } - /** - * The number of octets we can still read starting at mReadPtr. - */ - size_t mAvailable; + CHECK_RETURN_VALUE + Reader & ReadChar(char * dest) + { + (void) ReadBytes(reinterpret_cast(dest), 1); + return *this; + } - /** - * Our current status. - */ - CHIP_ERROR mStatus = CHIP_NO_ERROR; + CHECK_RETURN_VALUE + Reader & ReadBool(char * dest) + { + (void) ReadBytes(reinterpret_cast(dest), 1); + return *this; + } + + /// NOTE: only a subset of reads are supported here, more can be added if used/needed + CHECK_RETURN_VALUE + Reader & Read16(uint16_t * dest); + + CHECK_RETURN_VALUE + Reader & Read32(uint32_t * dest); }; -} // namespace LittleEndian +} // namespace BigEndian + } // namespace Encoding } // namespace chip diff --git a/src/lib/support/tests/TestBufferReader.cpp b/src/lib/support/tests/TestBufferReader.cpp index 97db9cf09737b9..ee100e1506bada 100644 --- a/src/lib/support/tests/TestBufferReader.cpp +++ b/src/lib/support/tests/TestBufferReader.cpp @@ -30,21 +30,21 @@ #include using namespace chip; -using namespace chip::Encoding::LittleEndian; +using namespace chip::Encoding; static const uint8_t test_buffer[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21 }; -struct TestReader : public Reader +struct LittleEndianTestReader : public LittleEndian::Reader { - TestReader() : Reader(test_buffer, std::extent::value) {} + LittleEndianTestReader() : LittleEndian::Reader(test_buffer, std::extent::value) {} }; -struct TestSpanReader : public Reader +struct LittleEndianTestSpanReader : public LittleEndian::Reader { - TestSpanReader() : Reader(ByteSpan{ test_buffer, std::extent::value }) {} + LittleEndianTestSpanReader() : LittleEndian::Reader(ByteSpan{ test_buffer, std::extent::value }) {} }; -static void TestBufferReader_BasicImpl(Reader & reader) +static void TestBufferReader_BasicImpl(LittleEndian::Reader & reader) { uint8_t first; uint16_t second; @@ -75,21 +75,21 @@ static void TestBufferReader_BasicImpl(Reader & reader) TEST(TestBufferReader, TestBufferReader_Basic) { - TestReader reader; + LittleEndianTestReader reader; TestBufferReader_BasicImpl(reader); } TEST(TestBufferReader, TestBufferReader_BasicSpan) { - TestSpanReader reader; + LittleEndianTestSpanReader reader; TestBufferReader_BasicImpl(reader); } TEST(TestBufferReader, TestBufferReader_Saturation) { - TestReader reader; + LittleEndianTestReader reader; uint64_t temp; // Read some bytes out so we can get to the end of the buffer. CHIP_ERROR err = reader.Read64(&temp).StatusCode(); @@ -113,12 +113,13 @@ TEST(TestBufferReader, TestBufferReader_Saturation) TEST(TestBufferReader, TestBufferReader_Skip) { - TestReader reader; + LittleEndianTestReader reader; uint8_t temp = 0; uint16_t firstSkipLen = 2; // Verify Skip() advances the start pointer the correct amount. - CHIP_ERROR err = reader.Skip(firstSkipLen).Read8(&temp).StatusCode(); + reader.Skip(firstSkipLen); + CHIP_ERROR err = reader.Read8(&temp).StatusCode(); EXPECT_EQ(err, CHIP_NO_ERROR); EXPECT_EQ(temp, test_buffer[firstSkipLen]); EXPECT_EQ(reader.OctetsRead(), (firstSkipLen + 1u)); @@ -175,7 +176,8 @@ TEST(TestBufferReader, TestBufferReader_LittleEndianScalars) uint32_t val1 = 0; uint32_t val2 = 0; - EXPECT_TRUE(reader.Skip(1).Read32(&val1).Read32(&val2).IsSuccess()); + reader.Skip(1); + EXPECT_TRUE(reader.Read32(&val1).Read32(&val2).IsSuccess()); EXPECT_EQ(reader.Remaining(), 1u); EXPECT_EQ(val1, static_cast(0xfffffffeUL)); EXPECT_EQ(val2, static_cast(0xffffffffUL)); @@ -227,7 +229,8 @@ TEST(TestBufferReader, TestBufferReader_LittleEndianScalars) int32_t val1 = 0; int32_t val2 = 0; - EXPECT_TRUE(reader.Skip(1).ReadSigned32(&val1).ReadSigned32(&val2).IsSuccess()); + reader.Skip(1); + EXPECT_TRUE(reader.ReadSigned32(&val1).ReadSigned32(&val2).IsSuccess()); EXPECT_EQ(reader.Remaining(), 1u); EXPECT_EQ(val1, static_cast(-2L)); EXPECT_EQ(val2, static_cast(-1L)); @@ -272,3 +275,23 @@ TEST(TestBufferReader, TestBufferReader_LittleEndianScalars) EXPECT_EQ(val3, '\xff'); } } + +TEST(TestBigEndianBufferReader, GenericTests) +{ + uint8_t test_buf[] = { 0x12, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xBC, 1, 2, 3 }; + + chip::Encoding::BigEndian::Reader reader{ ByteSpan{ test_buf } }; + + uint16_t v1; + uint32_t v2; + uint8_t v3; + + EXPECT_TRUE(reader.Read16(&v1).Read32(&v2).Read8(&v3).IsSuccess()); + EXPECT_EQ(reader.Remaining(), 3u); + EXPECT_EQ(v1, 0x1223u); + EXPECT_EQ(v2, 0x456789ABu); + EXPECT_EQ(v3, 0xBCu); + + // Insufficient buffer after that + EXPECT_FALSE(reader.Read32(&v2).IsSuccess()); +} From 2828b4cf46bae9610b502ac367ed817f258ecd6b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 22 Oct 2024 13:38:40 -0400 Subject: [PATCH 2/6] Minor re-arrange --- src/lib/support/BufferReader.cpp | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/lib/support/BufferReader.cpp b/src/lib/support/BufferReader.cpp index 141280ec4c2230..d4724689756904 100644 --- a/src/lib/support/BufferReader.cpp +++ b/src/lib/support/BufferReader.cpp @@ -31,18 +31,13 @@ BufferReader & BufferReader::ReadBytes(uint8_t * dest, size_t size) { static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing"); - if ((size > UINT16_MAX) || (mAvailable < size)) + if (EnsureAvailable(size)) { - mStatus = CHIP_ERROR_BUFFER_TOO_SMALL; - // Ensure that future reads all fail. - mAvailable = 0; - return *this; + memcpy(dest, mReadPtr, size); + mReadPtr += size; + mAvailable -= size; } - memcpy(dest, mReadPtr, size); - - mReadPtr += size; - mAvailable = static_cast(mAvailable - size); return *this; } @@ -80,18 +75,12 @@ void Reader::RawReadLowLevelBeCareful(T * retval) constexpr size_t data_size = sizeof(T); - if (mAvailable < data_size) + if (EnsureAvailable(data_size)) { - mStatus = CHIP_ERROR_BUFFER_TOO_SMALL; - // Ensure that future reads all fail. - mAvailable = 0; - return; + ReadHelper(mReadPtr, retval); + mReadPtr += data_size; + mAvailable -= data_size; } - - ReadHelper(mReadPtr, retval); - mReadPtr += data_size; - - mAvailable = static_cast(mAvailable - data_size); } // Explicit Read instantiations for the data types we want to support. From 86711feb04cc34f059a6290b37b5a517a84e0717 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 22 Oct 2024 13:40:57 -0400 Subject: [PATCH 3/6] Fix includes --- src/lib/support/BufferReader.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/support/BufferReader.cpp b/src/lib/support/BufferReader.cpp index d4724689756904..23452adf9afcd2 100644 --- a/src/lib/support/BufferReader.cpp +++ b/src/lib/support/BufferReader.cpp @@ -16,11 +16,11 @@ */ #include "BufferReader.h" -#include "lib/core/CHIPError.h" -#include +#include #include +#include #include #include From ec85079f20de7d97b09ad6a909a3ed8430666e76 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Tue, 22 Oct 2024 17:41:58 +0000 Subject: [PATCH 4/6] Restyled by clang-format --- src/lib/support/BufferReader.cpp | 2 +- src/lib/support/BufferReader.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib/support/BufferReader.cpp b/src/lib/support/BufferReader.cpp index 23452adf9afcd2..91981543394d87 100644 --- a/src/lib/support/BufferReader.cpp +++ b/src/lib/support/BufferReader.cpp @@ -17,8 +17,8 @@ #include "BufferReader.h" -#include #include +#include #include #include diff --git a/src/lib/support/BufferReader.h b/src/lib/support/BufferReader.h index c80e7ffb64de1e..977e8a76bb39a3 100644 --- a/src/lib/support/BufferReader.h +++ b/src/lib/support/BufferReader.h @@ -142,7 +142,6 @@ class BufferReader /// Our current status. CHIP_ERROR mStatus = CHIP_NO_ERROR; - /// Make sure we have at least the given number of bytes available (does not consume them) bool EnsureAvailable(size_t size) { From 1c54ceec2d2a715d45ca7d0b16f40ee3ea462803 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 22 Oct 2024 13:54:43 -0400 Subject: [PATCH 5/6] make some compilers happy --- src/lib/support/BufferReader.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/lib/support/BufferReader.cpp b/src/lib/support/BufferReader.cpp index 91981543394d87..cbd13f1ed172dc 100644 --- a/src/lib/support/BufferReader.cpp +++ b/src/lib/support/BufferReader.cpp @@ -108,10 +108,7 @@ Reader & Reader::Read16(uint16_t * dest) static_assert(sizeof(*dest) == 2); - *dest = mReadPtr[0]; - *dest <<= 8; - *dest += mReadPtr[1]; - + *dest = static_cast((mReadPtr[0] << 8) + mReadPtr[1]); mReadPtr += 2; mAvailable -= 2; return *this; From 6e1fe8eb69998285ab7a5efb5378fc40d1ce53f8 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 23 Oct 2024 14:27:54 -0400 Subject: [PATCH 6/6] Update src/lib/support/BufferReader.h Co-authored-by: Boris Zbarsky --- src/lib/support/BufferReader.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/support/BufferReader.h b/src/lib/support/BufferReader.h index 977e8a76bb39a3..725ca8e23ea915 100644 --- a/src/lib/support/BufferReader.h +++ b/src/lib/support/BufferReader.h @@ -396,8 +396,8 @@ class Reader : public BufferReader * Create a buffer reader from a given byte span. * * @param buffer The octet buffer byte span from which to read. The caller must ensure - * that the buffer outlives the reader. The buffer's ByteSpan .data() pointer - * is is nullptr, length is automatically overridden to zero, to avoid accesses. + * that the buffer outlives the reader. If the buffer's ByteSpan .data() pointer + * is nullptr, length is automatically overridden to zero, to avoid accesses. */ Reader(const ByteSpan & buffer) : Reader(buffer.data(), buffer.size()) {}