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

Remove raw SessionHolder::Get #18490

Merged
merged 2 commits into from
May 18, 2022

Conversation

mrjerryjohns
Copy link
Contributor

Problem

The SessionHolder::Get API returns the underlying SessionHandle under the assumption that one exists in the holder. It assumes callers have previously called operator bool() to check to see if there is a valid session in the holder BEFORE calling this API.

However, most consumers are lulled into thinking with that API that there is always a SessionHandle in that holder, which is not true since holders act as weak-references where the invalidation of a given session removes the referenced handles from all holders in the system.

This results in crashes if clients assume incorrectly about the session stored in the holder.

E.g If a subscription is setup with re-subscription enabled, the invalidation of the underlying session results in a crash later at re-subscription time.

Fix

Fix the API to return an Optional<SessionHandle> at all times to make it clear that the holder MAY NOT be holding on-to a session.

Consequently, I scrubbed and fixed all existing call-sites (including the buggy ReadClient one) to either test for the right invariance about session presence, or to return an error appropriately if the holder was not holding onto a session at the time.

Test

Using the REPL, I setup a subscription to a peer, then called CloseSession to emulate the forcible termination of a given session to a peer, then waited for the re-subscription to kick-in.

Validated that it crashed pre-fix, and correctly error'ed out post-fix.

@github-actions
Copy link

github-actions bot commented May 17, 2022

PR #18490: Size comparison from a6ed4d9 to 11f8918

Increases (9 builds for linux, mbed, nrfconnect, p6, telink)
platform target config section a6ed4d9 11f8918 change % change
linux chip-tool-no-interactive-ipv6only arm64 (read only) 9044388 9045052 664 0.0
.rodata 444564 444700 136 0.0
.text 7138116 7138644 528 0.0
thermostat-no-ble arm64 (read only) 2343156 2343652 496 0.0
.rodata 146980 147124 144 0.1
.text 1966352 1966704 352 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read/write) 2418600 2418728 128 0.0
.text 1381244 1381372 128 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1176903 1177047 144 0.0
text 807524 807672 148 0.0
p6 all-clusters-app default (read/write) 2529568 2532272 2704 0.1
.text 1487832 1490536 2704 0.2
light-app default (read/write) 2418504 2421032 2528 0.1
.text 1376768 1379296 2528 0.2
lock-app default (read/write) 2429152 2431744 2592 0.1
.text 1387416 1390008 2592 0.2
telink light-switch-app tlsr9518adk80d (read/write) 781904 781992 88 0.0
text 552922 553012 90 0.0
lighting-app tlsr9518adk80d (read/write) 801988 802084 96 0.0
text 569688 569778 90 0.0
Full report (9 builds for linux, mbed, nrfconnect, p6, telink)
platform target config section a6ed4d9 11f8918 change % change
linux chip-tool-no-interactive-ipv6only arm64 (read only) 9044388 9045052 664 0.0
(read/write) 644065 644065 0 0.0
.bss 41105 41105 0 0.0
.data 1192 1192 0 0.0
.data.rel.ro 582936 582936 0 0.0
.dynamic 560 560 0 0.0
.got 14984 14984 0 0.0
.init 24 24 0 0.0
.init_array 184 184 0 0.0
.rodata 444564 444700 136 0.0
.text 7138116 7138644 528 0.0
thermostat-no-ble arm64 (read only) 2343156 2343652 496 0.0
(read/write) 175009 175009 0 0.0
.bss 86353 86353 0 0.0
.data 1520 1520 0 0.0
.data.rel.ro 79336 79336 0 0.0
.dynamic 560 560 0 0.0
.got 4768 4768 0 0.0
.init 24 24 0 0.0
.init_array 376 376 0 0.0
.rodata 146980 147124 144 0.1
.text 1966352 1966704 352 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2418600 2418728 128 0.0
.bss 201596 201596 0 0.0
.data 5872 5872 0 0.0
.text 1381244 1381372 128 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1176903 1177047 144 0.0
bss 138368 138368 0 0.0
rodata 152256 152256 0 0.0
text 807524 807672 148 0.0
p6 all-clusters-app default (read/write) 2529568 2532272 2704 0.1
.bss 135104 135104 0 0.0
.data 2808 2808 0 0.0
.text 1487832 1490536 2704 0.2
light-app default (read/write) 2418504 2421032 2528 0.1
.bss 128432 128432 0 0.0
.data 2608 2608 0 0.0
.text 1376768 1379296 2528 0.2
lock-app default (read/write) 2429152 2431744 2592 0.1
.bss 128248 128248 0 0.0
.data 2568 2568 0 0.0
.text 1387416 1390008 2592 0.2
telink light-switch-app tlsr9518adk80d (read/write) 781904 781992 88 0.0
bss 70608 70608 0 0.0
noinit 40416 40416 0 0.0
text 552922 553012 90 0.0
lighting-app tlsr9518adk80d (read/write) 801988 802084 96 0.0
bss 70864 70864 0 0.0
noinit 40416 40416 0 0.0
text 569688 569778 90 0.0

src/controller/CommissioneeDeviceProxy.cpp Outdated Show resolved Hide resolved
src/lib/core/CHIPError.h Outdated Show resolved Hide resolved
src/messaging/ExchangeContext.cpp Outdated Show resolved Hide resolved
src/messaging/ExchangeContext.cpp Outdated Show resolved Hide resolved
src/transport/SessionHolder.h Show resolved Hide resolved
@msandstedt
Copy link
Contributor

Doesn't this just kick the problem around? Now we need to check existence in the Optional. SessionHolder is already acting as an optional. I don't really see how this helps.

@mrjerryjohns
Copy link
Contributor Author

Doesn't this just kick the problem around? Now we need to check existence in the Optional.

The point is that you should always be checking for existence with a holder, and this API is bolstering that mentality for forcing applications and protocol layers to think about that.

If our goal here is to eventually make these look like standard smart-ptr constructs, SessionHolder is going to eventually be modeled after std::weak_ptr. That very much has a lock() method that is pretty identical to what I'm doing here with Get().

As part of that, I want to eventually return a SessionSharedPtr back in Get() that is essentially Optional<SessionHandle>. This is setting up for that reality.

@andy31415 andy31415 merged commit 7dec705 into project-chip:master May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants