Add support for signed/bool to BufferReader/Writer (#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 #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: [email protected] <[email protected]> Co-authored-by: Restyled.io <[email protected]>
project-chip · Nov 14, 2023 · 1494988 · 1494988
1 parent 70a685f
commit 1494988
Show file tree

Hide file tree

Showing 7 changed files with 371 additions and 29 deletions.
diff --git a/src/lib/core/CHIPEncoding.h b/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

diff --git a/src/lib/support/BufferReader.cpp b/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

diff --git a/src/lib/support/BufferReader.h b/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.

diff --git a/src/lib/support/BufferWriter.cpp b/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
diff --git a/src/lib/support/BufferWriter.h b/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