From 2999b7181cecb444b16a324cc65e989d0ff1f635 Mon Sep 17 00:00:00 2001 From: Seth Rickard Date: Tue, 23 Aug 2022 10:22:32 -0500 Subject: [PATCH] Update KVS to handle zero length elements (#21977) * allow for nullptr in KVS write * complete audit with chip_support_enable_storage_api_audit=true --- .../cc13x2_26x2/CC13X2_26X2Config.cpp | 83 +++++++++++-------- .../cc13x2_26x2/KeyValueStoreManagerImpl.cpp | 7 +- .../cc32xx/KeyValueStoreManagerImpl.cpp | 7 +- 3 files changed, 57 insertions(+), 40 deletions(-) diff --git a/src/platform/cc13x2_26x2/CC13X2_26X2Config.cpp b/src/platform/cc13x2_26x2/CC13X2_26X2Config.cpp index 09e3393e07b1b1..8d25d6673f5ea1 100644 --- a/src/platform/cc13x2_26x2/CC13X2_26X2Config.cpp +++ b/src/platform/cc13x2_26x2/CC13X2_26X2Config.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #include @@ -165,7 +166,7 @@ CHIP_ERROR CC13X2_26X2Config::ReadConfigValueBin(Key key, uint8_t * buf, size_t /* Iterate through the key range to find a key that matches. */ static uint8_t FindKVSSubID(const char * key, uint16_t & subID) { - char key_scratch[32]; // 32 characters seems large enough for a key + char key_scratch[PersistentStorageDelegate::kKeyLengthMax + 1]; NVINTF_nvProxy_t nvProxy = { 0 }; uint8_t status = NVINTF_SUCCESS; @@ -209,20 +210,23 @@ CHIP_ERROR CC13X2_26X2Config::ReadKVS(const char * key, void * value, size_t val val_item.subID = subID; len = sNvoctpFps.getItemLen(val_item); - VerifyOrExit(len > 0, err = CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND); // key not found - if ((offset_bytes + value_size) > len) + if (value_size >= (len - offset_bytes)) { - // trying to read up to the end of the element + // reading to end of element read_len = len - offset_bytes; } else { read_len = value_size; + err = CHIP_ERROR_BUFFER_TOO_SMALL; } - VerifyOrExit(sNvoctpFps.readItem(val_item, (uint16_t) offset_bytes, read_len, value) == NVINTF_SUCCESS, - err = CHIP_ERROR_PERSISTED_STORAGE_FAILED); + if (read_len > 0) + { + VerifyOrExit(sNvoctpFps.readItem(val_item, (uint16_t) offset_bytes, read_len, value) == NVINTF_SUCCESS, + err = CHIP_ERROR_PERSISTED_STORAGE_FAILED); + } if (read_bytes_size) { @@ -264,22 +268,14 @@ CHIP_ERROR CC13X2_26X2Config::WriteKVS(const char * key, const void * value, siz CHIP_ERROR err = CHIP_NO_ERROR; uint16_t subID; - if (FindKVSSubID(key, subID) == NVINTF_SUCCESS) - { - NVINTF_itemID_t val_item = CC13X2_26X2Config::kConfigKey_KVS_value.nvID; - // key already exists, update value - val_item.subID = subID; - VerifyOrExit(sNvoctpFps.updateItem(val_item, (uint16_t) value_size, (void *) value) == NVINTF_SUCCESS, - err = CHIP_ERROR_PERSISTED_STORAGE_FAILED); - } - else + NVINTF_itemID_t key_item = CC13X2_26X2Config::kConfigKey_KVS_key.nvID; + NVINTF_itemID_t val_item = CC13X2_26X2Config::kConfigKey_KVS_value.nvID; + + if (FindKVSSubID(key, subID) != NVINTF_SUCCESS) { - // key does not exist, likely case + // key item not found, find an empty subID intptr_t lock_key = sNvoctpFps.lockNV(); - NVINTF_itemID_t key_item = CC13X2_26X2Config::kConfigKey_KVS_key.nvID; - NVINTF_itemID_t val_item = CC13X2_26X2Config::kConfigKey_KVS_value.nvID; - /* Iterate through the subID range to find an unused subID in the * keyspace. SubID is a 10 bit value, reference * `/source/ti/common/nv/nvocmp.c:MVOCMP_MAXSUBID`. @@ -289,26 +285,39 @@ CHIP_ERROR CC13X2_26X2Config::WriteKVS(const char * key, const void * value, siz key_item.subID = i; if (sNvoctpFps.getItemLen(key_item) == 0U) { - val_item.subID = i; + subID = i; break; } } + sNvoctpFps.unlockNV(lock_key); + // write they key item - if (sNvoctpFps.writeItem(key_item, (uint16_t) strlen(key), (void *) key) == NVINTF_SUCCESS) + VerifyOrExit(sNvoctpFps.writeItem(key_item, (uint16_t) strlen(key), (void *) key) == NVINTF_SUCCESS, + err = CHIP_ERROR_PERSISTED_STORAGE_FAILED); + } + + key_item.subID = subID; + val_item.subID = subID; + + if (value_size == 0U) + { + // delete the value item if it exists + int8_t ret = sNvoctpFps.deleteItem(val_item); + if (ret != NVINTF_SUCCESS && ret != NVINTF_NOTFOUND) { - if (sNvoctpFps.writeItem(val_item, (uint16_t) value_size, (void *) value) != NVINTF_SUCCESS) - { - // try to delete the key item - sNvoctpFps.deleteItem(key_item); - err = CHIP_ERROR_PERSISTED_STORAGE_FAILED; - } + err = CHIP_ERROR_PERSISTED_STORAGE_FAILED; } - else + } + else + { + if (sNvoctpFps.writeItem(val_item, (uint16_t) value_size, (void *) value) != NVINTF_SUCCESS) { + // try to delete the key item + sNvoctpFps.deleteItem(key_item); err = CHIP_ERROR_PERSISTED_STORAGE_FAILED; } - sNvoctpFps.unlockNV(lock_key); } + exit: return err; } @@ -325,25 +334,31 @@ CHIP_ERROR CC13X2_26X2Config::WriteConfigValueBin(Key key, const uint8_t * data, CHIP_ERROR CC13X2_26X2Config::ClearKVS(const char * key) { - CHIP_ERROR err = CHIP_NO_ERROR; + CHIP_ERROR err = CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; uint16_t subID; NVINTF_itemID_t key_item = CC13X2_26X2Config::kConfigKey_KVS_key.nvID; NVINTF_itemID_t val_item = CC13X2_26X2Config::kConfigKey_KVS_value.nvID; if (FindKVSSubID(key, subID) == NVINTF_SUCCESS) { + int8_t ret; + key_item.subID = subID; val_item.subID = subID; - // delete the value item - if (sNvoctpFps.deleteItem(val_item) != NVINTF_SUCCESS) + // delete the value item if it exists + ret = sNvoctpFps.deleteItem(val_item); + if (ret != NVINTF_SUCCESS && ret != NVINTF_NOTFOUND) { err = CHIP_ERROR_PERSISTED_STORAGE_FAILED; } - // delete the key item - if (sNvoctpFps.deleteItem(key_item) != NVINTF_SUCCESS) + // delete the key item if it exists + ret = sNvoctpFps.deleteItem(key_item); + if (ret != NVINTF_SUCCESS && ret != NVINTF_NOTFOUND) { err = CHIP_ERROR_PERSISTED_STORAGE_FAILED; } + + err = CHIP_NO_ERROR; } return err; diff --git a/src/platform/cc13x2_26x2/KeyValueStoreManagerImpl.cpp b/src/platform/cc13x2_26x2/KeyValueStoreManagerImpl.cpp index b74146c1de3704..5227b55e5e65df 100644 --- a/src/platform/cc13x2_26x2/KeyValueStoreManagerImpl.cpp +++ b/src/platform/cc13x2_26x2/KeyValueStoreManagerImpl.cpp @@ -44,7 +44,10 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t CHIP_ERROR err; VerifyOrReturnError(key, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(value, CHIP_ERROR_INVALID_ARGUMENT); + if (0U != value_size) + { + VerifyOrReturnError(value, CHIP_ERROR_INVALID_ARGUMENT); + } err = CC13X2_26X2Config::ReadKVS(key, value, value_size, read_bytes_size, offset_bytes); @@ -58,8 +61,6 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, size_t value_size) { VerifyOrReturnError(key, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(value, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(value_size > 0, CHIP_ERROR_INVALID_ARGUMENT); return CC13X2_26X2Config::WriteKVS(key, value, value_size); } diff --git a/src/platform/cc32xx/KeyValueStoreManagerImpl.cpp b/src/platform/cc32xx/KeyValueStoreManagerImpl.cpp index 13c1a023443594..ea0f2e546ce6c4 100644 --- a/src/platform/cc32xx/KeyValueStoreManagerImpl.cpp +++ b/src/platform/cc32xx/KeyValueStoreManagerImpl.cpp @@ -42,7 +42,10 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t size_t offset_bytes) const { VerifyOrReturnError(key, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(value, CHIP_ERROR_INVALID_ARGUMENT); + if (0U != value_size) + { + VerifyOrReturnError(value, CHIP_ERROR_INVALID_ARGUMENT); + } return CC32XXConfig::ReadKVS(key, value, value_size, read_bytes_size, offset_bytes); } @@ -50,8 +53,6 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, size_t value_size) { VerifyOrReturnError(key, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(value, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(value_size > 0, CHIP_ERROR_INVALID_ARGUMENT); return CC32XXConfig::WriteKVS(key, value, value_size); }