diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp index 8eb131204c342b..221ca831eb5dc9 100644 --- a/src/protocols/secure_channel/CASEServer.cpp +++ b/src/protocols/secure_channel/CASEServer.cpp @@ -86,11 +86,8 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const SuccessOrExit(err); exit: - if (err != CHIP_NO_ERROR) - { - PrepareForSessionEstablishment(); - } - + // CASESession::OnMessageReceived guarantees that it will call + // OnSessionEstablishmentError if it returns error, so nothing else to do here. return err; } @@ -157,16 +154,7 @@ void CASEServer::OnSessionEstablishmentError(CHIP_ERROR err) { ChipLogError(Inet, "CASE Session establishment failed: %" CHIP_ERROR_FORMAT, err.Format()); - // - // We're not allowed to call methods that will eventually result in calling SessionManager::AllocateSecureSession - // from a SessionDelegate::OnSessionReleased callback. Schedule the preparation as an async work item. - // - mSessionManager->SystemLayer()->ScheduleWork( - [](auto * systemLayer, auto * appState) -> void { - CASEServer * _this = static_cast(appState); - _this->PrepareForSessionEstablishment(); - }, - this); + PrepareForSessionEstablishment(); } void CASEServer::OnSessionEstablished(const SessionHandle & session) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 0f2d2c2316c7af..2b22dd9e447ff4 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -134,9 +134,9 @@ CASESession::~CASESession() void CASESession::OnSessionReleased() { + // Call into our super-class before we clear our state. + PairingSession::OnSessionReleased(); Clear(); - // Do this last in case the delegate frees us. - NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); } void CASESession::Clear() diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 1537a33a42baed..16139a1f9301a3 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -75,9 +75,9 @@ PASESession::~PASESession() void PASESession::OnSessionReleased() { + // Call into our super-class before we clear our state. + PairingSession::OnSessionReleased(); Clear(); - // Do this last in case the delegate frees us. - NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); } void PASESession::Finish() diff --git a/src/protocols/secure_channel/PairingSession.cpp b/src/protocols/secure_channel/PairingSession.cpp index 67d7229b462709..a167015a947784 100644 --- a/src/protocols/secure_channel/PairingSession.cpp +++ b/src/protocols/secure_channel/PairingSession.cpp @@ -182,4 +182,28 @@ void PairingSession::NotifySessionEstablishmentError(CHIP_ERROR error) delegate->OnSessionEstablishmentError(error); } +void PairingSession::OnSessionReleased() +{ + if (mRole == CryptoContext::SessionRole::kInitiator) { + NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); + return; + } + + // Send the error notification async, because our delegate is likely to want + // to create a new session to listen for new connection attempts, and doing + // that under an OnSessionReleased notification is not safe. + if (!mSessionManager) + { + return; + } + + mSessionManager->SystemLayer()->ScheduleWork( + [](auto * systemLayer, auto * appState) -> void { + ChipLogError(Inet, "ASYNC CASE Session establishment failed"); + auto * _this = static_cast(appState); + _this->NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); + }, + this); +} + } // namespace chip diff --git a/src/protocols/secure_channel/PairingSession.h b/src/protocols/secure_channel/PairingSession.h index 78e7727eb2f55e..37c419cbcee0d7 100644 --- a/src/protocols/secure_channel/PairingSession.h +++ b/src/protocols/secure_channel/PairingSession.h @@ -51,6 +51,7 @@ class DLL_EXPORT PairingSession : public SessionDelegate // Implement SessionDelegate NewSessionHandlingPolicy GetNewSessionHandlingPolicy() override { return NewSessionHandlingPolicy::kStayAtOldSession; } + void OnSessionReleased() override; Optional GetLocalSessionId() const { diff --git a/src/protocols/secure_channel/tests/TestPASESession.cpp b/src/protocols/secure_channel/tests/TestPASESession.cpp index ab97624e39e473..85e7bb8429c212 100644 --- a/src/protocols/secure_channel/tests/TestPASESession.cpp +++ b/src/protocols/secure_channel/tests/TestPASESession.cpp @@ -324,6 +324,12 @@ void SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inContext, S NL_TEST_ASSERT(inSuite, session.HasValue()); session.Value()->AsSecureSession()->MarkForEviction(); + // Evicting a session async notifies the PASESession's delegate. Normally + // that notification is what would delete the PASESession, but in our case + // that will happen as soon as things come off the stack. So make sure to + // process the async bits before that happens. + ctx.DrainAndServiceIO(); + // And check that this did not result in any new notifications. NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0); NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 1);