Support raw ByteSpan values correctly in SafeAttributePersistenceProvider

src/app/DefaultAttributePersistenceProvider.cpp

@@ -37,34 +37,40 @@ CHIP_ERROR DefaultAttributePersistenceProvider::InternalWriteValue(const Storage
     return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast<uint16_t>(aValue.size()));
 }
 
-CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType,
-                                                                  size_t aSize, MutableByteSpan & aValue)
+CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue)
 {
     VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
     uint16_t size = static_cast<uint16_t>(min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
     ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size));
-    EmberAfAttributeType type = aType;
-    if (emberAfIsStringAttributeType(type))
+    aValue.reduce_size(size);
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType,
+                                                                  size_t aExpectedSize, MutableByteSpan & aValue)
+{
+    ReturnErrorOnFailure(InternalReadValue(aKey, aValue));
+    size_t size = aValue.size();
+    if (emberAfIsStringAttributeType(aType))
     {
         // Ensure that we've read enough bytes that we are not ending up with
         // un-initialized memory.  Should have read length + 1 (for the length
         // byte).
-        VerifyOrReturnError(size >= emberAfStringLength(aValue.data()) + 1, CHIP_ERROR_INCORRECT_STATE);
+        VerifyOrReturnError(size >= 1 && size - 1 >= emberAfStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE);
     }
-    else if (emberAfIsLongStringAttributeType(type))
+    else if (emberAfIsLongStringAttributeType(aType))
     {
         // Ensure that we've read enough bytes that we are not ending up with
         // un-initialized memory.  Should have read length + 2 (for the length
         // bytes).
-        VerifyOrReturnError(size >= emberAfLongStringLength(aValue.data()) + 2, CHIP_ERROR_INCORRECT_STATE);
+        VerifyOrReturnError(size >= 2 && size - 2 >= emberAfLongStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE);
     }
     else
     {
         // Ensure we got the expected number of bytes for all other types.
-        VerifyOrReturnError(size == aSize, CHIP_ERROR_INVALID_ARGUMENT);
+        VerifyOrReturnError(size == aExpectedSize, CHIP_ERROR_INVALID_ARGUMENT);
     }
-    aValue.reduce_size(size);
     return CHIP_NO_ERROR;
 }
 
@@ -90,8 +96,7 @@ CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAtt
 CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue)
 {
     return InternalReadValue(
-        DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20,
-        aValue.size(), aValue);
+        DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue);
 }
 
 namespace {

src/app/DefaultAttributePersistenceProvider.h

@@ -63,7 +63,9 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider,
 
 private:
     CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue);
-    CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue);
+    CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue);
+    CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aExpectedSize,
+                                 MutableByteSpan & aValue);
 };
 
 } // namespace app

src/app/SafeAttributePersistenceProvider.h

@@ -40,8 +40,8 @@ class SafeAttributePersistenceProvider
 
     // The following API provides helper functions to simplify the access of commonly used types.
     // The API may not be complete.
-    // Currently implemented write and read types are: uint8_t, uint16_t, uint32_t, unit64_t and
-    // their nullable varieties, and bool.
+    // Currently implemented write and read types are: bool, uint8_t, uint16_t, uint32_t, unit64_t and
+    // their nullable varieties, as well as ByteSpans.
 
     /**
      * Write an attribute value of type intX, uintX or bool to non-volatile memory.
@@ -50,7 +50,7 @@ class SafeAttributePersistenceProvider
      * @param [in] aValue the data to write.
      */
     template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
-    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue)
+    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T aValue)
     {
         uint8_t value[sizeof(T)];
         auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T));
@@ -71,18 +71,16 @@ class SafeAttributePersistenceProvider
      *
      * @param [in]     aPath the attribute path for the data being persisted.
      * @param [in,out] aValue where to place the data.
+     *
+     * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute
      */
     template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
     CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue)
     {
         uint8_t attrData[sizeof(T)];
         MutableByteSpan tempVal(attrData);
-        auto err = SafeReadValue(aPath, tempVal);
-        if (err != CHIP_NO_ERROR)
-        {
-            return err;
-        }
-
+        ReturnErrorOnFailure(SafeReadValue(aPath, tempVal));
+        VerifyOrReturnError(tempVal.size() == sizeof(T), CHIP_ERROR_INCORRECT_STATE);
         Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size());
         r.RawReadLowLevelBeCareful(&aValue);
         return r.StatusCode();
@@ -95,7 +93,7 @@ class SafeAttributePersistenceProvider
      * @param [in] aValue the data to write.
      */
     template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
-    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
+    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, const DataModel::Nullable<T> & aValue)
     {
         typename NumericAttributeTraits<T>::StorageType storageValue;
         if (aValue.IsNull())
@@ -114,16 +112,14 @@ class SafeAttributePersistenceProvider
      *
      * @param [in]     aPath the attribute path for the data being persisted.
      * @param [in,out] aValue where to place the data.
+     *
+     * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute
      */
     template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
     CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
     {
         typename NumericAttributeTraits<T>::StorageType storageValue;
-        CHIP_ERROR err = ReadScalarValue(aPath, storageValue);
-        if (err != CHIP_NO_ERROR)
-        {
-            return err;
-        }
+        ReturnErrorOnFailure(ReadScalarValue(aPath, storageValue));
 
         if (NumericAttributeTraits<T>::IsNullValue(storageValue))
         {
@@ -137,25 +133,25 @@ class SafeAttributePersistenceProvider
         return CHIP_NO_ERROR;
     }
 
-protected:
     /**
      * Write an attribute value from the attribute store (i.e. not a struct or
      * list) to non-volatile memory.
      *
      * @param [in] aPath the attribute path for the data being written.
-     * @param [in] aValue the data to write. The value should be stored as-is.
+     * @param [in] aValue the data to write. The value will be stored as-is.
      */
     virtual CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) = 0;
 
     /**
-     * Read an attribute value from non-volatile memory.
-     * It can be assumed that this method will never be called upon to read
-     * an attribute of type string or long-string.
+     * Read an attribute value as a raw sequence of bytes from non-volatile memory.
      *
      * @param [in]     aPath the attribute path for the data being persisted.
      * @param [in,out] aValue where to place the data.  The size of the buffer
-     *                 will be equal to `aValue.size()`.  The callee is expected to adjust
-     *                 aValue's size to the actual number of bytes read.
+     *                 will be equal to `aValue.size()`. On success aValue.size()
+     *                 will be the actual number of bytes read.
+     *
+     * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute
+     * @retval CHIP_ERROR_BUFFER_TOO_SMALL aValue.size() is too small to hold the value.
      */
     virtual CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) = 0;
 };

src/app/tests/TestAttributePersistenceProvider.cpp

@@ -59,7 +59,13 @@ TEST_F(TestAttributePersistenceProvider, TestStorageAndRetrivalByteSpans)
     MutableByteSpan valueReadBack(getArray);
     err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack);
     EXPECT_EQ(err, CHIP_NO_ERROR);
-    EXPECT_TRUE(std::equal(valueReadBack.begin(), valueReadBack.end(), value.begin(), value.end()));
+    EXPECT_TRUE(valueReadBack.data_equal(value));
+
+    uint8_t getArrayThatIsLongerThanNeeded[10];
+    MutableByteSpan valueReadBack2(getArrayThatIsLongerThanNeeded);
+    err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack2);
+    EXPECT_EQ(err, CHIP_NO_ERROR);
+    EXPECT_TRUE(valueReadBack2.data_equal(value));
 
     // Finishing
     persistenceProvider.Shutdown();
@@ -320,6 +326,7 @@ TEST_F(TestAttributePersistenceProvider, TestBufferTooSmallErrors)
     err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBackByteSpan8);
     EXPECT_EQ(err, CHIP_ERROR_BUFFER_TOO_SMALL);
 
+    // TODO: ReadScalarValue() does not take a buffer, so expecting CHIP_ERROR_BUFFER_TOO_SMALL is bad API
     // Fail to get value as uint8_t
     uint8_t valueReadBack8;
     err = persistenceProvider.ReadScalarValue(TestConcretePath, valueReadBack8);