diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 5f4c04752fe389..623265d160349a 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -131,7 +131,7 @@ void CASESession::OnSessionReleased() { Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); + NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); } void CASESession::Clear() @@ -294,7 +294,7 @@ void CASESession::AbortPendingEstablish(CHIP_ERROR err) { Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(err); + NotifySessionEstablishmentError(err); } CHIP_ERROR CASESession::DeriveSecureSession(CryptoContext & session) const diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 6c48957a14d275..05fc3b38915983 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -72,7 +72,7 @@ void PASESession::OnSessionReleased() { Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); + NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); } void PASESession::Finish() @@ -242,7 +242,7 @@ void PASESession::OnResponseTimeout(ExchangeContext * ec) DiscardExchange(); Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); + NotifySessionEstablishmentError(CHIP_ERROR_TIMEOUT); } CHIP_ERROR PASESession::DeriveSecureSession(CryptoContext & session) const @@ -859,7 +859,7 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Payl Clear(); ChipLogError(SecureChannel, "Failed during PASE session setup: %" CHIP_ERROR_FORMAT, err.Format()); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(err); + NotifySessionEstablishmentError(err); } return err; } diff --git a/src/protocols/secure_channel/PairingSession.cpp b/src/protocols/secure_channel/PairingSession.cpp index 8c1dee5968ed4a..67d7229b462709 100644 --- a/src/protocols/secure_channel/PairingSession.cpp +++ b/src/protocols/secure_channel/PairingSession.cpp @@ -63,11 +63,15 @@ void PairingSession::Finish() if (err == CHIP_NO_ERROR) { VerifyOrDie(mSecureSessionHolder); - mDelegate->OnSessionEstablished(mSecureSessionHolder.Get().Value()); + // Make sure to null out mDelegate so we don't send it any other + // notifications. + auto * delegate = mDelegate; + mDelegate = nullptr; + delegate->OnSessionEstablished(mSecureSessionHolder.Get().Value()); } else { - mDelegate->OnSessionEstablishmentError(err); + NotifySessionEstablishmentError(err); } } @@ -165,4 +169,17 @@ void PairingSession::Clear() mSessionManager = nullptr; } +void PairingSession::NotifySessionEstablishmentError(CHIP_ERROR error) +{ + if (mDelegate == nullptr) + { + // Already notified success or error. + return; + } + + auto * delegate = mDelegate; + mDelegate = nullptr; + delegate->OnSessionEstablishmentError(error); +} + } // namespace chip diff --git a/src/protocols/secure_channel/PairingSession.h b/src/protocols/secure_channel/PairingSession.h index f8a707aa3af2d1..e220346e11b4a6 100644 --- a/src/protocols/secure_channel/PairingSession.h +++ b/src/protocols/secure_channel/PairingSession.h @@ -192,6 +192,12 @@ class DLL_EXPORT PairingSession : public SessionDelegate // TODO: remove Clear, we should create a new instance instead reset the old instance. void Clear(); + /** + * Notify our delegate about a session establishment error, if we have not + * notified it of an error or success before. + */ + void NotifySessionEstablishmentError(CHIP_ERROR error); + protected: CryptoContext::SessionRole mRole; SessionHolderWithDelegate mSecureSessionHolder; diff --git a/src/protocols/secure_channel/SessionEstablishmentDelegate.h b/src/protocols/secure_channel/SessionEstablishmentDelegate.h index f14566dae5a092..30cc8cac59f76b 100644 --- a/src/protocols/secure_channel/SessionEstablishmentDelegate.h +++ b/src/protocols/secure_channel/SessionEstablishmentDelegate.h @@ -36,7 +36,9 @@ class DLL_EXPORT SessionEstablishmentDelegate { public: /** - * Called when session establishment fails with an error + * Called when session establishment fails with an error. This will be + * called at most once per session establishment and will not be called if + * OnSessionEstablished is called. */ virtual void OnSessionEstablishmentError(CHIP_ERROR error) {} @@ -46,7 +48,9 @@ class DLL_EXPORT SessionEstablishmentDelegate virtual void OnSessionEstablishmentStarted() {} /** - * Called when the new secure session has been established + * Called when the new secure session has been established. This is + * mututally exclusive with OnSessionEstablishmentError for a give session + * establishment. */ virtual void OnSessionEstablished(const SessionHandle & session) {} diff --git a/src/protocols/secure_channel/tests/TestPASESession.cpp b/src/protocols/secure_channel/tests/TestPASESession.cpp index 61830fa174b530..ab97624e39e473 100644 --- a/src/protocols/secure_channel/tests/TestPASESession.cpp +++ b/src/protocols/secure_channel/tests/TestPASESession.cpp @@ -290,7 +290,9 @@ void SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inContext, S // Let's make sure atleast number is >= than the minimum messages required to complete the // handshake. NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount >= sTestPaseMessageCount); + NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0); NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 1); + NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 0); NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 1); if (mrpCommissionerConfig.HasValue()) @@ -313,6 +315,21 @@ void SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inContext, S mrpAccessoryConfig.Value().mActiveRetransTimeout); } + // Now evict the PASE sessions. + auto session = pairingCommissioner.CopySecureSession(); + NL_TEST_ASSERT(inSuite, session.HasValue()); + session.Value()->AsSecureSession()->MarkForEviction(); + + session = pairingAccessory.CopySecureSession(); + NL_TEST_ASSERT(inSuite, session.HasValue()); + session.Value()->AsSecureSession()->MarkForEviction(); + + // 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); + NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 0); + NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 1); + loopback.SetLoopbackTransportDelegate(nullptr); }