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.

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

[TLVWriter] Rationalize TLVWriter copy/init #30825

Open
tcarmelveilleux opened this issue Dec 5, 2023 · 0 comments
Open

[TLVWriter] Rationalize TLVWriter copy/init #30825

tcarmelveilleux opened this issue Dec 5, 2023 · 0 comments

Comments

@tcarmelveilleux
Copy link
Contributor

tcarmelveilleux commented Dec 5, 2023

TLVWriter has multiple complex initialization paths:

  • OpenContainer
  • Init(xxxx) --> several methods
  • Default-constructed.

There are many implementation details of buffering that are directly exposed by TLVWriter, which makes fixes/refactors much more difficult, since there is no common logic to make sure all changes propagate all fields properly.

Some prior dynamic analyses have found uninitialized accesses, which were fixed, so we would benefit from clearing-up the different cases of init of TLVWrite.

tcarmelveilleux pushed a commit to tcarmelveilleux/connectedhomeip that referenced this issue Dec 7, 2023
- It was found through static analysis that in some cases, TLVWriters
  did not have all fields initialized and this led to no errors, but
  possibly could lead to bad states.
- None of the code making use of uninitialized TLVWriters was found
  in the SDK, but there may be usages.
- In adding the checks, marked several areas that could be improved
  in TLVWriter data access/management.

Issue project-chip#30825

Testing done:
- All tests still pass.
- There is never uninitialized used of a TLVWriter.
tcarmelveilleux added a commit that referenced this issue Dec 11, 2023
* Ensure fields are always initialized in TLVWriters

- It was found through static analysis that in some cases, TLVWriters
  did not have all fields initialized and this led to no errors, but
  possibly could lead to bad states.
- None of the code making use of uninitialized TLVWriters was found
  in the SDK, but there may be usages.
- In adding the checks, marked several areas that could be improved
  in TLVWriter data access/management.

Issue #30825

Testing done:
- All tests still pass.
- There is never uninitialized used of a TLVWriter.

---------

Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant