From b78a6e6c09ae9a5930fd4a5e32ea57bcdb0b3726 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sat, 2 Jul 2022 00:46:50 -0400 Subject: [PATCH] Fix chip-tool PersistentStorageDelegate failing on some keys (#20239) * Fix chip-tool PersistentStorageDelegate failing on some keys - chip-tool's PersistentStorageDelegate failed the audit: ``` [1656721506.201383][688277:688277] CHIP:ATM: ../../../examples/chip-tool/third_party/connectedhomeip/src/lib/support/PersistentStorageAudit.cpp:248: assertion failed: "err == CHIP_NO_ERROR" [1656721506.201409][688277:688277] CHIP:ATM: ../../../examples/chip-tool/third_party/connectedhomeip/src/lib/support/PersistentStorageAudit.cpp:249: assertion failed: "size == strlen(kBase64SymbolValues)" [1656721506.201413][688277:688277] CHIP:ATM: ../../../examples/chip-tool/third_party/connectedhomeip/src/lib/support/PersistentStorageAudit.cpp:250: assertion failed: "0 == memcmp(&buf[0], kBase64SymbolValues, strlen(kBase64SymbolValues))" [1656721506.201437][688277:688277] CHIP:ATM: ../../../examples/chip-tool/third_party/connectedhomeip/src/lib/support/PersistentStorageAudit.cpp:255: assertion failed: "err == CHIP_NO_ERROR" ``` - The audit also crashed on SyncSetKeyValue with nullptr argument - To fail the audit, had to run the audit on a version that forcibly loaded what was stored again. - Root cause was base64 padding has `=` which confuses INI parser into giving out keys that are too small Issue #20188 This PR: - Implements `SyncDoesKeyExist` natively - Fixes handling of `nullptr` arguments and zero-size keys in the storage impl - Uses `\x00` C-style hex escaping for any characters that could fool the INI parser, which retains human readability of keys in the INI files, but fixes the bugs found by the audit - Removes unnecessary newliens in storage audit logging Testing done: - Unit tests still pass - Cert tests still pass - Storage audit passed ``` [1656722163.492413][696066:696066] CHIP:ATM: ==== PersistentStorageDelegate API audit: SUCCESS ==== ``` * Restyled by clang-format * Remove a stale temporary added during testing Co-authored-by: Restyled.io --- .../chip-tool/config/PersistentStorage.cpp | 58 ++++++++++++++++--- examples/chip-tool/config/PersistentStorage.h | 1 + src/lib/support/PersistentStorageAudit.cpp | 2 +- 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/examples/chip-tool/config/PersistentStorage.cpp b/examples/chip-tool/config/PersistentStorage.cpp index 8ec4eb35c256f0..cf2324b1c3e751 100644 --- a/examples/chip-tool/config/PersistentStorage.cpp +++ b/examples/chip-tool/config/PersistentStorage.cpp @@ -50,6 +50,29 @@ std::string GetFilename(const char * name) namespace { +std::string EscapeKey(const std::string & key) +{ + std::string escapedKey; + escapedKey.reserve(key.size()); + + for (char c : key) + { + // Replace spaces, non-printable chars and `=` with hex-escaped (C-style) characters. + if ((c <= 0x20) || (c == '=') || (c >= 0x7F)) + { + char escaped[5] = { 0 }; + snprintf(escaped, sizeof(escaped), "\\x%02x", (static_cast(c) & 0xff)); + escapedKey += escaped; + } + else + { + escapedKey += c; + } + } + + return escapedKey; +} + std::string StringToBase64(const std::string & value) { std::unique_ptr buffer(new char[BASE64_ENCODED_LEN(value.length())]); @@ -106,10 +129,11 @@ CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, ui 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); - ReturnErrorCodeIf(!inipp::extract(section[key], iniValue), CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(!SyncDoesKeyExist(key), CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + + std::string escapedKey = EscapeKey(key); + ReturnErrorCodeIf(!inipp::extract(section[escapedKey], iniValue), CHIP_ERROR_INVALID_ARGUMENT); iniValue = Base64ToString(iniValue); @@ -126,8 +150,19 @@ CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, ui CHIP_ERROR PersistentStorage::SyncSetKeyValue(const char * key, const void * value, uint16_t size) { + ReturnErrorCodeIf((value == nullptr) && (size != 0), CHIP_ERROR_INVALID_ARGUMENT); + auto section = mConfig.sections[kDefaultSectionName]; - section[key] = StringToBase64(std::string(static_cast(value), size)); + + std::string escapedKey = EscapeKey(key); + if (value == nullptr) + { + section[escapedKey] = ""; + } + else + { + section[escapedKey] = StringToBase64(std::string(static_cast(value), size)); + } mConfig.sections[kDefaultSectionName] = section; return CommitConfig(mName); @@ -136,15 +171,24 @@ CHIP_ERROR PersistentStorage::SyncSetKeyValue(const char * key, const void * val CHIP_ERROR PersistentStorage::SyncDeleteKeyValue(const char * key) { auto section = mConfig.sections[kDefaultSectionName]; - auto it = section.find(key); - ReturnErrorCodeIf(it == section.end(), CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); - section.erase(key); + ReturnErrorCodeIf(!SyncDoesKeyExist(key), CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + + std::string escapedKey = EscapeKey(key); + section.erase(escapedKey); mConfig.sections[kDefaultSectionName] = section; return CommitConfig(mName); } +bool PersistentStorage::SyncDoesKeyExist(const char * key) +{ + std::string escapedKey = EscapeKey(key); + auto section = mConfig.sections[kDefaultSectionName]; + auto it = section.find(escapedKey); + return (it != section.end()); +} + CHIP_ERROR PersistentStorage::SyncClearAll() { ChipLogProgress(chipTool, "Clearing %s storage", kDefaultSectionName); diff --git a/examples/chip-tool/config/PersistentStorage.h b/examples/chip-tool/config/PersistentStorage.h index d26582ea9b9dfa..106dcd64949a87 100644 --- a/examples/chip-tool/config/PersistentStorage.h +++ b/examples/chip-tool/config/PersistentStorage.h @@ -32,6 +32,7 @@ class PersistentStorage : public chip::PersistentStorageDelegate CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override; CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override; CHIP_ERROR SyncDeleteKeyValue(const char * key) override; + bool SyncDoesKeyExist(const char * key) override; uint16_t GetListenPort(); chip::Logging::LogCategory GetLoggingLevel(); diff --git a/src/lib/support/PersistentStorageAudit.cpp b/src/lib/support/PersistentStorageAudit.cpp index 5846694db1f3f2..cad5d8dee1e923 100644 --- a/src/lib/support/PersistentStorageAudit.cpp +++ b/src/lib/support/PersistentStorageAudit.cpp @@ -34,7 +34,7 @@ \ if (!(inCondition)) \ { \ - ChipLogError(Automation, "%s:%u: assertion failed: \"%s\"\n", __FILE__, __LINE__, #inCondition); \ + ChipLogError(Automation, "%s:%u: assertion failed: \"%s\"", __FILE__, __LINE__, #inCondition); \ (inSuite)->failedAssertions += 1; \ (inSuite)->flagError = true; \ } \