From 1361228c027be235dfb8d80c805079cb6d89ddf6 Mon Sep 17 00:00:00 2001 From: tehampson Date: Fri, 1 Jul 2022 02:16:56 -0400 Subject: [PATCH] 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 * 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 * Address when buffer is nulptr and size is 0 * Fix comment from PR * Restyle * add include Co-authored-by: Tennessee Carmel-Veilleux Co-authored-by: Boris Zbarsky --- .../chip-tool/config/PersistentStorage.cpp | 16 +- .../ChipDeviceController-StorageDelegate.cpp | 21 +- .../python/chip/storage/__init__.py | 19 +- .../CHIPPersistentStorageDelegateBridge.mm | 6 +- src/lib/core/CHIPPersistentStorageDelegate.h | 43 ++- src/lib/support/PersistentStorageAudit.cpp | 291 +++++++++++++++++- .../support/TestPersistentStorageDelegate.h | 20 +- .../TestTestPersistentStorageDelegate.cpp | 174 ++++++++--- 8 files changed, 492 insertions(+), 98 deletions(-) diff --git a/examples/chip-tool/config/PersistentStorage.cpp b/examples/chip-tool/config/PersistentStorage.cpp index c2bf34349bccab..8ec4eb35c256f0 100644 --- 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(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 index 2f66fee2b476f7..7e25d71a440cd1 100644 --- a/src/controller/python/ChipDeviceController-StorageDelegate.cpp +++ b/src/controller/python/ChipDeviceController-StorageDelegate.cpp @@ -24,6 +24,7 @@ #include #include +#include #include namespace chip { @@ -31,28 +32,21 @@ 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 index 10cbcb5bec3216..4b3fc28ff18c37 100644 --- 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 index 5981127efd37f5..5fc7f200a1f3f4 100644 --- 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([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 index 3f556d7f798950..dd3eb33aa82e9e 100644 --- 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 diff --git a/src/lib/support/PersistentStorageAudit.cpp b/src/lib/support/PersistentStorageAudit.cpp index eb8b1e08f3e601..5846694db1f3f2 100644 --- a/src/lib/support/PersistentStorageAudit.cpp +++ b/src/lib/support/PersistentStorageAudit.cpp @@ -15,18 +15,307 @@ * limitations under the License. */ +#include + #include #include #include #include "PersistentStorageAudit.h" +#ifdef NL_TEST_ASSERT +#undef NL_TEST_ASSERT +#endif + +#define NL_TEST_ASSERT(inSuite, inCondition) \ + do \ + { \ + (inSuite)->performedAssertions += 1; \ + \ + if (!(inCondition)) \ + { \ + ChipLogError(Automation, "%s:%u: assertion failed: \"%s\"\n", __FILE__, __LINE__, #inCondition); \ + (inSuite)->failedAssertions += 1; \ + (inSuite)->flagError = true; \ + } \ + } while (0) + namespace chip { namespace audit { +// The following test is a copy of `src/lib/support/tests/TestTestPersistentStorageDelegate.cpp` 's +// `TestBasicApi()` test. It has to be copied since we currently are not setup to +// run on-device unit tests at large on all embedded platforms part of the SDK. bool ExecutePersistentStorageApiAudit(PersistentStorageDelegate & storage) { - (void) storage; + struct fakeTestSuite + { + int performedAssertions = 0; + int failedAssertions = 0; + bool flagError = false; + } theSuite; + auto * inSuite = &theSuite; + + const char * kLongKeyString = "aKeyThatIsExactlyMaxKeyLengthhhh"; + // Start fresh. + (void) storage.SyncDeleteKeyValue("roboto"); + (void) storage.SyncDeleteKeyValue("key2"); + (void) storage.SyncDeleteKeyValue("key3"); + (void) storage.SyncDeleteKeyValue("key4"); + (void) storage.SyncDeleteKeyValue("keyDOES_NOT_EXIST"); + (void) storage.SyncDeleteKeyValue(kLongKeyString); + + // ========== Start of actual audit from TestTestPersistentStorageDelegate.cpp ========= + + uint8_t buf[16]; + const uint16_t actualSizeOfBuf = static_cast(sizeof(buf)); + uint16_t size = actualSizeOfBuf; + + // Key not there + CHIP_ERROR err; + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("roboto", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + NL_TEST_ASSERT(inSuite, size == actualSizeOfBuf); + + err = storage.SyncDeleteKeyValue("roboto"); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + + // Add basic key, read it back, erase it + const char * kStringValue1 = "abcd"; + err = storage.SyncSetKeyValue("roboto", kStringValue1, static_cast(strlen(kStringValue1))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("roboto", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == strlen(kStringValue1)); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue1, strlen(kStringValue1))); + + err = storage.SyncDeleteKeyValue("roboto"); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("roboto", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + NL_TEST_ASSERT(inSuite, size == actualSizeOfBuf); + + // Validate adding 2 different keys + const char * kStringValue2 = "0123abcd"; + const char * kStringValue3 = "cdef89"; + err = storage.SyncSetKeyValue("key2", kStringValue2, static_cast(strlen(kStringValue2))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = storage.SyncSetKeyValue("key3", kStringValue3, static_cast(strlen(kStringValue3))); + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist("key3")); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // Read them back + + uint8_t all_zeroes[sizeof(buf)]; + memset(&all_zeroes[0], 0, sizeof(all_zeroes)); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("key2", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue2, strlen(kStringValue2))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("key3", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == strlen(kStringValue3)); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue3, strlen(kStringValue3))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + + // Read providing too small a buffer. Data read up to `size` and nothing more. + memset(&buf[0], 0, sizeof(buf)); + size = static_cast(strlen(kStringValue2) - 1); + uint16_t sizeBeforeGetKeyValueCall = size; + err = storage.SyncGetKeyValue("key2", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_BUFFER_TOO_SMALL); + NL_TEST_ASSERT(inSuite, size == sizeBeforeGetKeyValueCall); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue2, size)); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + + // Read in too small a buffer, which is nullptr and size == 0: check CHIP_ERROR_BUFFER_TOO_SMALL is given. + memset(&buf[0], 0, sizeof(buf)); + size = 0; + sizeBeforeGetKeyValueCall = size; + err = storage.SyncGetKeyValue("key2", nullptr, size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_BUFFER_TOO_SMALL); + NL_TEST_ASSERT(inSuite, size != strlen(kStringValue2)); + NL_TEST_ASSERT(inSuite, size == sizeBeforeGetKeyValueCall); + // Just making sure that implementation doesn't hold onto reference of previous destination buffer when + // nullptr is provided. + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); + + // Read in too small a buffer, which is nullptr and size != 0: error + size = static_cast(strlen(kStringValue2) - 1); + sizeBeforeGetKeyValueCall = size; + err = storage.SyncGetKeyValue("key2", nullptr, size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + NL_TEST_ASSERT(inSuite, size == sizeBeforeGetKeyValueCall); + // Just making sure that implementation doesn't hold onto reference of previous destination buffer when + // nullptr is provided. + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); + + // When key not found, size is not touched. + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("keyDOES_NOT_EXIST", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + NL_TEST_ASSERT(inSuite, actualSizeOfBuf == size); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); + + size = 0; + err = storage.SyncGetKeyValue("keyDOES_NOT_EXIST", nullptr, size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + NL_TEST_ASSERT(inSuite, 0 == size); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); + + // Even when key not found, cannot pass nullptr with size != 0. + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("keyDOES_NOT_EXIST", nullptr, size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + NL_TEST_ASSERT(inSuite, actualSizeOfBuf == size); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], size)); + + // Attempt an empty key write with either nullptr or zero size works + err = storage.SyncSetKeyValue("key2", kStringValue2, 0); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist("key2")); + + size = 0; + err = storage.SyncGetKeyValue("key2", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == 0); + + size = 0; + err = storage.SyncGetKeyValue("key2", nullptr, size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == 0); + + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("key2", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == 0); + + err = storage.SyncSetKeyValue("key2", nullptr, 0); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist("key2")); + + size = 0; + err = storage.SyncGetKeyValue("key2", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == 0); + + // Failure to set key if buffer is nullptr and size != 0 + size = 10; + err = storage.SyncSetKeyValue("key4", nullptr, size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist("key4")); + + // Can delete empty key + err = storage.SyncDeleteKeyValue("key2"); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist("key2")); + + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue("key2", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + NL_TEST_ASSERT(inSuite, size == actualSizeOfBuf); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], size)); + + // Using key and value with base64 symbols + const char * kBase64SymbolsKey = "key+/="; + const char * kBase64SymbolValues = "value+/="; + err = storage.SyncSetKeyValue(kBase64SymbolsKey, kBase64SymbolValues, static_cast(strlen(kBase64SymbolValues))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue(kBase64SymbolsKey, &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == strlen(kBase64SymbolValues)); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kBase64SymbolValues, strlen(kBase64SymbolValues))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + + err = storage.SyncDeleteKeyValue(kBase64SymbolsKey); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist(kBase64SymbolsKey)); + + // Try using key that is a size that equals PersistentStorageDelegate::kKeyLengthMax + char longKeyString[PersistentStorageDelegate::kKeyLengthMax + 1]; + memset(&longKeyString, 'X', PersistentStorageDelegate::kKeyLengthMax); + longKeyString[sizeof(longKeyString) - 1] = '\0'; + // strlen() is not compile time so we just have this runtime assert that should aways pass as a sanity check. + NL_TEST_ASSERT(inSuite, strlen(longKeyString) == PersistentStorageDelegate::kKeyLengthMax); + + err = storage.SyncSetKeyValue(longKeyString, kStringValue2, static_cast(strlen(kStringValue2))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue(longKeyString, &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue2, strlen(kStringValue2))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist(longKeyString)); + + err = storage.SyncDeleteKeyValue(longKeyString); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist(longKeyString)); + + constexpr size_t kMaxCHIPCertLength = 400; // From credentials/CHIPCert.h and spec + uint8_t largeBuffer[kMaxCHIPCertLength]; + memset(&largeBuffer, 'X', sizeof(largeBuffer)); + uint8_t largeBufferForCheck[sizeof(largeBuffer)]; + memcpy(largeBufferForCheck, largeBuffer, sizeof(largeBuffer)); + + err = storage.SyncSetKeyValue(longKeyString, largeBuffer, static_cast(sizeof(largeBuffer))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&largeBuffer, 0, sizeof(largeBuffer)); + size = static_cast(sizeof(largeBuffer)); + err = storage.SyncGetKeyValue(longKeyString, &largeBuffer[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == static_cast(sizeof(largeBuffer))); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&largeBuffer, largeBufferForCheck, sizeof(largeBuffer))); + + err = storage.SyncDeleteKeyValue(longKeyString); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // Cleaning up + (void) storage.SyncDeleteKeyValue("roboto"); + (void) storage.SyncDeleteKeyValue("key2"); + (void) storage.SyncDeleteKeyValue("key3"); + (void) storage.SyncDeleteKeyValue("key4"); + (void) storage.SyncDeleteKeyValue(kBase64SymbolsKey); + (void) storage.SyncDeleteKeyValue(kLongKeyString); + + // ========== End of code from TestTestPersistentStorageDelegate.cpp ========= + if (inSuite->flagError) + { + ChipLogError(Automation, + "==== PersistentStorageDelegate API audit: FAILED: %d/%d failed assertions ====", inSuite->failedAssertions, + inSuite->performedAssertions); + return false; + } + ChipLogError(Automation, "==== PersistentStorageDelegate API audit: SUCCESS ===="); return true; } diff --git a/src/lib/support/TestPersistentStorageDelegate.h b/src/lib/support/TestPersistentStorageDelegate.h index c04b339c36e8c9..8fb05ca939e976 100644 --- a/src/lib/support/TestPersistentStorageDelegate.h +++ b/src/lib/support/TestPersistentStorageDelegate.h @@ -197,10 +197,7 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate protected: virtual CHIP_ERROR SyncGetKeyValueInternal(const char * key, void * buffer, uint16_t & size) { - if ((buffer == nullptr) && (size != 0)) - { - return CHIP_ERROR_INVALID_ARGUMENT; - } + ReturnErrorCodeIf(((buffer == nullptr) && (size != 0)), CHIP_ERROR_INVALID_ARGUMENT); // Making sure poison keys are not accessed if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end()) @@ -213,15 +210,20 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate std::vector & value = mStorage[key]; size_t valueSize = value.size(); - if (size < valueSize) + if (!CanCastTo(valueSize)) { - size = CanCastTo(valueSize) ? static_cast(valueSize) : 0; - return CHIP_ERROR_BUFFER_TOO_SMALL; + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; } - size = static_cast(valueSize); + uint16_t valueSizeUint16 = static_cast(valueSize); + ReturnErrorCodeIf(size == 0 && valueSizeUint16 == 0, CHIP_NO_ERROR); + ReturnErrorCodeIf(buffer == nullptr, CHIP_ERROR_BUFFER_TOO_SMALL); + + uint16_t sizeToCopy = std::min(size, valueSizeUint16); + + size = sizeToCopy; memcpy(buffer, value.data(), size); - return CHIP_NO_ERROR; + return size < valueSizeUint16 ? CHIP_ERROR_BUFFER_TOO_SMALL : CHIP_NO_ERROR; } virtual CHIP_ERROR SyncSetKeyValueInternal(const char * key, const void * value, uint16_t size) diff --git a/src/lib/support/tests/TestTestPersistentStorageDelegate.cpp b/src/lib/support/tests/TestTestPersistentStorageDelegate.cpp index 10cda95e078cc4..d585a294fb146e 100644 --- a/src/lib/support/tests/TestTestPersistentStorageDelegate.cpp +++ b/src/lib/support/tests/TestTestPersistentStorageDelegate.cpp @@ -55,15 +55,16 @@ void TestBasicApi(nlTestSuite * inSuite, void * inContext) TestPersistentStorageDelegate storage; uint8_t buf[16]; - uint16_t size = sizeof(buf); + const uint16_t actualSizeOfBuf = static_cast(sizeof(buf)); + uint16_t size = actualSizeOfBuf; // Key not there CHIP_ERROR err; memset(&buf[0], 0, sizeof(buf)); - size = sizeof(buf); + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("roboto", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); - NL_TEST_ASSERT(inSuite, size == sizeof(buf)); + NL_TEST_ASSERT(inSuite, size == actualSizeOfBuf); NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == 0); @@ -76,7 +77,7 @@ void TestBasicApi(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); memset(&buf[0], 0, sizeof(buf)); - size = sizeof(buf); + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("roboto", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, size == strlen(kStringValue1)); @@ -86,10 +87,10 @@ void TestBasicApi(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); memset(&buf[0], 0, sizeof(buf)); - size = sizeof(buf); + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("roboto", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); - NL_TEST_ASSERT(inSuite, size == sizeof(buf)); + NL_TEST_ASSERT(inSuite, size == actualSizeOfBuf); // Validate adding 2 different keys const char * kStringValue2 = "0123abcd"; @@ -98,6 +99,7 @@ void TestBasicApi(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); err = storage.SyncSetKeyValue("key3", kStringValue3, static_cast(strlen(kStringValue3))); + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist("key3")); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == 2); @@ -107,66 +109,65 @@ void TestBasicApi(nlTestSuite * inSuite, void * inContext) // Read them back + uint8_t all_zeroes[sizeof(buf)]; + memset(&all_zeroes[0], 0, sizeof(all_zeroes)); + memset(&buf[0], 0, sizeof(buf)); - size = sizeof(buf); + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("key2", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue2, strlen(kStringValue2))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); memset(&buf[0], 0, sizeof(buf)); - size = sizeof(buf); + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("key3", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, size == strlen(kStringValue3)); NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue3, strlen(kStringValue3))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + // Read providing too small a buffer. Data read up to `size` and nothing more. memset(&buf[0], 0, sizeof(buf)); - size = sizeof(buf); - err = storage.SyncGetKeyValue("key2", &buf[0], size); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); - NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); - NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue2, strlen(kStringValue2))); - - // Pre-clear buffer to make sure next operations don't change contents - uint8_t all_zeroes[sizeof(buf)]; - memset(&buf[0], 0, sizeof(buf)); - memset(&all_zeroes[0], 0, sizeof(all_zeroes)); - - // Read in too small a buffer: no data read, but correct size given - memset(&buf[0], 0, sizeof(buf)); - size = static_cast(strlen(kStringValue2) - 1); - err = storage.SyncGetKeyValue("key2", &buf[0], size); + size = static_cast(strlen(kStringValue2) - 1); + uint16_t sizeBeforeGetKeyValueCall = size; + err = storage.SyncGetKeyValue("key2", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_BUFFER_TOO_SMALL); - NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); - NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); + NL_TEST_ASSERT(inSuite, size == sizeBeforeGetKeyValueCall); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue2, size)); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); - // Read in too small a buffer, which is nullptr and size == 0: check correct size given - size = 0; - err = storage.SyncGetKeyValue("key2", nullptr, size); + // Read in too small a buffer, which is nullptr and size == 0: check CHIP_ERROR_BUFFER_TOO_SMALL is given. + memset(&buf[0], 0, sizeof(buf)); + size = 0; + sizeBeforeGetKeyValueCall = size; + err = storage.SyncGetKeyValue("key2", nullptr, size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_BUFFER_TOO_SMALL); - NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); + NL_TEST_ASSERT(inSuite, size != strlen(kStringValue2)); + NL_TEST_ASSERT(inSuite, size == sizeBeforeGetKeyValueCall); + // Just making sure that implementation doesn't hold onto reference of previous destination buffer when + // nullptr is provided. NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); // Read in too small a buffer, which is nullptr and size != 0: error - size = static_cast(strlen(kStringValue2) - 1); - err = storage.SyncGetKeyValue("key2", nullptr, size); + size = static_cast(strlen(kStringValue2) - 1); + sizeBeforeGetKeyValueCall = size; + err = storage.SyncGetKeyValue("key2", nullptr, size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); - NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); - - // Read in zero size buffer, which is also nullptr (i.e. just try to find if key exists without - // using a buffer). - size = 0; - err = storage.SyncGetKeyValue("key2", nullptr, size); - NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_BUFFER_TOO_SMALL); - NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); + NL_TEST_ASSERT(inSuite, size == sizeBeforeGetKeyValueCall); + // Just making sure that implementation doesn't hold onto reference of previous destination buffer when + // nullptr is provided. NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); // When key not found, size is not touched. - size = sizeof(buf); + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("keyDOES_NOT_EXIST", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); - NL_TEST_ASSERT(inSuite, sizeof(buf) == size); + NL_TEST_ASSERT(inSuite, actualSizeOfBuf == size); NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); size = 0; @@ -176,23 +177,35 @@ void TestBasicApi(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], sizeof(buf))); // Even when key not found, cannot pass nullptr with size != 0. - size = static_cast(sizeof(buf)); + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("keyDOES_NOT_EXIST", nullptr, size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); - NL_TEST_ASSERT(inSuite, sizeof(buf) == size); + NL_TEST_ASSERT(inSuite, actualSizeOfBuf == size); NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], size)); // Attempt an empty key write with either nullptr or zero size works err = storage.SyncSetKeyValue("key2", kStringValue2, 0); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist("key2")); + + size = 0; + err = storage.SyncGetKeyValue("key2", &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == 0); size = 0; + err = storage.SyncGetKeyValue("key2", nullptr, size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == 0); + + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("key2", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, size == 0); err = storage.SyncSetKeyValue("key2", nullptr, 0); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist("key2")); size = 0; err = storage.SyncGetKeyValue("key2", &buf[0], size); @@ -203,15 +216,86 @@ void TestBasicApi(nlTestSuite * inSuite, void * inContext) size = 10; err = storage.SyncSetKeyValue("key4", nullptr, size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist("key4")); // Can delete empty key err = storage.SyncDeleteKeyValue("key2"); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); - size = static_cast(sizeof(buf)); + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist("key2")); + + size = actualSizeOfBuf; err = storage.SyncGetKeyValue("key2", &buf[0], size); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); - NL_TEST_ASSERT(inSuite, size == sizeof(buf)); + NL_TEST_ASSERT(inSuite, size == actualSizeOfBuf); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], &all_zeroes[0], size)); + + // Using key and value with base64 symbols + const char * kBase64SymbolsKey = "key+/="; + const char * kBase64SymbolValues = "value+/="; + err = storage.SyncSetKeyValue(kBase64SymbolsKey, kBase64SymbolValues, static_cast(strlen(kBase64SymbolValues))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue(kBase64SymbolsKey, &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == strlen(kBase64SymbolValues)); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kBase64SymbolValues, strlen(kBase64SymbolValues))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + + err = storage.SyncDeleteKeyValue(kBase64SymbolsKey); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist(kBase64SymbolsKey)); + + // Try using key that is a size that equals PersistentStorageDelegate::kKeyLengthMax + char longKeyString[PersistentStorageDelegate::kKeyLengthMax + 1]; + memset(&longKeyString, 'X', PersistentStorageDelegate::kKeyLengthMax); + longKeyString[sizeof(longKeyString) - 1] = '\0'; + // strlen() is not compile time so we just have this runtime assert that should aways pass as a sanity check. + NL_TEST_ASSERT(inSuite, strlen(longKeyString) == PersistentStorageDelegate::kKeyLengthMax); + + err = storage.SyncSetKeyValue(longKeyString, kStringValue2, static_cast(strlen(kStringValue2))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&buf[0], 0, sizeof(buf)); + size = actualSizeOfBuf; + err = storage.SyncGetKeyValue(longKeyString, &buf[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == strlen(kStringValue2)); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[0], kStringValue2, strlen(kStringValue2))); + // Make sure that there was no buffer overflow during SyncGetKeyValue + NL_TEST_ASSERT(inSuite, 0 == memcmp(&buf[size], &all_zeroes[0], sizeof(buf) - size)); + + NL_TEST_ASSERT(inSuite, storage.SyncDoesKeyExist(longKeyString)); + + err = storage.SyncDeleteKeyValue(longKeyString); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, !storage.SyncDoesKeyExist(longKeyString)); + + constexpr size_t kMaxCHIPCertLength = 400; // From credentials/CHIPCert.h and spec + uint8_t largeBuffer[kMaxCHIPCertLength]; + memset(&largeBuffer, 'X', sizeof(largeBuffer)); + uint8_t largeBufferForCheck[sizeof(largeBuffer)]; + memcpy(largeBufferForCheck, largeBuffer, sizeof(largeBuffer)); + + err = storage.SyncSetKeyValue(longKeyString, largeBuffer, static_cast(sizeof(largeBuffer))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + memset(&largeBuffer, 0, sizeof(largeBuffer)); + size = static_cast(sizeof(largeBuffer)); + err = storage.SyncGetKeyValue(longKeyString, &largeBuffer[0], size); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, size == static_cast(sizeof(largeBuffer))); + NL_TEST_ASSERT(inSuite, 0 == memcmp(&largeBuffer, largeBufferForCheck, sizeof(largeBuffer))); + + err = storage.SyncDeleteKeyValue(longKeyString); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // Cleaning up + err = storage.SyncDeleteKeyValue("key3"); + NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == 0); } // ClearStorage is not a PersistentStorageDelegate base class method, it only