From c9d066292635240ab070da21a55fcb5a0d6dd8fc Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Thu, 6 Jan 2022 19:40:35 -0600 Subject: [PATCH 1/6] Cleanup CASESession and PASESession lifetime The fix in #12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. This means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the code in the OperationalDeviceProxy that defers release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discarded immediately. --- src/app/OperationalDeviceProxy.cpp | 19 ++++++------------- src/app/OperationalDeviceProxy.h | 2 +- src/protocols/secure_channel/CASESession.cpp | 18 ++++++++++++++---- src/protocols/secure_channel/PASESession.cpp | 10 +++++++++- third_party/pigweed/repo | 2 +- 5 files changed, 31 insertions(+), 20 deletions(-) diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp index 9cf2234098ca51..5cead800fa3b55 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalDeviceProxy.cpp @@ -221,7 +221,7 @@ void OperationalDeviceProxy::HandleCASEConnectionFailure(void * context, CASECli device->DequeueConnectionSuccessCallbacks(/* executeCallback */ false); device->DequeueConnectionFailureCallbacks(error, /* executeCallback */ true); - device->DeferCloseCASESession(); + device->CloseCASESession(); } void OperationalDeviceProxy::HandleCASEConnected(void * context, CASEClient * client) @@ -242,7 +242,7 @@ void OperationalDeviceProxy::HandleCASEConnected(void * context, CASEClient * cl device->DequeueConnectionFailureCallbacks(CHIP_NO_ERROR, /* executeCallback */ false); device->DequeueConnectionSuccessCallbacks(/* executeCallback */ true); - device->DeferCloseCASESession(); + device->CloseCASESession(); } } @@ -280,22 +280,15 @@ void OperationalDeviceProxy::Clear() mInitParams = DeviceProxyInitParams(); } -void OperationalDeviceProxy::CloseCASESessionTask(System::Layer * layer, void * context) +void OperationalDeviceProxy::CloseCASESession() { - OperationalDeviceProxy * device = static_cast(context); - if (device->mCASEClient) + if (this->mCASEClient) { - device->mInitParams.clientPool->Release(device->mCASEClient); - device->mCASEClient = nullptr; + this->mInitParams.clientPool->Release(this->mCASEClient); + this->mCASEClient = nullptr; } } -void OperationalDeviceProxy::DeferCloseCASESession() -{ - // Defer the release for the pending Ack to be sent - mSystemLayer->ScheduleWork(CloseCASESessionTask, this); -} - void OperationalDeviceProxy::OnSessionReleased(const SessionHandle & session) { VerifyOrReturn(mSecureSession.Contains(session), diff --git a/src/app/OperationalDeviceProxy.h b/src/app/OperationalDeviceProxy.h index c56a92453c94f0..87a0c0d4f453cb 100644 --- a/src/app/OperationalDeviceProxy.h +++ b/src/app/OperationalDeviceProxy.h @@ -235,7 +235,7 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, SessionReleaseDele static void CloseCASESessionTask(System::Layer * layer, void * context); - void DeferCloseCASESession(); + void CloseCASESession(); void EnqueueConnectionCallbacks(Callback::Callback * onConnection, Callback::Callback * onFailure); diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 6555232ccb863b..fb856f828247c6 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -252,11 +252,13 @@ 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); - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); // Null out mExchangeCtxt so that Clear() doesn't try closing it. The // exchange will handle that. + mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; Clear(); + // Do this last in case the delegate frees us. + mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); } CHIP_ERROR CASESession::DeriveSecureSession(CryptoContext & session, CryptoContext::SessionRole role) @@ -684,9 +686,11 @@ CHIP_ERROR CASESession::HandleSigma2Resume(System::PacketBufferHandle && msg) mCASESessionEstablished = true; // Forget our exchange, as no additional messages are expected from the peer + mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; // Call delegate to indicate session establishment is successful + // Do this last in case the delegate frees us. mDelegate->OnSessionEstablished(); exit: @@ -1118,9 +1122,11 @@ 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; // Call delegate to indicate session establishment is successful + // Do this last in case the delegate frees us. mDelegate->OnSessionEstablished(); exit: @@ -1302,15 +1308,17 @@ void CASESession::OnSuccessStatusReport() mCASESessionEstablished = true; // Forget our exchange, as no additional messages are expected from the peer + mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; - // Call delegate to indicate pairing completion - mDelegate->OnSessionEstablished(); - mState = kInitialized; // TODO: Set timestamp on the new session, to allow selecting a least-recently-used session for eviction // on running out of session contexts. + + // Call delegate to indicate pairing completion. + // Do this last in case the delegate frees us. + mDelegate->OnSessionEstablished(); } CHIP_ERROR CASESession::OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) @@ -1524,8 +1532,10 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea { // Null out mExchangeCtxt so that Clear() doesn't try closing it. The // exchange will handle that. + mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; Clear(); + // Do this last in case the delegate frees us. mDelegate->OnSessionEstablishmentError(err); } return err; diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index f02b501feed4e4..a1a682fe621e1d 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -343,11 +343,13 @@ 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)); - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); // Null out mExchangeCtxt so that Clear() doesn't try closing it. The // exchange will handle that. + mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; Clear(); + // Do this last in case the delegate frees us. + mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); } CHIP_ERROR PASESession::DeriveSecureSession(CryptoContext & session, CryptoContext::SessionRole role) @@ -824,9 +826,11 @@ 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; // Call delegate to indicate pairing completion + // Do this last in case the delegate frees us. mDelegate->OnSessionEstablished(); exit: @@ -843,9 +847,11 @@ void PASESession::OnSuccessStatusReport() mPairingComplete = true; // Forget our exchange, as no additional messages are expected from the peer + mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; // Call delegate to indicate pairing completion + // Do this last in case the delegate frees us. mDelegate->OnSessionEstablished(); } @@ -938,9 +944,11 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Payl { // Null out mExchangeCtxt so that Clear() doesn't try closing it. The // exchange will handle that. + mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; Clear(); ChipLogError(SecureChannel, "Failed during PASE session setup. %s", ErrorStr(err)); + // Do this last in case the delegate frees us. mDelegate->OnSessionEstablishmentError(err); } return err; diff --git a/third_party/pigweed/repo b/third_party/pigweed/repo index f30bce8bf2513e..444f6d0df8a002 160000 --- a/third_party/pigweed/repo +++ b/third_party/pigweed/repo @@ -1 +1 @@ -Subproject commit f30bce8bf2513e924f461da6f2e7d1f2620a89f3 +Subproject commit 444f6d0df8a002a7c577066dcb1eca939c0a6b13 From 219f780f206f79c59f37294d8e04590e1f97b0e5 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Fri, 7 Jan 2022 12:54:28 -0600 Subject: [PATCH 2/6] Update src/app/OperationalDeviceProxy.cpp Co-authored-by: Boris Zbarsky --- src/app/OperationalDeviceProxy.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp index 5cead800fa3b55..06cd1c6cc21888 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalDeviceProxy.cpp @@ -282,7 +282,7 @@ void OperationalDeviceProxy::Clear() void OperationalDeviceProxy::CloseCASESession() { - if (this->mCASEClient) + if (mCASEClient) { this->mInitParams.clientPool->Release(this->mCASEClient); this->mCASEClient = nullptr; From ccac9d1eb46c6c889545117c7a4f36da474de275 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Fri, 7 Jan 2022 12:54:43 -0600 Subject: [PATCH 3/6] Update src/app/OperationalDeviceProxy.cpp Co-authored-by: Boris Zbarsky --- src/app/OperationalDeviceProxy.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp index 06cd1c6cc21888..d84eb7a2ce91ec 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalDeviceProxy.cpp @@ -284,8 +284,8 @@ void OperationalDeviceProxy::CloseCASESession() { if (mCASEClient) { - this->mInitParams.clientPool->Release(this->mCASEClient); - this->mCASEClient = nullptr; + mInitParams.clientPool->Release(mCASEClient); + mCASEClient = nullptr; } } From 94ebad8e385bc494506cd47cab60f157f15dd779 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Fri, 7 Jan 2022 12:54:54 -0600 Subject: [PATCH 4/6] Update src/protocols/secure_channel/CASESession.cpp Co-authored-by: Boris Zbarsky --- src/protocols/secure_channel/CASESession.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index fb856f828247c6..f2f50e387600cf 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -252,9 +252,11 @@ 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 // exchange will handle that. - mExchangeCtxt->SetDelegate(nullptr); mExchangeCtxt = nullptr; Clear(); // Do this last in case the delegate frees us. From 3d044247818b5f66613f929822991eb2d9a61426 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Fri, 7 Jan 2022 12:55:03 -0600 Subject: [PATCH 5/6] Update src/protocols/secure_channel/CASESession.cpp Co-authored-by: Boris Zbarsky --- src/protocols/secure_channel/CASESession.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index f2f50e387600cf..ce34b0618cac6e 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -687,8 +687,10 @@ CHIP_ERROR CASESession::HandleSigma2Resume(System::PacketBufferHandle && msg) mCASESessionEstablished = true; - // Forget our exchange, as no additional messages are expected from the peer + // 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; // Call delegate to indicate session establishment is successful From ba75737d6e030410ba9511b689d03995e1394105 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Fri, 7 Jan 2022 14:13:07 -0600 Subject: [PATCH 6/6] 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;