Fix PersistentStorageDelegate 0-storage behavior (project-chip#16139)

* Fix PersistentStorageDelegate 0-storage behavior

- Make Server's PersistentStorageDelegate and
  TestPersistentStorage delegate properly handle nullptr
  input for zero-length.
- Document behavior expected of PersistentStorageDelegate interface

Issue project-chip#16130

* Add unit tests for TestPersistentStorageDelegate

* Address review comments

* Restyled by clang-format

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>

src/app/server/Server.h

@@ -115,19 +115,51 @@ class Server
     {
         CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
         {
+            uint8_t emptyPlaceholder = 0;
+            if (buffer == nullptr)
+            {
+                if (size != 0)
+                {
+                    return CHIP_ERROR_INVALID_ARGUMENT;
+                }
+                else
+                {
+                    // When size is zero, let's give a non-nullptr to the KVS backend
+                    buffer = &emptyPlaceholder;
+                }
+            }
+
             size_t bytesRead = 0;
             CHIP_ERROR err   = DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size, &bytesRead);
 
+            // Update size only if it made sense
+            if ((CHIP_ERROR_BUFFER_TOO_SMALL == err) || (CHIP_NO_ERROR == err))
+            {
+                size = CanCastTo<uint16_t>(bytesRead) ? static_cast<uint16_t>(bytesRead) : 0;
+            }
+
             if (err == CHIP_NO_ERROR)
             {
                 ChipLogProgress(AppServer, "Retrieved from server storage: %s", key);
             }
-            size = static_cast<uint16_t>(bytesRead);
+
             return err;
         }
 
         CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override
         {
+            uint8_t placeholderForEmpty = 0;
+            if (value == nullptr)
+            {
+                if (size == 0)
+                {
+                    value = &placeholderForEmpty;
+                }
+                else
+                {
+                    return CHIP_ERROR_INVALID_ARGUMENT;
+                }
+            }
             return DeviceLayer::PersistedStorage::KeyValueStoreMgr().Put(key, value, size);
         }
 

src/app/tests/TestBindingTable.cpp

@@ -150,26 +150,26 @@ void TestPersistentStorage(nlTestSuite * aSuite, void * aContext)
     VerifyRestored(aSuite, testStorage, expected);
 
     // Verify storage untouched if add fails
-    testStorage.AddErrorKey(key.BindingTableEntry(4));
+    testStorage.AddPoisonKey(key.BindingTableEntry(4));
     NL_TEST_ASSERT(aSuite, table.Add(EmberBindingTableEntry::ForNode(4, 4, 0, 0, NullOptional)) != CHIP_NO_ERROR);
     VerifyRestored(aSuite, testStorage, expected);
-    testStorage.ClearErrorKey();
+    testStorage.ClearPoisonKeys();
 
     // Verify storage untouched if removing head fails
-    testStorage.AddErrorKey(key.BindingTable());
+    testStorage.AddPoisonKey(key.BindingTable());
     auto iter = table.begin();
     NL_TEST_ASSERT(aSuite, table.RemoveAt(iter) != CHIP_NO_ERROR);
     VerifyTableSame(aSuite, table, expected);
-    testStorage.ClearErrorKey();
+    testStorage.ClearPoisonKeys();
     VerifyRestored(aSuite, testStorage, expected);
 
     // Verify storage untouched if removing other nodes fails
-    testStorage.AddErrorKey(key.BindingTableEntry(0));
+    testStorage.AddPoisonKey(key.BindingTableEntry(0));
     iter = table.begin();
     ++iter;
     NL_TEST_ASSERT(aSuite, table.RemoveAt(iter) != CHIP_NO_ERROR);
     VerifyTableSame(aSuite, table, expected);
-    testStorage.ClearErrorKey();
+    testStorage.ClearPoisonKeys();
     VerifyRestored(aSuite, testStorage, expected);
 
     // Verify removing head

src/lib/core/CHIPPersistentStorageDelegate.h

@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2020 Project CHIP Authors
+ *    Copyright (c) 2020-2022 Project CHIP Authors
  *    All rights reserved.
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
@@ -18,14 +18,22 @@
 
 #pragma once
 
-#include <lib/core/CHIPCore.h>
+#include <lib/core/CHIPError.h>
 #include <lib/support/DLLUtil.h>
+#include <stddef.h>
+#include <stdint.h>
 
 namespace chip {
 
 class DLL_EXPORT PersistentStorageDelegate
 {
 public:
+    /**
+     *  Maximum length of a key required by implementations. Any implementer of PersistentStorageDelegate
+     *  must support keys AT LEAST this long.
+     */
+    static constexpr size_t kKeyLengthMax = 32;
+
     virtual ~PersistentStorageDelegate() {}
 
     /**
@@ -38,35 +46,44 @@ class DLL_EXPORT PersistentStorageDelegate
      *   Caller is responsible to take care of any special formatting needs (e.g. byte
      *   order, null terminators, consistency checks or versioning).
      *
+     *   This API allows for determining the size of a stored value. Whenever
+     *   the passed `size` is smaller than needed and the key exists in storage, the error
+     *   CHIP_ERROR_BUFFER_TOO_SMALL will be given, and the `size` will be updated to the
+     *   size of the stored value. It is legal to use `nullptr` for `buffer` if `size` is 0.
+     *
+     *   If a key is found and the `buffer`'s `size` is large enough, then the value will
+     *   be copied to `buffer` and `size` will be updated to the actual size used.
+     *
+     *   The easiest way to determine if a key exists (and the value's size if so) is to pass
+     *   `size` of 0, which is always valid to do, and will return CHIP_ERROR_BUFFER_TOO_SMALL
+     *   if the key exists and CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if the
+     *   key is not found.
      *
      * @param[in]      key Key to lookup
-     * @param[out]     buffer Value for the key
-     * @param[in, out] size Input value buffer size, output length of value.
-     *                 The output length could be larger than input value. In
-     *                 such cases, the user should allocate the buffer large
-     *                 enough (>= output length), and call the API again.
+     * @param[out]     buffer Pointer to a buffer where the place the read value.
+     * @param[in, out] size Input is maximum buffer size, output updated to length of value.
      *
-     * @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is unknown.
-     * @return CHIP_ERROR_BUFFER_TOO_SMALL the provided buffer is not big
-     *                                     enough.  In this case "size" will
-     *                                     indicate the needed buffer size.
-     *                                     Some data may or may not be placed
-     *                                     in "buffer" in this case; consumers
-     *                                     should not rely on that behavior.
-     *                                     CHIP_ERROR_BUFFER_TOO_SMALL combined
-     *                                     with setting "size" to 0 means the
-     *                                     actual size was too large to fit in
-     *                                     uint16_t.
+     * @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is not found in storage.
+     * @return CHIP_ERROR_BUFFER_TOO_SMALL the provided buffer is not big enough.  In this case
+     *                                     "size" will indicate the needed buffer size. Some data
+     *                                     may or may not be placed in "buffer" in this case; consumers
+     *                                     should not rely on that behavior. CHIP_ERROR_BUFFER_TOO_SMALL
+     *                                     combined with setting "size" to 0 means the actual size was
+     *                                     too large to fit in uint16_t.
      */
     virtual CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) = 0;
 
     /**
      * @brief
-     *   Set the value for the key to a byte buffer.
+     *   Set the value for the key to a byte buffer. Empty values can be stored
+     *   with size == 0, in which case `value` may be nullptr.
+     *
+     * @param[in] key Key to set
+     * @param[in] value Pointer to bytes of value to be set. `value` can only be `nullptr` if size == 0.
+     * @param[in] size Size of the `value` to store.
      *
-     * @param[in] key Key to be set
-     * @param[in] value Value to be set
-     * @param[in] size Size of the Value
+     * @return CHIP_NO_ERROR on success, CHIP_INVALID_ARGUMENT on `value` being `nullptr` with size > 0,
+     *         or another CHIP_ERROR value from implementation on failure.
      */
     virtual CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) = 0;
 
@@ -76,7 +93,8 @@ class DLL_EXPORT PersistentStorageDelegate
      *
      * @param[in] key Key to be deleted
      *
-     * @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is unknown.
+     * @return CHIP_NO_ERROR on success, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is not found in storage,
+     *         or another CHIP_ERROR value from implementation on failure.
      */
     virtual CHIP_ERROR SyncDeleteKeyValue(const char * key) = 0;
 };

src/lib/support/DefaultStorageKeyAllocator.h

@@ -18,6 +18,7 @@
 
 #include <app/ConcreteAttributePath.h>
 #include <app/util/basic-types.h>
+#include <lib/core/CHIPPersistentStorageDelegate.h>
 #include <lib/support/EnforceFormat.h>
 #include <lib/support/logging/Constants.h>
 #include <string.h>
@@ -80,8 +81,6 @@ class DefaultStorageKeyAllocator
     static const char * OTAUpdateToken() { return "o/ut"; }
 
 private:
-    static const size_t kKeyLengthMax = 32;
-
     // The ENFORCE_FORMAT args are "off by one" because this is a class method,
     // with an implicit "this" as first arg.
     const char * ENFORCE_FORMAT(2, 3) Format(const char * format, ...)
@@ -93,7 +92,7 @@ class DefaultStorageKeyAllocator
         return mKeyName;
     }
 
-    char mKeyName[kKeyLengthMax + 1] = { 0 };
+    char mKeyName[PersistentStorageDelegate::kKeyLengthMax + 1] = { 0 };
 };
 
 } // namespace chip

src/lib/support/TestPersistentStorageDelegate.h

@@ -23,66 +23,121 @@
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPPersistentStorageDelegate.h>
 #include <lib/support/DLLUtil.h>
+#include <lib/support/SafeInt.h>
 #include <map>
 #include <set>
 #include <string>
 #include <vector>
 
 namespace chip {
 
+/**
+ * Implementation of PersistentStorageDelegate suitable for unit tests,
+ * where persistence lasts for the object's lifetime and where all data is retained
+ * is memory.
+ *
+ * This version also has "poison keys" which, if accessed, yield an error. This can
+ * be used in unit tests to make sure a module making use of the PersistentStorageDelegate
+ * does not access some particular keys which should remain untouched by underlying
+ * logic.
+ */
 class TestPersistentStorageDelegate : public PersistentStorageDelegate
 {
 public:
     TestPersistentStorageDelegate() {}
 
     CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
     {
-        if (mErrorKeys.find(std::string(key)) != mErrorKeys.end())
+        if ((buffer == nullptr) && (size != 0))
+        {
+            return CHIP_ERROR_INVALID_ARGUMENT;
+        }
+
+        // Making sure poison keys are not accessed
+        if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
         {
             return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
         }
+
         bool contains = mStorage.find(key) != mStorage.end();
         VerifyOrReturnError(contains, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
 
         std::vector<uint8_t> & value = mStorage[key];
-        uint16_t value_size          = static_cast<uint16_t>(value.size());
-        VerifyOrReturnError(value_size <= size, CHIP_ERROR_BUFFER_TOO_SMALL);
-
-        size = std::min(value_size, size);
-        memcpy(buffer, value.data(), size);
-        return CHIP_NO_ERROR;
+        size_t valueSize             = value.size();
+        if (size < valueSize)
+        {
+            size = CanCastTo<uint16_t>(valueSize) ? static_cast<uint16_t>(valueSize) : 0;
+            return CHIP_ERROR_BUFFER_TOO_SMALL;
+        }
+        else
+        {
+            size = static_cast<uint16_t>(valueSize);
+            memcpy(buffer, value.data(), size);
+            return CHIP_NO_ERROR;
+        }
     }
 
     CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override
     {
-        if (mErrorKeys.find(std::string(key)) != mErrorKeys.end())
+        // Make sure poison keys are not accessed
+        if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
         {
             return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
         }
-        const uint8_t * bytes = static_cast<const uint8_t *>(value);
-        mStorage[key]         = std::vector<uint8_t>(bytes, bytes + size);
-        return CHIP_NO_ERROR;
+
+        // Handle empty values
+        if (value == nullptr)
+        {
+            if (size == 0)
+            {
+                mStorage[key] = std::vector<uint8_t>();
+                return CHIP_NO_ERROR;
+            }
+            else
+            {
+                return CHIP_ERROR_INVALID_ARGUMENT;
+            }
+        }
+        // Handle non-empty values
+        else
+        {
+            const uint8_t * bytes = static_cast<const uint8_t *>(value);
+            mStorage[key]         = std::vector<uint8_t>(bytes, bytes + size);
+            return CHIP_NO_ERROR;
+        }
     }
 
     CHIP_ERROR SyncDeleteKeyValue(const char * key) override
     {
-        if (mErrorKeys.find(std::string(key)) != mErrorKeys.end())
+        // Make sure poison keys are not accessed
+        if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
         {
             return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
         }
+
         bool contains = mStorage.find(key) != mStorage.end();
         VerifyOrReturnError(contains, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
         mStorage.erase(key);
         return CHIP_NO_ERROR;
     }
 
-    void AddErrorKey(const std::string & key) { mErrorKeys.insert(key); }
+    /**
+     * @brief Adds a "poison key": a key that, if read/written, implies some bad
+     *        behavior occurred.
+     *
+     * @param key - Poison key to add to the set.
+     */
+    void AddPoisonKey(const std::string & key) { mPoisonKeys.insert(key); }
 
-    void ClearErrorKey() { mErrorKeys.clear(); }
+    /**
+     * @brief Clear all "poison keys"
+     *
+     */
+    void ClearPoisonKeys() { mPoisonKeys.clear(); }
 
 protected:
     std::map<std::string, std::vector<uint8_t>> mStorage;
-    std::set<std::string> mErrorKeys;
+    std::set<std::string> mPoisonKeys;
 };
 
 } // namespace chip

src/lib/support/tests/BUILD.gn

@@ -44,6 +44,7 @@ chip_test_suite("tests") {
     "TestSpan.cpp",
     "TestStateMachine.cpp",
     "TestStringBuilder.cpp",
+    "TestTestPersistentStorageDelegate.cpp",
     "TestThreadOperationalDataset.cpp",
     "TestTimeUtils.cpp",
     "TestTlvToJson.cpp",