Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure Linux examples platform storage backend succeeds Storage audit #20188

Closed
tcarmelveilleux opened this issue Jun 30, 2022 · 0 comments · Fixed by #20289
Closed

Ensure Linux examples platform storage backend succeeds Storage audit #20188

tcarmelveilleux opened this issue Jun 30, 2022 · 0 comments · Fixed by #20289
Assignees
@tehampson

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

PR #20134 and #20164 together introduced a storage API compliance audit.

This audit must be run and pass on Linux and succeed against PersistentStorageDelegate used by all-clusters-app (KVS file based)

To run the audit for anything using Server, build your target with chip_support_enable_storage_api_audit=true added to your GN args and then run your binary in a debugger. The default audit is added to Server::Init() and will cause an assert with logs on failure.

Audit passes once you see the following string in your logs

  • ==== PersistentStorageDelegate API audit: SUCCESS ====

On failure, informative assertion failures are logged to assist in debugging.

Proposed Solution

  • Build with audit enabled
  • Run and comment on passing along with the SHA of the commit where it was run. Provide a logs capture of your run.
  • If you find failures, please document them in comments on the issue to help SDK team audit the types of failures/assumptions made.
  • Close the issue once you pass
@tcarmelveilleux tcarmelveilleux changed the title Ensure Linux examplkes platform storage backend succeeds Storage audit Ensure Linux examples platform storage backend succeeds Storage audit Jul 1, 2022
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Jul 2, 2022
@tcarmelveilleux
Fix chip-tool PersistentStorageDelegate failing on some keys
e759c59 
- 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 ====
```
@tcarmelveilleux tcarmelveilleux mentioned this issue Jul 2, 2022
Merged
woody-apple pushed a commit that referenced this issue Jul 2, 2022
@tcarmelveilleux @restyled-commits
Fix chip-tool PersistentStorageDelegate failing on some keys (#20239)
b78a6e6 
* 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 <commits@restyled.io>
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Jul 4, 2022
@tcarmelveilleux
Fix escaping in Linux app storage
9016a09 
- Linux apps (like all-clusters-app) use a different storage backend
  than chip-tool, but the backend eventually goes to the same INI
  format.
- The Linux app did not properly manage keys with `=` and `\n` in the
  key, when attempting reload of value.

Fixes project-chip#20188

This PR:

- Adds the same escaping to Linux apps as for chip-tool
- Adds unescaping
- Adds unit tests for escaping and unescaping
- Adds debug dumping of all keys to chip-tool storage init

Testing done:

- Added unit tests pass
- Existing unit tests pass
- Cert tests pass
- Manual validation of restart with colliding keys in INI backend
  no longer shows collision after change
@tcarmelveilleux tcarmelveilleux mentioned this issue Jul 4, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Jul 5, 2022
@tcarmelveilleux
Fix escaping in Linux app storage (#20289)
7f8dc0d 
* Fix escaping in Linux app storage

- Linux apps (like all-clusters-app) use a different storage backend
  than chip-tool, but the backend eventually goes to the same INI
  format.
- The Linux app did not properly manage keys with `=` and `\n` in the
  key, when attempting reload of value.

Fixes #20188

This PR:

- Adds the same escaping to Linux apps as for chip-tool
- Adds unescaping
- Adds unit tests for escaping and unescaping
- Adds debug dumping of all keys to chip-tool storage init

Testing done:

- Added unit tests pass
- Existing unit tests pass
- Cert tests pass
- Manual validation of restart with colliding keys in INI backend
  no longer shows collision after change

* Fix lint

* Fix CI

* Address review comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tehampson tehampson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix escaping in Linux app storage tcarmelveilleux/connectedhomeip
2 participants
@tcarmelveilleux @tehampson