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

[TLVWriter] TLVWriter has direct access to write buffer in ways that can become unsafe in the future #31769

Closed
tcarmelveilleux opened this issue Jan 30, 2024 · 0 comments · Fixed by #31770
Closed

[TLVWriter] TLVWriter has direct access to write buffer in ways that can become unsafe in the future #31769

tcarmelveilleux opened this issue Jan 30, 2024 · 0 comments · Fixed by #31770
Labels
security

Comments

@tcarmelveilleux
Copy link
Contributor

TLVWriter directly touches the write buffer outside of WriteData, which is the only part that actually can properly manage the stream.

We need to remove all direct access to mWritePoint in TLVWriter to prevent unsafe usage of buffers in future updated logic.

This should not impact the public API.
tcarmelveilleux pushed a commit to tcarmelveilleux/connectedhomeip that referenced this issue Jan 30, 2024
@tennessee-google
Remove several direct uses of TLVWriter::mWritePoint
fb0909a 
- Remove all writing to mWritePoint that occur outside of WriteData
- Remove unsafe usage of CONFIG_HAVE_VSNPRINTF_EX
  - Does not impact public API as it was one underlying
    possible optimization, and the remaining code would work.
    Also, not used in SDK at all.
- Removed secondary paths that were potentially unsafe in TLVWriter::WriteElementHead

Fixes project-chip#31769

Testing done:
- All existing unit tests still pass.
- Only removed and non-functionally-refactored code.
@tcarmelveilleux tcarmelveilleux mentioned this issue Jan 30, 2024
Merged
@mergify mergify bot closed this as completed in #31770 Jan 31, 2024
mergify bot pushed a commit that referenced this issue Jan 31, 2024
@tcarmelveilleux @tennessee-google
@restyled-commits @kpschoedel
Remove several direct uses of TLVWriter::mWritePoint (#31770)
bdbd6de 
* Remove several direct uses of TLVWriter::mWritePoint

- Remove all writing to mWritePoint that occur outside of WriteData
- Remove unsafe usage of CONFIG_HAVE_VSNPRINTF_EX
  - Does not impact public API as it was one underlying
    possible optimization, and the remaining code would work.
    Also, not used in SDK at all.
- Removed secondary paths that were potentially unsafe in TLVWriter::WriteElementHead

Fixes #31769

Testing done:
- All existing unit tests still pass.
- Only removed and non-functionally-refactored code.

* Restyled by clang-format

* Update src/lib/core/TLVWriter.cpp

Co-authored-by: Kevin Schoedel <[email protected]>

---------

Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Kevin Schoedel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
security
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove several direct uses of TLVWriter::mWritePoint tcarmelveilleux/connectedhomeip
1 participant
@tcarmelveilleux