Change persistent storage SyncGetKeyValue API to match constraints of…

… KvsStorageManager (#20164) * PersistentStorageDelegate::SyncGetKeyValue remove ability to get value size Some implmentation of PersistentStorageDelegate use KeyValueStoreManager which is not capable of giving the size of for the time being, which makes this obligation impossible to fulfill right now. After performing a code audit no one is currently using the functionality to get the size, so this change is safe, but we need platforms to adhear to the this updated description. We also are now enforcing the PersistentStorageDelegate::SyncGetKeyValue API such that we fill up the buffer up to the size provided even if the buffer is not large enough. This is done so that in the case a version downgrade needs to happen that (due to OTA revert, security reason, or any other reason) we can make sure as long as TLV data was stored you could still ensure that you are able to work in critical situations. * Update implemenations of PersistentStorageDelegate::SyncGetKeyValue * Restyle * Restyle * Fix a couple things * Address comments from PR * Restyle * Fix CI issue * Address PR comments * Update src/lib/core/CHIPPersistentStorageDelegate.h Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Address PR comments, add audit test * Restyle * Address PR comments * Add test for base64 symbol in key and value * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Address when buffer is nulptr and size is 0 * Fix comment from PR * Restyle * add include Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Sep 11, 2023 · 1361228 · 1361228
1 parent f0d5028
commit 1361228
Show file tree

Hide file tree

Showing 8 changed files with 492 additions and 98 deletions.
diff --git a/examples/chip-tool/config/PersistentStorage.cpp b/examples/chip-tool/config/PersistentStorage.cpp
@@ -103,6 +103,8 @@ CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, ui
 {
     std::string iniValue;
 
+    ReturnErrorCodeIf(((value == nullptr) && (size != 0)), CHIP_ERROR_INVALID_ARGUMENT);
+
     auto section = mConfig.sections[kDefaultSectionName];
     auto it      = section.find(key);
     ReturnErrorCodeIf(it == section.end(), CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
@@ -112,16 +114,14 @@ CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, ui
     iniValue = Base64ToString(iniValue);
 
     uint16_t dataSize = static_cast<uint16_t>(iniValue.size());
-    if (dataSize > size)
-    {
-        size = dataSize;
-        return CHIP_ERROR_BUFFER_TOO_SMALL;
-    }
+    ReturnErrorCodeIf(size == 0 && dataSize == 0, CHIP_NO_ERROR);
+    ReturnErrorCodeIf(value == nullptr, CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    size = dataSize;
-    memcpy(value, iniValue.data(), dataSize);
+    uint16_t sizeToCopy = std::min(size, dataSize);
 
-    return CHIP_NO_ERROR;
+    memcpy(value, iniValue.data(), sizeToCopy);
+    size = sizeToCopy;
+    return size < dataSize ? CHIP_ERROR_BUFFER_TOO_SMALL : CHIP_NO_ERROR;
 }
 
 CHIP_ERROR PersistentStorage::SyncSetKeyValue(const char * key, const void * value, uint16_t size)

diff --git a/src/controller/python/ChipDeviceController-StorageDelegate.cpp b/src/controller/python/ChipDeviceController-StorageDelegate.cpp
@@ -24,35 +24,29 @@
 #include <string>
 
 #include <lib/core/CHIPPersistentStorageDelegate.h>
+#include <lib/support/CodeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 
 namespace chip {
 namespace Controller {
 
 CHIP_ERROR PythonPersistentStorageDelegate::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
 {
+    ReturnErrorCodeIf(((value == nullptr) && (size != 0)), CHIP_ERROR_INVALID_ARGUMENT);
+
     auto val = mStorage.find(key);
     if (val == mStorage.end())
     {
         return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
     }
 
-    if (value == nullptr)
-    {
-        size = 0;
-    }
-
     uint16_t neededSize = val->second.size();
-    if (size == 0)
-    {
-        size = neededSize;
-        return CHIP_ERROR_BUFFER_TOO_SMALL;
-    }
+    ReturnErrorCodeIf(size == 0 && neededSize == 0, CHIP_NO_ERROR);
+    ReturnErrorCodeIf(value == nullptr, CHIP_ERROR_BUFFER_TOO_SMALL);
 
     if (size < neededSize)
     {
         memcpy(value, val->second.data(), size);
-        size = neededSize;
         return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
 
@@ -86,6 +80,10 @@ namespace Python {
 CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
 {
     ChipLogDetail(Controller, "StorageAdapter::GetKeyValue: Key = %s, Value = %p (%u)", key, value, size);
+    if ((value == nullptr) && (size != 0))
+    {
+        return CHIP_ERROR_INVALID_ARGUMENT;
+    }
 
     uint16_t tmpSize = size;
 
@@ -99,7 +97,6 @@ CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint1
     if (size < tmpSize)
     {
         ChipLogDetail(Controller, "Buf not big enough\n");
-        size = tmpSize;
         return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
 

diff --git a/src/controller/python/chip/storage/__init__.py b/src/controller/python/chip/storage/__init__.py
@@ -46,23 +46,34 @@ def _OnSyncSetKeyValueCb(storageObj, key: str, value, size):
 
 @_SyncGetKeyValueCbFunct
 def _OnSyncGetKeyValueCb(storageObj, key: str, value, size):
+    ''' This does not adhere to the API requirements of
+    PersistentStorageDelegate::SyncGetKeyValue, but that is okay since
+    the C++ storage binding layer is capable of adapting results from
+    this method to the requirements of
+    PersistentStorageDelegate::SyncGetKeyValue.
+    '''
     try:
         keyValue = storageObj.GetSdkKey(key.decode("utf-8"))
     except Exception as ex:
         keyValue = None
 
     if (keyValue):
-        if (size[0] < len(keyValue)):
-            size[0] = len(keyValue)
-            return
+        sizeOfValue = size[0]
+        sizeToCopy = min(sizeOfValue, len(keyValue))
 
         count = 0
 
         for idx, val in enumerate(keyValue):
+            if sizeToCopy == count:
+                break
             value[idx] = val
             count = count + 1
 
-        size[0] = count
+        # As mentioned above, we are intentionally not returning
+        # sizeToCopy as one might expect because the caller
+        # will use the value in size[0] to determine if it should
+        # return CHIP_ERROR_BUFFER_TOO_SMALL.
+        size[0] = len(keyValue)
     else:
         size[0] = 0
 

diff --git a/src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm b/src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm
@@ -45,19 +45,17 @@
         }
 
         if ([value length] > UINT16_MAX) {
-            error = CHIP_ERROR_BUFFER_TOO_SMALL;
-            size = 0;
+            error = CHIP_ERROR_PERSISTED_STORAGE_FAILED;
             return;
         }
 
         uint16_t valueSize = static_cast<uint16_t>([value length]);
         if (valueSize > size) {
             error = CHIP_ERROR_BUFFER_TOO_SMALL;
+        } else {
             size = valueSize;
-            return;
         }
 
-        size = valueSize;
         if (size != 0) {
             // buffer is known to be non-null here.
             memcpy(buffer, [value bytes], size);

diff --git a/src/lib/core/CHIPPersistentStorageDelegate.h b/src/lib/core/CHIPPersistentStorageDelegate.h
@@ -46,30 +46,26 @@ 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.
+     *   Whenever the passed `size` is smaller than the size of the stored value for the given key,
+     *   CHIP_ERROR_BUFFER_TOO_SMALL will be returned, but the `buffer` will still be filled with the
+     *   first `size` bytes of the stored value.
+     *
+     *   In the case where `size` of 0 is given, and the value stored is of size 0, CHIP_NO_ERROR is
+     *   returned. It is recommended to use helper method SyncDoesKeyExist(key) to determine if key
+     *   exists.
+     *
+     *   It is legal to use `nullptr` for `buffer` if `size` is 0.
      *
      * @param[in]      key Key to lookup
      * @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.
+     * @param[in, out] size Input is maximum buffer size, output updated to number of bytes written into `buffer`.
      *
      * @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.
+     *                                     the first `size` bytes of the value will be placed in `buffer`.
      */
     virtual CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) = 0;
 
@@ -97,6 +93,23 @@ class DLL_EXPORT PersistentStorageDelegate
      *         or another CHIP_ERROR value from implementation on failure.
      */
     virtual CHIP_ERROR SyncDeleteKeyValue(const char * key) = 0;
+
+    /**
+     * @brief
+     *   Helper function that identifies if a key exists.
+     *
+     *   This may be overridden to provide an implementation that is simpler or more direct.
+     *
+     * @param[in] key Key to check if it exist
+     *
+     * @return true if key exists in storage. It returns false if key does not exist in storage or an internal error arises.
+     */
+    virtual bool SyncDoesKeyExist(const char * key)
+    {
+        uint16_t size  = 0;
+        CHIP_ERROR err = SyncGetKeyValue(key, nullptr, size);
+        return (err == CHIP_ERROR_BUFFER_TOO_SMALL) || (err == CHIP_NO_ERROR);
+    }
 };
 
 } // namespace chip