Don't notify OnSessionEstablishmentError after OnSessionEstablished. (p…

…roject-chip#22261)

A PairingSession that has sent an OnSessionEstablished notification should not
send an OnSessionEstablishmentError notification after that (e.g. if the session
gets evicted).  The session is established at that point, and the
_establishment_ cannot hit an error.

This is needed to allow PASE sessions to be sanely evicted (e.g. when we're done
with them) without triggering spurious errors.

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

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;
 }

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

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;

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) {}
 

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);
 }