From a23df45f73ca7d1b09dbe8787e5e07a7cc4992a1 Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Fri, 23 Jun 2023 17:49:56 +0530 Subject: [PATCH 1/9] CASE: Handle failure if unable to schedule handle/send sigma3c - We are no longer unregistering the unsolicit message hander for sigma1. Based on handshake state and failure to schedule work, decide what to do. 1. If in middle of handshake but it's zombie, tear down the handshake. 2. If still in middle of handshake, return without responding. 3. Otherwise, jsut do a new handshake. - Added APIs in helper to check if it fails to schedule after work callback and to re run it from foreground thread. - Added wrapper around the APIs for HandleSigma3 and SendSigma3 cases. --- src/protocols/secure_channel/CASEServer.cpp | 23 ++++++++- src/protocols/secure_channel/CASEServer.h | 3 ++ src/protocols/secure_channel/CASESession.cpp | 50 ++++++++++++++++++++ src/protocols/secure_channel/CASESession.h | 13 +++++ 4 files changed, 87 insertions(+), 2 deletions(-) diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp index b0d74694a5196b..7abb04bf336654 100644 --- a/src/protocols/secure_channel/CASEServer.cpp +++ b/src/protocols/secure_channel/CASEServer.cpp @@ -73,6 +73,20 @@ CHIP_ERROR CASEServer::OnUnsolicitedMessageReceived(const PayloadHeader & payloa CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) { + if (mDoingCASEHandshake) + { + if (GetSession().IsHandleSigma3PermanentlyBlockedOnBackgroundWork()) + { + ChipLogError(Inet, "CASE session helper failed to schedule HandleSigma3c, executing HandleSigma3c"); + GetSession().UnblockHandleSigma3FromForegroundWork(); + } + else + { + ChipLogError(Inet, "CASE session is in establishing state, returning without responding"); + return CHIP_NO_ERROR; + } + } + if (!ec->GetSessionHandle()->IsUnauthenticatedSession()) { ChipLogError(Inet, "CASE Server received Sigma1 message %s EC %p", "over encrypted session. Ignoring.", ec); @@ -84,10 +98,13 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const CHIP_ERROR err = InitCASEHandshake(ec); SuccessOrExit(err); + mDoingCASEHandshake = true; + // TODO - Enable multiple concurrent CASE session establishment // https://github.com/project-chip/connectedhomeip/issues/8342 - ChipLogProgress(Inet, "CASE Server disabling CASE session setups"); - mExchangeManager->UnregisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::CASE_Sigma1); + + // Earlier we used to unregister the unsolicit message handler for Sigma1 messages. + // Now, we keep it registered and perform the appropriate action in above block err = GetSession().OnMessageReceived(ec, payloadHeader, std::move(payload)); SuccessOrExit(err); @@ -100,6 +117,8 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyEstablishedPeer) { + mDoingCASEHandshake = false; + // Let's re-register for CASE Sigma1 message, so that the next CASE session setup request can be processed. // https://github.com/project-chip/connectedhomeip/issues/8342 ChipLogProgress(Inet, "CASE Server enabling CASE session setups"); diff --git a/src/protocols/secure_channel/CASEServer.h b/src/protocols/secure_channel/CASEServer.h index 894d496c93bfac..1206c96f61062e 100644 --- a/src/protocols/secure_channel/CASEServer.h +++ b/src/protocols/secure_channel/CASEServer.h @@ -89,6 +89,9 @@ class CASEServer : public SessionEstablishmentDelegate, // Optional mPinnedSecureSession; + // Track if we are in the middle of secure session handshake + bool mDoingCASEHandshake = false; + CASESession mPairingSession; SessionManager * mSessionManager = nullptr; diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index def2ce11cf0c08..90bc355e6d1793 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -203,6 +203,27 @@ class CASESession::WorkHelper bool IsCancelled() const { return mSession.load() == nullptr; } + // This API returns true when background thread fails to schedule the AfterWorkCallback + bool SchedulingAfterWorkCallbackFailed() { return mScheduleAfterWorkFailed.load(); } + + // This API runs the AfterWorkCallback. This is supposed to be called from foreground thread + // and only call if SchedulingAfterWorkCallbackFailed() returns false. + CHIP_ERROR RunAfterWorkCallback() + { + // Ensure that this function is being called from main Matter thread + assertChipStackLockedByCurrentThread(); + + // This API is supposed to be called only if scheduling after work callback is failed + // If we are are being called, assert + VerifyOrDie(SchedulingAfterWorkCallbackFailed()); + + if (auto *session = mSession.load()) + { + return (session->*(mAfterWorkCallback))(mData, mStatus); + } + return CHIP_ERROR_NOT_FOUND; + } + private: // Create a work helper using the specified session, work callback, after work callback, and data (template arg). // Lifetime is not managed, see `Create` for that option. @@ -226,6 +247,11 @@ class CASESession::WorkHelper auto status = DeviceLayer::PlatformMgr().ScheduleWork(AfterWorkHandler, reinterpret_cast(helper)); if (status != CHIP_NO_ERROR) { + // We failed to schedule after work callback, so setting mScheduleAfterWorkFailed flag to true + // This can be checked from foreground thread and after work callback can be retried + ChipLogError(SecureChannel, "Failed to Schedule the AfterWorkCallback on foreground thread"); + helper->mScheduleAfterWorkFailed.store(true); + // Release strong ptr since scheduling failed helper->mStrongPtr.reset(); } @@ -263,6 +289,10 @@ class CASESession::WorkHelper // Return value of `mWorkCallback`, passed to `mAfterWorkCallback`. CHIP_ERROR mStatus; + // If background thread fails to schedule AfterWorkCallback then this flag is set to true + // and CASEServer then can check this one and run the AfterWorkCallback for us. + std::atomic mScheduleAfterWorkFailed {false}; + public: // Data passed to `mWorkCallback` and `mAfterWorkCallback`. DATA mData; @@ -2179,4 +2209,24 @@ System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableM kExpectedHighProcessingTime; } +bool CASESession::IsSendSigma3PermanentlyBlockedOnBackgroundWork() +{ + return mSendSigma3Helper->SchedulingAfterWorkCallbackFailed(); +} + +bool CASESession::IsHandleSigma3PermanentlyBlockedOnBackgroundWork() +{ + return mHandleSigma3Helper->SchedulingAfterWorkCallbackFailed(); +} + +CHIP_ERROR CASESession::UnblockSendSigma3FromForegroundWork() +{ + return mSendSigma3Helper->RunAfterWorkCallback(); +} + +CHIP_ERROR CASESession::UnblockHandleSigma3FromForegroundWork() +{ + return mHandleSigma3Helper->RunAfterWorkCallback(); +} + } // namespace chip diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index f0caa1675f7845..56154def107a47 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -191,6 +191,19 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, **/ void Clear(); + // These APIs returns true if background thread fails to schedule the after work callback + // Then someone can run the after work callback in foreground thread + bool IsSendSigma3PermanentlyBlockedOnBackgroundWork(); + bool IsHandleSigma3PermanentlyBlockedOnBackgroundWork(); + + // If work helper fails to schedule the after work callback then call these function to run the + // after work callback from foreground thread. + // Please make sure the check if the work is really blocked on background thread + // NOTE: These APIs will assert if not called from main matter thread and work is not blocked + // in background thread. + CHIP_ERROR UnblockSendSigma3FromForegroundWork(); + CHIP_ERROR UnblockHandleSigma3FromForegroundWork(); + private: friend class TestCASESession; enum class State : uint8_t From c1e7ae0f5ef1bc8d972c1f589da17f85e8be16c1 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Mon, 26 Jun 2023 11:32:41 +0000 Subject: [PATCH 2/9] Restyled by clang-format --- src/protocols/secure_channel/CASESession.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 90bc355e6d1793..c0ac9d192aa90d 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -217,7 +217,7 @@ class CASESession::WorkHelper // If we are are being called, assert VerifyOrDie(SchedulingAfterWorkCallbackFailed()); - if (auto *session = mSession.load()) + if (auto * session = mSession.load()) { return (session->*(mAfterWorkCallback))(mData, mStatus); } @@ -291,7 +291,7 @@ class CASESession::WorkHelper // If background thread fails to schedule AfterWorkCallback then this flag is set to true // and CASEServer then can check this one and run the AfterWorkCallback for us. - std::atomic mScheduleAfterWorkFailed {false}; + std::atomic mScheduleAfterWorkFailed{ false }; public: // Data passed to `mWorkCallback` and `mAfterWorkCallback`. From f5116800f3517b2480ef2fede5a06fde457a2cb6 Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Tue, 27 Jun 2023 12:37:50 +0530 Subject: [PATCH 3/9] Address review comments Added the accessor for CASESession state. Now, all the logic for checking and resetting stays with CASESession --- src/protocols/secure_channel/CASEServer.cpp | 20 ++----- src/protocols/secure_channel/CASEServer.h | 3 - src/protocols/secure_channel/CASESession.cpp | 60 +++++++++++--------- src/protocols/secure_channel/CASESession.h | 24 +++----- 4 files changed, 49 insertions(+), 58 deletions(-) diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp index 7abb04bf336654..c544fbe2c842db 100644 --- a/src/protocols/secure_channel/CASEServer.cpp +++ b/src/protocols/secure_channel/CASEServer.cpp @@ -73,15 +73,14 @@ CHIP_ERROR CASEServer::OnUnsolicitedMessageReceived(const PayloadHeader & payloa CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) { - if (mDoingCASEHandshake) + if (GetSession().GetState() != CASESession::State::kInitialized) { - if (GetSession().IsHandleSigma3PermanentlyBlockedOnBackgroundWork()) - { - ChipLogError(Inet, "CASE session helper failed to schedule HandleSigma3c, executing HandleSigma3c"); - GetSession().UnblockHandleSigma3FromForegroundWork(); - } - else + // We are in the middle of CASE handshake + + // Check if we are stuck on background thread, if yes, unblock it, and continue + if (GetSession().InvokeBackgroundWorkWatchdog() == false) { + // TODO: Send Busy response, #27473 ChipLogError(Inet, "CASE session is in establishing state, returning without responding"); return CHIP_NO_ERROR; } @@ -98,14 +97,9 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const CHIP_ERROR err = InitCASEHandshake(ec); SuccessOrExit(err); - mDoingCASEHandshake = true; - // TODO - Enable multiple concurrent CASE session establishment // https://github.com/project-chip/connectedhomeip/issues/8342 - // Earlier we used to unregister the unsolicit message handler for Sigma1 messages. - // Now, we keep it registered and perform the appropriate action in above block - err = GetSession().OnMessageReceived(ec, payloadHeader, std::move(payload)); SuccessOrExit(err); @@ -117,8 +111,6 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyEstablishedPeer) { - mDoingCASEHandshake = false; - // Let's re-register for CASE Sigma1 message, so that the next CASE session setup request can be processed. // https://github.com/project-chip/connectedhomeip/issues/8342 ChipLogProgress(Inet, "CASE Server enabling CASE session setups"); diff --git a/src/protocols/secure_channel/CASEServer.h b/src/protocols/secure_channel/CASEServer.h index 1206c96f61062e..894d496c93bfac 100644 --- a/src/protocols/secure_channel/CASEServer.h +++ b/src/protocols/secure_channel/CASEServer.h @@ -89,9 +89,6 @@ class CASEServer : public SessionEstablishmentDelegate, // Optional mPinnedSecureSession; - // Track if we are in the middle of secure session handshake - bool mDoingCASEHandshake = false; - CASESession mPairingSession; SessionManager * mSessionManager = nullptr; diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index c0ac9d192aa90d..0558f6f7893d9e 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -171,6 +171,9 @@ class CASESession::WorkHelper // No scheduling, no outstanding work, no shared lifetime management. CHIP_ERROR DoWork() { + // Ensure that this function is being called from main Matter thread + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mSession && mWorkCallback && mAfterWorkCallback, CHIP_ERROR_INCORRECT_STATE); auto * helper = this; bool cancel = false; @@ -204,24 +207,20 @@ class CASESession::WorkHelper bool IsCancelled() const { return mSession.load() == nullptr; } // This API returns true when background thread fails to schedule the AfterWorkCallback - bool SchedulingAfterWorkCallbackFailed() { return mScheduleAfterWorkFailed.load(); } + bool UnableToScheduleAfterWorkCallback() { return mScheduleAfterWorkFailed.load(); } - // This API runs the AfterWorkCallback. This is supposed to be called from foreground thread - // and only call if SchedulingAfterWorkCallbackFailed() returns false. - CHIP_ERROR RunAfterWorkCallback() + // Do after work immediately. + // No scheduling, no outstanding work, no shared lifetime management. + CHIP_ERROR DoAfterWork() { // Ensure that this function is being called from main Matter thread assertChipStackLockedByCurrentThread(); - // This API is supposed to be called only if scheduling after work callback is failed - // If we are are being called, assert - VerifyOrDie(SchedulingAfterWorkCallbackFailed()); + VerifyOrReturnError(mSession && mAfterWorkCallback, CHIP_ERROR_INCORRECT_STATE); - if (auto * session = mSession.load()) - { - return (session->*(mAfterWorkCallback))(mData, mStatus); - } - return CHIP_ERROR_NOT_FOUND; + auto * helper = this; + helper->mStatus = (helper->mSession->*(helper->mAfterWorkCallback))(helper->mData, helper->mStatus); + return helper->mStatus; } private: @@ -2209,24 +2208,33 @@ System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableM kExpectedHighProcessingTime; } -bool CASESession::IsSendSigma3PermanentlyBlockedOnBackgroundWork() +bool CASESession::InvokeBackgroundWorkWatchdog() { - return mSendSigma3Helper->SchedulingAfterWorkCallbackFailed(); -} + bool status = false; -bool CASESession::IsHandleSigma3PermanentlyBlockedOnBackgroundWork() -{ - return mHandleSigma3Helper->SchedulingAfterWorkCallbackFailed(); -} + if (mSendSigma3Helper && mSendSigma3Helper->UnableToScheduleAfterWorkCallback()) + { + ChipLogError(SecureChannel, "SendSigma3Helper was unable to schedule the AfterWorkCallback"); + if (mSendSigma3Helper->DoAfterWork() != CHIP_NO_ERROR) + { + SendStatusReport(mExchangeCtxt, kProtocolCodeInvalidParam); + mState = State::kInitialized; + } + status = true; + } -CHIP_ERROR CASESession::UnblockSendSigma3FromForegroundWork() -{ - return mSendSigma3Helper->RunAfterWorkCallback(); -} + if (mHandleSigma3Helper && mHandleSigma3Helper->UnableToScheduleAfterWorkCallback()) + { + ChipLogError(SecureChannel, "HandleSigma3Helper was unable to schedule the AfterWorkCallback"); + if (mHandleSigma3Helper->DoAfterWork() != CHIP_NO_ERROR) + { + SendStatusReport(mExchangeCtxt, kProtocolCodeInvalidParam); + mState = State::kInitialized; + } + status = true; + } -CHIP_ERROR CASESession::UnblockHandleSigma3FromForegroundWork() -{ - return mHandleSigma3Helper->RunAfterWorkCallback(); + return status; } } // namespace chip diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index 56154def107a47..788864ad053f3a 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -191,21 +191,6 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, **/ void Clear(); - // These APIs returns true if background thread fails to schedule the after work callback - // Then someone can run the after work callback in foreground thread - bool IsSendSigma3PermanentlyBlockedOnBackgroundWork(); - bool IsHandleSigma3PermanentlyBlockedOnBackgroundWork(); - - // If work helper fails to schedule the after work callback then call these function to run the - // after work callback from foreground thread. - // Please make sure the check if the work is really blocked on background thread - // NOTE: These APIs will assert if not called from main matter thread and work is not blocked - // in background thread. - CHIP_ERROR UnblockSendSigma3FromForegroundWork(); - CHIP_ERROR UnblockHandleSigma3FromForegroundWork(); - -private: - friend class TestCASESession; enum class State : uint8_t { kInitialized = 0, @@ -220,6 +205,15 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, kHandleSigma3Pending = 9, }; + State GetState() { return mState; } + + // Check if any of the work helper is unable to schedule the after work callback + // If they are stuck run the after work callback from the foreground thread. + bool InvokeBackgroundWorkWatchdog(); + +private: + friend class TestCASESession; + /* * Initialize the object given a reference to the SessionManager, certificate validity policy and a delegate which will be * notified of any further progress on this session. From c73229f96488f2023465392a315946a6ef2aea22 Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Tue, 27 Jun 2023 13:05:58 +0530 Subject: [PATCH 4/9] Fix some API docs --- src/protocols/secure_channel/CASESession.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index 788864ad053f3a..c12b58a8b7a3e3 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -207,8 +207,8 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, State GetState() { return mState; } - // Check if any of the work helper is unable to schedule the after work callback - // If they are stuck run the after work callback from the foreground thread. + // Checks if any of the work helper is unable to schedule the after work callback + // If they are stuck, runs the after work callback from the foreground thread. bool InvokeBackgroundWorkWatchdog(); private: From 50037cfe3a3e7cc42c8aff75739c7f1a3e88f30e Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Tue, 27 Jun 2023 23:08:57 +0530 Subject: [PATCH 5/9] Addressed review comments --- src/protocols/secure_channel/CASESession.cpp | 36 +++++++------------- src/protocols/secure_channel/CASESession.h | 4 +-- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 0558f6f7893d9e..22beaa5fd6061a 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -211,16 +211,10 @@ class CASESession::WorkHelper // Do after work immediately. // No scheduling, no outstanding work, no shared lifetime management. - CHIP_ERROR DoAfterWork() + void DoAfterWork() { - // Ensure that this function is being called from main Matter thread - assertChipStackLockedByCurrentThread(); - - VerifyOrReturnError(mSession && mAfterWorkCallback, CHIP_ERROR_INCORRECT_STATE); - - auto * helper = this; - helper->mStatus = (helper->mSession->*(helper->mAfterWorkCallback))(helper->mData, helper->mStatus); - return helper->mStatus; + VerifyOrDie(UnableToScheduleAfterWorkCallback()); + AfterWorkHandler(reinterpret_cast(this)); } private: @@ -250,6 +244,7 @@ class CASESession::WorkHelper // This can be checked from foreground thread and after work callback can be retried ChipLogError(SecureChannel, "Failed to Schedule the AfterWorkCallback on foreground thread"); helper->mScheduleAfterWorkFailed.store(true); + helper->mStatus = status; // Release strong ptr since scheduling failed helper->mStrongPtr.reset(); @@ -259,6 +254,9 @@ class CASESession::WorkHelper // Handler for the after work callback. static void AfterWorkHandler(intptr_t arg) { + // Ensure that this function is being called from main Matter thread + assertChipStackLockedByCurrentThread(); + auto * helper = reinterpret_cast(arg); // Hold strong ptr while work is handled auto strongPtr(std::move(helper->mStrongPtr)); @@ -2210,31 +2208,23 @@ System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableM bool CASESession::InvokeBackgroundWorkWatchdog() { - bool status = false; + bool wasBlocked = false; if (mSendSigma3Helper && mSendSigma3Helper->UnableToScheduleAfterWorkCallback()) { ChipLogError(SecureChannel, "SendSigma3Helper was unable to schedule the AfterWorkCallback"); - if (mSendSigma3Helper->DoAfterWork() != CHIP_NO_ERROR) - { - SendStatusReport(mExchangeCtxt, kProtocolCodeInvalidParam); - mState = State::kInitialized; - } - status = true; + mSendSigma3Helper->DoAfterWork(); + wasBlocked = true; } if (mHandleSigma3Helper && mHandleSigma3Helper->UnableToScheduleAfterWorkCallback()) { ChipLogError(SecureChannel, "HandleSigma3Helper was unable to schedule the AfterWorkCallback"); - if (mHandleSigma3Helper->DoAfterWork() != CHIP_NO_ERROR) - { - SendStatusReport(mExchangeCtxt, kProtocolCodeInvalidParam); - mState = State::kInitialized; - } - status = true; + mHandleSigma3Helper->DoAfterWork(); + wasBlocked = true; } - return status; + return wasBlocked; } } // namespace chip diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index c12b58a8b7a3e3..7453b6b5002dc4 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -207,8 +207,8 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, State GetState() { return mState; } - // Checks if any of the work helper is unable to schedule the after work callback - // If they are stuck, runs the after work callback from the foreground thread. + // Returns true if the CASE session handshake was stuck due to failing to schedule work on the Matter thread. + // If this function returns true, the CASE session has been reset and is ready for a new session establishment. bool InvokeBackgroundWorkWatchdog(); private: From 7c230414524724726395825378487662b0bbbc2b Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Tue, 27 Jun 2023 11:11:03 -0700 Subject: [PATCH 6/9] Update src/protocols/secure_channel/CASESession.cpp Co-authored-by: Marc Lepage <67919234+mlepage-google@users.noreply.github.com> --- src/protocols/secure_channel/CASESession.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 22beaa5fd6061a..76c2097c6340bb 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -2208,7 +2208,7 @@ System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableM bool CASESession::InvokeBackgroundWorkWatchdog() { - bool wasBlocked = false; + bool watchdogFired = false; if (mSendSigma3Helper && mSendSigma3Helper->UnableToScheduleAfterWorkCallback()) { From 34b82b84d931cf6e9b5e4f56c2b9ba65821630ff Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Wed, 28 Jun 2023 06:37:27 +0530 Subject: [PATCH 7/9] Update src/protocols/secure_channel/CASEServer.cpp Co-authored-by: Marc Lepage <67919234+mlepage-google@users.noreply.github.com> --- src/protocols/secure_channel/CASEServer.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp index c544fbe2c842db..082be87c0a5f0e 100644 --- a/src/protocols/secure_channel/CASEServer.cpp +++ b/src/protocols/secure_channel/CASEServer.cpp @@ -77,9 +77,11 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const { // We are in the middle of CASE handshake - // Check if we are stuck on background thread, if yes, unblock it, and continue - if (GetSession().InvokeBackgroundWorkWatchdog() == false) + // Invoke watchdog to fix any stuck handshakes + bool watchdogFired = GetSession().InvokeBackgroundWorkWatchdog(); + if (!watchdogFired) { + // Handshake wasn't stuck, let it continue its work // TODO: Send Busy response, #27473 ChipLogError(Inet, "CASE session is in establishing state, returning without responding"); return CHIP_NO_ERROR; From 0ffd17970014b961a17b20ad97ee591b5be35e66 Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Wed, 28 Jun 2023 06:41:39 +0530 Subject: [PATCH 8/9] Moved status check before setting atomic variable --- src/protocols/secure_channel/CASESession.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 76c2097c6340bb..3876e023fe70c6 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -242,9 +242,9 @@ class CASESession::WorkHelper { // We failed to schedule after work callback, so setting mScheduleAfterWorkFailed flag to true // This can be checked from foreground thread and after work callback can be retried + helper->mStatus = status; ChipLogError(SecureChannel, "Failed to Schedule the AfterWorkCallback on foreground thread"); helper->mScheduleAfterWorkFailed.store(true); - helper->mStatus = status; // Release strong ptr since scheduling failed helper->mStrongPtr.reset(); From ede7156156dc1b3f93c78136f4131b99c00c0049 Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Wed, 28 Jun 2023 06:47:03 +0530 Subject: [PATCH 9/9] fix build failures --- src/protocols/secure_channel/CASESession.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 3876e023fe70c6..35597cde87241c 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -2214,17 +2214,17 @@ bool CASESession::InvokeBackgroundWorkWatchdog() { ChipLogError(SecureChannel, "SendSigma3Helper was unable to schedule the AfterWorkCallback"); mSendSigma3Helper->DoAfterWork(); - wasBlocked = true; + watchdogFired = true; } if (mHandleSigma3Helper && mHandleSigma3Helper->UnableToScheduleAfterWorkCallback()) { ChipLogError(SecureChannel, "HandleSigma3Helper was unable to schedule the AfterWorkCallback"); mHandleSigma3Helper->DoAfterWork(); - wasBlocked = true; + watchdogFired = true; } - return wasBlocked; + return watchdogFired; } } // namespace chip