Fix CASEServer cleanup to not nuke unrelated CASE handshakes. #23644
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
lazarkov,
LuDuda,
mlepage-google,
msandstedt,
mspang,
rgoliver,
robszewczyk,
saurabhst and
selissia
pullapprove
bot
requested review from
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
|
PR #23644: Size comparison from bbf3111 to f4ac24c Increases (17 builds for bl602, bl702, k32w, linux, mbed, nrfconnect, qpg, telink)
Decreases (6 builds for bl702, k32w, linux)
Full report (19 builds for bl602, bl702, k32w, linux, mbed, nrfconnect, qpg, telink)
|
bzbarsky-apple
force-pushed
the
fix-async-CASEServer-cleanup
branch
from
f4ac24c
to
1e7384e
Compare
|
PR #23644: Size comparison from bbf3111 to 1e7384e Increases (36 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (30 builds for bl702, cc13x2_26x2, cyw30739, k32w, linux, psoc6)
Full report (51 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bzbarsky-apple
force-pushed
the
fix-async-CASEServer-cleanup
branch
from
1e7384e
to
2b06024
Compare
We could run into a problem when an invalid Sigma1 (e.g. not matching any fabric) was received, because we would do the following: 1) Call CASEServer::OnSessionEstablishmentError from inside CASESession::OnMessageReceived, which would queue a task to blow away the CASEServer state and start listening for new CASE handshakes (PrepareForSessionEstablishment). 2) Return an error from CASESession::OnMessageReceived, which would cause us to synchronously do call PrepareForSessionEstablishment. If we now got a new Sigma1 before the async task ran (e.g. if it was already waiting in the queue), we could respond to the Sigma1 with Sigma2Resume, then the async task would run and blow away our state. We would end up in a situation where the other side then sends a StatusResponse and thinks CASE is established, but on our end we don't recognize the session ID for the session they think has been established. The fix is to restrict the async behavior to the one case it's actually needed (OnSessionReleased), for both CASE and PASE, to synchronously call PrepareForSessionEstablishment from OnSessionEstablishmentError, and to remove the now-redundant PrepareForSessionEstablishment call on error return from CASESession::OnMessageReceived.
bzbarsky-apple
force-pushed
the
fix-async-CASEServer-cleanup
branch
from
2b06024
to
b17b1a5
Compare
|
PR #23644: Size comparison from 361c74d to b17b1a5 Increases (36 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (17 builds for bl702, cc13x2_26x2, cyw30739, k32w, psoc6)
Full report (40 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
msandstedt
approved these changes
Nov 17, 2022
andy31415
approved these changes
Nov 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We could run into a problem when an invalid Sigma1 (e.g. not matching any fabric) was received, because we would do the following:
Call CASEServer::OnSessionEstablishmentError from inside
CASESession::OnMessageReceived, which would queue a task to blow away the
CASEServer state and start listening for new CASE handshakes
(PrepareForSessionEstablishment).
Return an error from CASESession::OnMessageReceived, which would cause us to
synchronously do call PrepareForSessionEstablishment.
If we now got a new Sigma1 before the async task ran (e.g. if it was already waiting in the queue), we could respond to the Sigma1 with Sigma2Resume, then the async task would run and blow away our state.
We would end up in a situation where the other side then sends a StatusResponse and thinks CASE is established, but on our end we don't recognize the session ID for the session they think has been established.
The fix is to restrict the async behavior to the one case it's actually needed (OnSessionReleased), for both CASE and PASE, to synchronously call PrepareForSessionEstablishment from OnSessionEstablishmentError, and to remove the now-redundant PrepareForSessionEstablishment call on error return from CASESession::OnMessageReceived.