Resolve comments

project-chip · May 5, 2022 · dae9409 · dae9409
1 parent 052193a
commit dae9409
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 56 deletions.
diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp
@@ -137,8 +137,6 @@ void CASESession::Clear()
     mState = State::kInitialized;
     Crypto::ClearSecretData(mIPK);
 
-    AbortExchange();
-
     mLocalNodeId = kUndefinedNodeId;
     mPeerNodeId  = kUndefinedNodeId;
     mFabricInfo  = nullptr;
@@ -233,9 +231,6 @@ void CASESession::OnResponseTimeout(ExchangeContext * ec)
     VerifyOrReturn(mExchangeCtxt == ec, ChipLogError(SecureChannel, "CASESession::OnResponseTimeout exchange doesn't match"));
     ChipLogError(SecureChannel, "CASESession timed out while waiting for a response from the peer. Current state was %u",
                  to_underlying(mState));
-    // Discard the exchange so that Clear() doesn't try closing it.  The
-    // exchange will handle that.
-    DiscardExchange();
     Clear();
     // Do this last in case the delegate frees us.
     mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
@@ -1724,9 +1719,6 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
     // Call delegate to indicate session establishment failure.
     if (err != CHIP_NO_ERROR)
     {
-        // Discard the exchange so that Clear() doesn't try closing it.  The
-        // exchange will handle that.
-        DiscardExchange();
         Clear();
         // Do this last in case the delegate frees us.
         mDelegate->OnSessionEstablishmentError(err);

diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp
@@ -96,7 +96,6 @@ void PASESession::Clear()
     mKeLen           = sizeof(mKe);
     mPairingComplete = false;
     PairingSession::Clear();
-    CloseExchange();
 }
 
 CHIP_ERROR PASESession::Init(SessionManager & sessionManager, uint32_t setupCode, SessionEstablishmentDelegate * delegate)
@@ -231,9 +230,6 @@ void PASESession::OnResponseTimeout(ExchangeContext * ec)
                    ChipLogError(SecureChannel, "PASESession::OnResponseTimeout exchange doesn't match"));
     ChipLogError(SecureChannel, "PASESession timed out while waiting for a response from the peer. Expected message type was %u",
                  to_underlying(mNextExpectedMsg));
-    // Discard the exchange so that Clear() doesn't try closing it.  The
-    // exchange will handle that.
-    DiscardExchange();
     Clear();
     // Do this last in case the delegate frees us.
     mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
@@ -833,9 +829,6 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Payl
     // Call delegate to indicate pairing failure
     if (err != CHIP_NO_ERROR)
     {
-        // Discard the exchange so that Clear() doesn't try closing it.  The
-        // exchange will handle that.
-        DiscardExchange();
         Clear();
         ChipLogError(SecureChannel, "Failed during PASE session setup. %s", ErrorStr(err));
         // Do this last in case the delegate frees us.

diff --git a/src/protocols/secure_channel/PairingSession.cpp b/src/protocols/secure_channel/PairingSession.cpp
@@ -61,8 +61,17 @@ void PairingSession::Finish()
 {
     Transport::PeerAddress address = mExchangeCtxt->GetSessionHandle()->AsUnauthenticatedSession()->GetPeerAddress();
 
-    // Discard the exchange so that Clear() doesn't try closing it. The exchange will handle that.
-    DiscardExchange();
+    // Discard the exchange such that `Clear()` does not try aborting it. The
+    // exchange is needed to run MRP for the last message.
+    if (mExchangeCtxt != nullptr)
+    {
+        // Make sure the exchange doesn't try to notify us when it closes,
+        // since we might be dead by then.
+        mExchangeCtxt->SetDelegate(nullptr);
+        // Null out mExchangeCtxt so that our subclasses don't try closing it.  The
+        // exchange will handle that.
+        mExchangeCtxt = nullptr;
+    }
 
     CHIP_ERROR err = ActivateSecureSession(address);
     if (err == CHIP_NO_ERROR)
@@ -75,41 +84,6 @@ void PairingSession::Finish()
     }
 }
 
-void PairingSession::AbortExchange()
-{
-    if (mExchangeCtxt != nullptr)
-    {
-        // The only time we reach this is if we are getting destroyed in the
-        // middle of our handshake.  In that case, there is no point trying to
-        // do MRP resends of the last message we sent, so abort the exchange
-        // instead of just closing it.
-        mExchangeCtxt->Abort();
-        mExchangeCtxt = nullptr;
-    }
-}
-
-void PairingSession::CloseExchange()
-{
-    if (mExchangeCtxt != nullptr)
-    {
-        mExchangeCtxt->Close();
-        mExchangeCtxt = nullptr;
-    }
-}
-
-void PairingSession::DiscardExchange()
-{
-    if (mExchangeCtxt != nullptr)
-    {
-        // Make sure the exchange doesn't try to notify us when it closes,
-        // since we might be dead by then.
-        mExchangeCtxt->SetDelegate(nullptr);
-        // Null out mExchangeCtxt so that Clear() doesn't try closing it.  The
-        // exchange will handle that.
-        mExchangeCtxt = nullptr;
-    }
-}
-
 CHIP_ERROR PairingSession::EncodeMRPParameters(TLV::Tag tag, const ReliableMessageProtocolConfig & mrpConfig,
                                                TLV::TLVWriter & tlvWriter)
 {
@@ -162,6 +136,19 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
 
 void PairingSession::Clear()
 {
+    // Clear acts like the destructor if PairingSession, if it is call during
+    // middle of a pairing, means we should terminate the exchange. For normal
+    // path, the exchange should already be discarded before calling Clear.
+    if (mExchangeCtxt != nullptr)
+    {
+        // The only time we reach this is if we are getting destroyed in the
+        // middle of our handshake.  In that case, there is no point trying to
+        // do MRP resends of the last message we sent, so abort the exchange
+        // instead of just closing it.
+        mExchangeCtxt->Abort();
+        mExchangeCtxt = nullptr;
+    }
+
     if (mSessionManager != nullptr)
     {
         if (mSecureSessionHolder && !mSecureSessionHolder->AsSecureSession()->IsActiveSession())

diff --git a/src/protocols/secure_channel/PairingSession.h b/src/protocols/secure_channel/PairingSession.h
@@ -98,10 +98,6 @@ class DLL_EXPORT PairingSession
 
     void Finish();
 
-    void AbortExchange();
-    void CloseExchange();
-    void DiscardExchange();
-
     void SetPeerSessionId(uint16_t id) { mPeerSessionId.SetValue(id); }
     virtual void OnSuccessStatusReport() {}
     virtual CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode)