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

Unbounded stack in src/setup_payload #3663

Closed
kpschoedel opened this issue Nov 4, 2020 · 4 comments
Closed

Unbounded stack in src/setup_payload #3663

kpschoedel opened this issue Nov 4, 2020 · 4 comments
Assignees
@alexhqwang
Labels
memory p1 priority 1 work security
Milestone
V1.0

Comments

@kpschoedel
Copy link
Contributor

Problem

payloadBase41RepresentationWithTLV() allocates stack space based on an argument, with no upper bound. This could lead to stack exhaustion on small devices.

Proposed Solution

TBD. The caller's callers pass in a destination buffer, which might be usable as temporary space.

Code in this path also uses heap-allocated std::strings, which should ultimately be removed for issue #2748 (All Embedded Device-side Code Needs to Be Scrubbed of Direct Heap Usage).
@issue-label-bot
Copy link

Issue Label Bot is not confident enough to auto-label this issue. See dashboard for more details.

kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Nov 5, 2020
@kpschoedel
Enable -Wstack-usage on device builds
7acc910 
#### Problem

RAM on small devices is limited, so excesssive stack should not be used
without good reason. Stack allocation is also not easily checked at run
time so overflow is likely to cause crashes.

#### Summary of Changes

- Add a `-Wstack-usage` compiler flag on embedded builds. (For this
  purpose, an ‘embedded build’ is any that is not using a whitelisted
  non-embedded OS, so new platforms will have this enabled by default.)

  In this PR, the stack limit is set high enough that _only_ dynamically
  unbounded stack usage triggers it. The intent is to lower the limit
  in the future so that any unusually large stack requires whitelisting
  with justification in review.

- Replace most uses of dynamically unbounded stack.

- Filed separate issues for two remaining uses of dynamic stack:
    - project-chip#3662 emberAfPrintBuffer()
    - project-chip#3663 payloadBase41RepresentationWithTLV()

fixes project-chip#3505 Use "-Wstack-usage" for device builds
@kpschoedel kpschoedel mentioned this issue Nov 5, 2020
Merged
@kpschoedel kpschoedel changed the title Unbounded stack in payloadBase41RepresentationWithTLV() Unbounded stack in src/setup_payload Nov 6, 2020
@kpschoedel
Copy link
Contributor Author

decimalStringWithPadding() in src/setup_payload/ManualSetupPayloadGenerator.cpp added to this issue.

Per https://github.com/project-chip/connectedhomeip/pull/3683/files#r518644463 these uses are probably OK, and just need to be recorded as such.

bzbarsky-apple pushed a commit that referenced this issue Nov 7, 2020
@kpschoedel
Enable -Wstack-usage on device builds (#3683)
2d17edb 
* Enable -Wstack-usage on device builds

#### Problem

RAM on small devices is limited, so excesssive stack should not be used
without good reason. Stack allocation is also not easily checked at run
time so overflow is likely to cause crashes.

#### Summary of Changes

- Add a `-Wstack-usage` compiler flag on embedded builds. (For this
  purpose, an ‘embedded build’ is any that is not using a whitelisted
  non-embedded OS, so new platforms will have this enabled by default.)

  In this PR, the stack limit is set high enough that _only_ dynamically
  unbounded stack usage triggers it. The intent is to lower the limit
  in the future so that any unusually large stack requires whitelisting
  with justification in review.

- Replace most uses of dynamically unbounded stack.

- Filed separate issues for two remaining uses of dynamic stack:
    - #3662 emberAfPrintBuffer()
    - #3663 payloadBase41RepresentationWithTLV()

fixes #3505 Use "-Wstack-usage" for device builds

* Fix dynamic-sized stack array in CHIPDeviceController.cpp

* Additional dynamic-stack-size fixes.

* Revert ManualSetupPayloadGenerator.cpp
@woody-apple woody-apple added this to the V1.0 milestone Jan 29, 2021
@todo todo bot mentioned this issue Apr 19, 2021
Merged
@alexhqwang
Copy link
Contributor

alexhqwang commented Jul 16, 2021 •
edited
Loading

Not sure how to change assignee, but I am looking into this since embedded targets may not support std::string.

alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 20, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
0e88244
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 20, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
17c319d
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 20, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
e326586
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 20, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
23d43a9
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 21, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
f7ce1d8
@alexhqwang alexhqwang mentioned this issue Jul 21, 2021
Merged
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 21, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
6b2d959
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 21, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
44537af
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 21, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
aeaa0ed
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 22, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
d897720
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 26, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
ccbf36d
alexhqwang added a commit to alexhqwang/connectedhomeip that referenced this issue Jul 27, 2021
@alexhqwang
Fix unbounded stack in src/setup_payload (project-chip#3663)
95117f7
andy31415 pushed a commit that referenced this issue Jul 27, 2021
@alexhqwang @tcarmelveilleux
Change QR code payload setup to use C strings (#8526)
4ca8b8f 
* Change QR code payload setup to use C strings

Devices that need to display a QR code may not support std::string.
Split base 38 encode and decode functions into separate files, and
change the encoding function to use char buffers.

* Fix unbounded stack in src/setup_payload (#3663)

* Apply suggestions from code review

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
@alexhqwang
Copy link
Contributor

PR has merged, this can be closed. thanks!

@alexhqwang alexhqwang mentioned this issue Sep 1, 2021
Merged
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@alexhqwang @tcarmelveilleux
Change QR code payload setup to use C strings (project-chip#8526)
42ad5dc 
* Change QR code payload setup to use C strings

Devices that need to display a QR code may not support std::string.
Split base 38 encode and decode functions into separate files, and
change the encoding function to use char buffers.

* Fix unbounded stack in src/setup_payload (project-chip#3663)

* Apply suggestions from code review

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alexhqwang alexhqwang

Labels
memory p1 priority 1 work security
Projects
[Performance] Flash + RAM usage
Archived in project
Milestone
V1.0
Development

No branches or pull requests
3 participants
@woody-apple @alexhqwang @kpschoedel