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

Fix crashes during CASE establishment. #25868

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

bzbarsky-apple
Copy link
Contributor

There were two potential sources of crashes:

  1. TryNextResult on the resolver scheduled an async task that could not be canceled. If, while that task was pending, the OperationalSessionSetup was destroyed (which could happen if another session for the same peer had become active in the meantime and another connection attempt happened) we would end up with use-after-free.

  2. ScheduleSessionSetupReattempt scheduled a timer that was never being canceled. Similar to above, if the OperationalSessionSetup got destroyed before the timer fired we could end up with use-after-free.

The fix for the first problem is to make TryNextResult synchronous, instead of scheduling an async task.

The fix for the second problem is to introduce a new state of OperationalSessionSetup that's used while waiting for the timer and to ensure that the timer is canceled when leaving that state or if the OperationalSessionSetup is destroyed.

To preserve existing behavior, if we are in the "waiting for timer" state and new connection attempt happens and there is a session we can attach to, then we immediately move to the Connected state (and cancel the timer).

There were two potential sources of crashes:

1) TryNextResult on the resolver scheduled an async task that could not be
canceled.  If, while that task was pending, the OperationalSessionSetup was
destroyed (which could happen if another session for the same peer had become
active in the meantime and another connection attempt happened) we would end up
with use-after-free.

2) ScheduleSessionSetupReattempt scheduled a timer that was never being
canceled.  Similar to above, if the OperationalSessionSetup got destroyed before
the timer fired we could end up with use-after-free.

The fix for the first problem is to make TryNextResult synchronous, instead of
scheduling an async task.

The fix for the second problem is to introduce a new state of
OperationalSessionSetup that's used while waiting for the timer and to ensure
that the timer is canceled when leaving that state or if the
OperationalSessionSetup is destroyed.

To preserve existing behavior, if we are in the "waiting for timer" state and
new connection attempt happens and there is a session we can attach to, then we
immediately move to the Connected state (and cancel the timer).
@github-actions
Copy link

PR #25868: Size comparison from 745da97 to 0bacd9f

Increases (1 build for cc32xx)
platform target config section 745da97 0bacd9f change % change
cc32xx lock CC3235SF_LAUNCHXL (read only) 645745 645769 24 0.0
.debug_info 20309906 20310044 138 0.0
.debug_line 2680953 2681022 69 0.0
.debug_str 3041114 3041144 30 0.0
.symtab 257456 257472 16 0.0
.text 537672 537696 24 0.0
Decreases (1 build for cc32xx)
platform target config section 745da97 0bacd9f change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_frame 301604 301596 -8 -0.0
.debug_loc 2827596 2827360 -236 -0.0
.debug_ranges 286376 286352 -24 -0.0
.strtab 380555 380543 -12 -0.0
Full report (1 build for cc32xx)
platform target config section 745da97 0bacd9f change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 645745 645769 24 0.0
(read/write) 203848 203848 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197248 197248 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 933102 933102 0 0.0
.debug_aranges 87704 87704 0 0.0
.debug_frame 301604 301596 -8 -0.0
.debug_info 20309906 20310044 138 0.0
.debug_line 2680953 2681022 69 0.0
.debug_loc 2827596 2827360 -236 -0.0
.debug_ranges 286376 286352 -24 -0.0
.debug_str 3041114 3041144 30 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105953 105953 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 380555 380543 -12 -0.0
.symtab 257456 257472 16 0.0
.text 537672 537696 24 0.0

src/app/OperationalSessionSetup.cpp Outdated Show resolved Hide resolved
src/app/OperationalSessionSetup.cpp Outdated Show resolved Hide resolved
src/app/OperationalSessionSetup.cpp Show resolved Hide resolved
@bzbarsky-apple bzbarsky-apple merged commit 0f1fc1b into project-chip:master Mar 29, 2023
@bzbarsky-apple bzbarsky-apple deleted the fix-CASE-crashes branch March 29, 2023 16:43
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