Add support for signed/bool to BufferReader/Writer (project-chip#27637)

* Add support for signed/bool to BufferReader/Writer

Problem:

- Safe buffer reader/writer doesn't support signed values
- Safe buffer reader/writer doesn't support bools
- Fixes project-chip#27629

This PR:

- Adds signed and bool support to BufferWriter and to Reader
- Adds missing unit tests
- Makes it clear that low-level reads are not to be used directly

Testing done:
- Integration tests pass
- New unit tests added and pass
- Existing unit tests all pass

* Restyled by clang-format

---------

Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>

src/lib/core/CHIPEncoding.h

@@ -216,6 +216,13 @@ namespace LittleEndian {
 template <typename T>
 inline T HostSwap(T v);
 
+// For completeness of the set, we have identity.
+template <>
+inline uint8_t HostSwap<uint8_t>(uint8_t v)
+{
+ return v;
+}
+
 /**
  * This conditionally performs, as necessary for the target system, a
  * byte order swap by value of the specified 16-bit value, presumed to

src/lib/support/BufferReader.cpp

@@ -17,40 +17,46 @@
 
 #include "BufferReader.h"
 
+#include <lib/core/CHIPEncoding.h>
+
 #include <string.h>
+#include <type_traits>
 
 namespace chip {
 namespace Encoding {
 namespace LittleEndian {
 
 namespace {
-// These helper methods return void and put the value being read into an
+
+// This helper methods return void and put the value being read into an
 // outparam because that allows us to easily overload on the type of the
 // thing being read.
-void ReadHelper(const uint8_t *& p, uint8_t * dest)
-{
- *dest = Read8(p);
-}
-void ReadHelper(const uint8_t *& p, uint16_t * dest)
-{
- *dest = Read16(p);
-}
-void ReadHelper(const uint8_t *& p, uint32_t * dest)
+void ReadHelper(const uint8_t * p, bool * dest)
 {
- *dest = Read32(p);
+ *dest = (*p != 0);
 }
-void ReadHelper(const uint8_t *& p, uint64_t * dest)
+
+template <typename T>
+void ReadHelper(const uint8_t * p, T * dest)
 {
- *dest = Read64(p);
+ std::make_unsigned_t<T> result;
+ memcpy(&result, p, sizeof(result));
+ result = chip::Encoding::LittleEndian::HostSwap(result);
+
+ *dest = static_cast<T>(result);
 }
+
 } // anonymous namespace
 
 template <typename T>
-void Reader::RawRead(T * retval)
+void Reader::RawReadLowLevelBeCareful(T * retval)
 {
  static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing");
+ static_assert((-1 & 3) == 3, "LittleEndian::BufferReader only works with 2's complement architectures.");
+
+ VerifyOrReturn(IsSuccess());
 
- static constexpr size_t data_size = sizeof(T);
+ constexpr size_t data_size = sizeof(T);
 
  if (mAvailable < data_size)
  {
@@ -61,6 +67,8 @@ void Reader::RawRead(T * retval)
  }
 
  ReadHelper(mReadPtr, retval);
+ mReadPtr += data_size;
+
  mAvailable = static_cast<uint16_t>(mAvailable - data_size);
 }
 
@@ -84,10 +92,15 @@ Reader & Reader::ReadBytes(uint8_t * dest, size_t size)
 }
 
 // Explicit Read instantiations for the data types we want to support.
-template void Reader::RawRead(uint8_t *);
-template void Reader::RawRead(uint16_t *);
-template void Reader::RawRead(uint32_t *);
-template void Reader::RawRead(uint64_t *);
+template void Reader::RawReadLowLevelBeCareful(bool *);
+template void Reader::RawReadLowLevelBeCareful(int8_t *);
+template void Reader::RawReadLowLevelBeCareful(int16_t *);
+template void Reader::RawReadLowLevelBeCareful(int32_t *);
+template void Reader::RawReadLowLevelBeCareful(int64_t *);
+template void Reader::RawReadLowLevelBeCareful(uint8_t *);
+template void Reader::RawReadLowLevelBeCareful(uint16_t *);
+template void Reader::RawReadLowLevelBeCareful(uint32_t *);
+template void Reader::RawReadLowLevelBeCareful(uint64_t *);
 
 } // namespace LittleEndian
 } // namespace Encoding

src/lib/support/BufferReader.h

@@ -90,6 +90,28 @@ class Reader
  CHECK_RETURN_VALUE
  CHIP_ERROR StatusCode() const { return mStatus; }
 
+ /**
+ * @return false if the reader is in error, true if the reader is OK.
+ */
+ bool IsSuccess() const { return StatusCode() == CHIP_NO_ERROR; }
+
+ /**
+ * Read a bool, assuming single byte storage.
+ *
+ * @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.
+ */
+ CHECK_RETURN_VALUE
+ Reader & ReadBool(bool * dest)
+ {
+ RawReadLowLevelBeCareful(dest);
+ return *this;
+ }
+
  /**
  * Read a single 8-bit unsigned integer.
  *
@@ -103,7 +125,7 @@ class Reader
  CHECK_RETURN_VALUE
  Reader & Read8(uint8_t * dest)
  {
- RawRead(dest);
+ RawReadLowLevelBeCareful(dest);
  return *this;
  }
 
@@ -120,7 +142,7 @@ class Reader
  CHECK_RETURN_VALUE
  Reader & Read16(uint16_t * dest)
  {
- RawRead(dest);
+ RawReadLowLevelBeCareful(dest);
  return *this;
  }
 
@@ -137,7 +159,7 @@ class Reader
  CHECK_RETURN_VALUE
  Reader & Read32(uint32_t * dest)
  {
- RawRead(dest);
+ RawReadLowLevelBeCareful(dest);
  return *this;
  }
 
@@ -154,7 +176,75 @@ class Reader
  CHECK_RETURN_VALUE
  Reader & Read64(uint64_t * dest)
  {
- RawRead(dest);
+ RawReadLowLevelBeCareful(dest);
+ return *this;
+ }
+
+ /**
+ * Read a single 8-bit signed 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.
+ */
+ CHECK_RETURN_VALUE
+ Reader & ReadSigned8(int8_t * dest)
+ {
+ RawReadLowLevelBeCareful(dest);
+ return *this;
+ }
+
+ /**
+ * Read a single 16-bit signed integer.
+ *
+ * @param [out] dest Where the 16-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.
+ */
+ CHECK_RETURN_VALUE
+ Reader & ReadSigned16(int16_t * dest)
+ {
+ RawReadLowLevelBeCareful(dest);
+ return *this;
+ }
+
+ /**
+ * Read a single 32-bit signed integer.
+ *
+ * @param [out] dest Where the 32-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.
+ */
+ CHECK_RETURN_VALUE
+ Reader & ReadSigned32(int32_t * dest)
+ {
+ RawReadLowLevelBeCareful(dest);
+ return *this;
+ }
+
+ /**
+ * Read a single 64-bit signed integer.
+ *
+ * @param [out] dest Where the 64-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.
+ */
+ CHECK_RETURN_VALUE
+ Reader & ReadSigned64(int64_t * dest)
+ {
+ RawReadLowLevelBeCareful(dest);
  return *this;
  }
 
@@ -180,7 +270,7 @@ class Reader
  * delegate to this one.
  */
  template <typename T>
- void RawRead(T * retval);
+ void RawReadLowLevelBeCareful(T * retval);
 
  /**
  * Advance the Reader forward by the specified number of octets.

src/lib/support/BufferWriter.cpp

@@ -60,6 +60,17 @@ LittleEndian::BufferWriter & LittleEndian::BufferWriter::EndianPut(uint64_t x, s
  return *this;
 }
 
+LittleEndian::BufferWriter & LittleEndian::BufferWriter::EndianPutSigned(int64_t x, size_t size)
+{
+ while (size > 0)
+ {
+ Put(static_cast<uint8_t>(x & 0xff));
+ x >>= 8;
+ size--;
+ }
+ return *this;
+}
+
 BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPut(uint64_t x, size_t size)
 {
  while (size-- > 0)
@@ -69,5 +80,14 @@ BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPut(uint64_t x, size_t
  return *this;
 }
 
+BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPutSigned(int64_t x, size_t size)
+{
+ while (size-- > 0)
+ {
+ Put(static_cast<uint8_t>((x >> (size * 8)) & 0xff));
+ }
+ return *this;
+}
+
 } // namespace Encoding
 } // namespace chip

src/lib/support/BufferWriter.h

@@ -104,13 +104,18 @@ class EndianBufferWriterBase : public BufferWriter
  Derived & Put(uint8_t c) { return static_cast<Derived &>(BufferWriter::Put(c)); }
  Derived & Skip(size_t len) { return static_cast<Derived &>(BufferWriter::Skip(len)); }
 
- // write an integer into a buffer, in an endian specific way
+ // write an integer into a buffer, in an endian-specific way
 
  Derived & Put8(uint8_t c) { return static_cast<Derived *>(this)->Put(c); }
  Derived & Put16(uint16_t x) { return static_cast<Derived *>(this)->EndianPut(x, sizeof(x)); }
  Derived & Put32(uint32_t x) { return static_cast<Derived *>(this)->EndianPut(x, sizeof(x)); }
  Derived & Put64(uint64_t x) { return static_cast<Derived *>(this)->EndianPut(x, sizeof(x)); }
 
+ Derived & PutSigned8(int8_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }
+ Derived & PutSigned16(int16_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }
+ Derived & PutSigned32(int32_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }
+ Derived & PutSigned64(int64_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }
+
 protected:
  EndianBufferWriterBase(uint8_t * buf, size_t len) : BufferWriter(buf, len) {}
  EndianBufferWriterBase(MutableByteSpan buf) : BufferWriter(buf.data(), buf.size()) {}
@@ -123,11 +128,15 @@ namespace LittleEndian {
 class BufferWriter : public EndianBufferWriterBase<BufferWriter>
 {
 public:
- BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len) {}
+ BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len)
+ {
+ static_assert((-1 & 3) == 3, "LittleEndian::BufferWriter only works with 2's complement architectures.");
+ }
  BufferWriter(MutableByteSpan buf) : EndianBufferWriterBase<BufferWriter>(buf) {}
  BufferWriter(const BufferWriter & other) = default;
  BufferWriter & operator=(const BufferWriter & other) = default;
  BufferWriter & EndianPut(uint64_t x, size_t size);
+ BufferWriter & EndianPutSigned(int64_t x, size_t size);
 };
 
 } // namespace LittleEndian
@@ -137,11 +146,15 @@ namespace BigEndian {
 class BufferWriter : public EndianBufferWriterBase<BufferWriter>
 {
 public:
- BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len) {}
+ BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len)
+ {
+ static_assert((-1 & 3) == 3, "BigEndian::BufferWriter only works with 2's complement architectures.");
+ }
  BufferWriter(MutableByteSpan buf) : EndianBufferWriterBase<BufferWriter>(buf) {}
  BufferWriter(const BufferWriter & other) = default;
  BufferWriter & operator=(const BufferWriter & other) = default;
  BufferWriter & EndianPut(uint64_t x, size_t size);
+ BufferWriter & EndianPutSigned(int64_t x, size_t size);
 };
 
 } // namespace BigEndian