From dae94096b7522fd9d3d39ffa63f98cb9f67c6fd1 Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Thu, 5 May 2022 19:16:04 +0800 Subject: [PATCH] Resolve comments --- src/protocols/secure_channel/CASESession.cpp | 8 --- src/protocols/secure_channel/PASESession.cpp | 7 --- .../secure_channel/PairingSession.cpp | 61 ++++++++----------- src/protocols/secure_channel/PairingSession.h | 4 -- 4 files changed, 24 insertions(+), 56 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index ca15c021416f75..45377ca59e1c5d 100644 --- 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 index 942fb065312f17..4d0f1dd87387ee 100644 --- 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 index 51011bd2dcf3e8..1ce06c73b64aff 100644 --- 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 index 5aaa9f7ddb9785..9dc6ba15c3bad3 100644 --- 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)