From ba75737d6e030410ba9511b689d03995e1394105 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Fri, 7 Jan 2022 14:13:07 -0600 Subject: [PATCH] Per bzbarsky, add a helper to discard our exchange reference without closing it --- src/protocols/secure_channel/CASESession.cpp | 45 +++++++++++--------- src/protocols/secure_channel/CASESession.h | 6 +++ src/protocols/secure_channel/PASESession.cpp | 35 +++++++++------ src/protocols/secure_channel/PASESession.h | 6 +++ 4 files changed, 61 insertions(+), 31 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index ce34b0618cac6e..43125581f21297 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -122,6 +122,19 @@ void CASESession::CloseExchange() } } +void CASESession::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 CASESession::ToCachable(CASESessionCachable & cachableSession) { const NodeId peerNodeId = GetPeerNodeId(); @@ -252,12 +265,9 @@ 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 %" PRIu8, mState); - // 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 + // Discard the exchange so that Clear() doesn't try closing it. The // exchange will handle that. - mExchangeCtxt = nullptr; + DiscardExchange(); Clear(); // Do this last in case the delegate frees us. mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); @@ -687,11 +697,9 @@ CHIP_ERROR CASESession::HandleSigma2Resume(System::PacketBufferHandle && msg) mCASESessionEstablished = true; - // Make sure the exchange doesn't try to notify us when it closes, - // since we might be dead by then. - mExchangeCtxt->SetDelegate(nullptr); - // Forget our exchange, as no additional messages are expected from the peer - mExchangeCtxt = nullptr; + // Discard the exchange so that Clear() doesn't try closing it. The + // exchange will handle that. + DiscardExchange(); // Call delegate to indicate session establishment is successful // Do this last in case the delegate frees us. @@ -1125,9 +1133,9 @@ CHIP_ERROR CASESession::HandleSigma3(System::PacketBufferHandle && msg) mCASESessionEstablished = true; - // Forget our exchange, as no additional messages are expected from the peer - mExchangeCtxt->SetDelegate(nullptr); - mExchangeCtxt = nullptr; + // Discard the exchange so that Clear() doesn't try closing it. The + // exchange will handle that. + DiscardExchange(); // Call delegate to indicate session establishment is successful // Do this last in case the delegate frees us. @@ -1311,9 +1319,9 @@ void CASESession::OnSuccessStatusReport() ChipLogProgress(SecureChannel, "Success status report received. Session was established"); mCASESessionEstablished = true; - // Forget our exchange, as no additional messages are expected from the peer - mExchangeCtxt->SetDelegate(nullptr); - mExchangeCtxt = nullptr; + // Discard the exchange so that Clear() doesn't try closing it. The + // exchange will handle that. + DiscardExchange(); mState = kInitialized; @@ -1534,10 +1542,9 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea // Call delegate to indicate session establishment failure. if (err != CHIP_NO_ERROR) { - // Null out mExchangeCtxt so that Clear() doesn't try closing it. The + // Discard the exchange so that Clear() doesn't try closing it. The // exchange will handle that. - mExchangeCtxt->SetDelegate(nullptr); - mExchangeCtxt = nullptr; + DiscardExchange(); Clear(); // Do this last in case the delegate frees us. mDelegate->OnSessionEstablishmentError(err); diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index b266158c011f4d..a780c298a6dd9a 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -220,6 +220,12 @@ class DLL_EXPORT CASESession : public Messaging::ExchangeDelegate, public Pairin void CloseExchange(); + /** + * Clear our reference to our exchange context pointer so that it can close + * itself at some later time. + */ + void DiscardExchange(); + // TODO: Remove this and replace with system method to retrieve current time CHIP_ERROR SetEffectiveTime(void); diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index a1a682fe621e1d..6c045af35174c7 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -116,6 +116,19 @@ void PASESession::CloseExchange() } } +void PASESession::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 PASESession::Serialize(PASESessionSerialized & output) { PASESessionSerializable serializable; @@ -343,10 +356,9 @@ void PASESession::OnResponseTimeout(ExchangeContext * ec) ChipLogError(SecureChannel, "PASESession timed out while waiting for a response from the peer. Expected message type was %" PRIu8, to_underlying(mNextExpectedMsg)); - // Null out mExchangeCtxt so that Clear() doesn't try closing it. The + // Discard the exchange so that Clear() doesn't try closing it. The // exchange will handle that. - mExchangeCtxt->SetDelegate(nullptr); - mExchangeCtxt = nullptr; + DiscardExchange(); Clear(); // Do this last in case the delegate frees us. mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); @@ -825,9 +837,9 @@ CHIP_ERROR PASESession::HandleMsg3(System::PacketBufferHandle && msg) mPairingComplete = true; - // Forget our exchange, as no additional messages are expected from the peer - mExchangeCtxt->SetDelegate(nullptr); - mExchangeCtxt = nullptr; + // Discard the exchange so that Clear() doesn't try closing it. The + // exchange will handle that. + DiscardExchange(); // Call delegate to indicate pairing completion // Do this last in case the delegate frees us. @@ -846,9 +858,9 @@ void PASESession::OnSuccessStatusReport() { mPairingComplete = true; - // Forget our exchange, as no additional messages are expected from the peer - mExchangeCtxt->SetDelegate(nullptr); - mExchangeCtxt = nullptr; + // Discard the exchange so that Clear() doesn't try closing it. The + // exchange will handle that. + DiscardExchange(); // Call delegate to indicate pairing completion // Do this last in case the delegate frees us. @@ -942,10 +954,9 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Payl // Call delegate to indicate pairing failure if (err != CHIP_NO_ERROR) { - // Null out mExchangeCtxt so that Clear() doesn't try closing it. The + // Discard the exchange so that Clear() doesn't try closing it. The // exchange will handle that. - mExchangeCtxt->SetDelegate(nullptr); - mExchangeCtxt = nullptr; + 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/PASESession.h b/src/protocols/secure_channel/PASESession.h index 2be0629dcc9035..2bf38dd8efd3c7 100644 --- a/src/protocols/secure_channel/PASESession.h +++ b/src/protocols/secure_channel/PASESession.h @@ -263,6 +263,12 @@ class DLL_EXPORT PASESession : public Messaging::ExchangeDelegate, public Pairin void CloseExchange(); + /** + * Clear our reference to our exchange context pointer so that it can close + * itself at some later time. + */ + void DiscardExchange(); + SessionEstablishmentDelegate * mDelegate = nullptr; Protocols::SecureChannel::MsgType mNextExpectedMsg = Protocols::SecureChannel::MsgType::PASE_PakeError;