Fix chip-tool PersistentStorageDelegate failing on some keys #20239
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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 project-chip#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 ==== ```
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda and
mrjerryjohns
pullapprove
bot
requested review from
mspang,
rgoliver,
saurabhst,
selissia,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
msandstedt
approved these changes
Jul 2, 2022
|
PR #20239: Size comparison from e491f85 to d75823a Increases (2 builds for cc13x2_26x2, linux)
Decreases (5 builds for cc13x2_26x2, cyw30739, telink)
Full report (28 builds for cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, nrfconnect, p6, telink)
|
woody-apple
approved these changes
Jul 2, 2022
bzbarsky-apple
approved these changes
Jul 2, 2022
|
PR #20239: Size comparison from e491f85 to 1518e5e Increases (4 builds for cc13x2_26x2, linux)
Decreases (5 builds for cc13x2_26x2, telink)
Full report (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
tcarmelveilleux
added a commit
to tcarmelveilleux/connectedhomeip
that referenced
this pull request
Jul 2, 2022
- PR project-chip#20239 got merged before some @bluebin14 had time to review and a bug exists where having the escape in a key can collide with an escaped key. This PR: - Adds the escape character to the escaping set Testing done: - Adding key "hello=" and "hello\x3d" leads to different stored keys ``` key\x3d=MQ== key\x5cx3d=MQ== ``` - All unit tests pass, integration tests pass
tcarmelveilleux
added a commit
to tcarmelveilleux/connectedhomeip
that referenced
this pull request
Jul 2, 2022
- PR project-chip#20239 got merged before some @bluebin14 had time to review and a bug exists where having the escape in a key can collide with an escaped key. This PR: - Adds the escape character to the escaping set Testing done: - Adding key `key=` and `key\x3d` leads to different stored keys ``` key\x3d=MQ== key\x5cx3d=MQ== ``` - All unit tests pass, integration tests pass
woody-apple
pushed a commit
that referenced
this pull request
Jul 2, 2022
- PR #20239 got merged before some @bluebin14 had time to review and a bug exists where having the escape in a key can collide with an escaped key. This PR: - Adds the escape character to the escaping set Testing done: - Adding key `key=` and `key\x3d` leads to different stored keys ``` key\x3d=MQ== key\x5cx3d=MQ== ``` - All unit tests pass, integration tests pass
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
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 intogiving out keys that are too small
Issue #20188
Change overview
This PR:
SyncDoesKeyExistnativelynullptrarguments and zero-size keys in the storage impl\x00C-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
Testing