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