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 <tennessee.carmelveilleux@gmail.com>

* 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 <bzbarsky@apple.com>

* Address when buffer is nulptr and size is 0

* Fix comment from PR

* Restyle

* add include

Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

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)

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;
  }
 

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
 

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);

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