From 52762515f58587a1a763809b87ab08dc960b5a7f Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 22 Jun 2022 19:19:30 -0400 Subject: [PATCH] Fix crashes when closing all exchanges for fabric. (#19780) * Fix crashes when closing all exchanges for fabric. Our "close all exchanges except this special one for this fabric" API messes up exchange refcounting, leading to use-after-free. The fix is to reuse, as much as possible, the normal "session is going away" flow to notify exchanges, and other session consumers, that the sessions are in fact going away. Fixes https://github.com/project-chip/connectedhomeip/issues/19747 * Address review comments. * Updates to fix fallout from #19502. We need to allow messages on inactive sessions to reach the exchange manager, because such sessions need to be able to deliver an MRP ack to an exchange waiting for one. We also don't want to crash on an attempt to transition from Inactive to Defunct state; the transition should just be ignored. This way if we start trying to transitionin to Defunct on MRP delivery failures we will not start crashing if such a failure happens on an Inactive session. * Address review comment * Address review comments * Address Jerry's review comments. --- src/messaging/ExchangeContext.cpp | 34 ++- src/messaging/ExchangeContext.h | 37 ++- src/messaging/ExchangeMgr.cpp | 27 +- src/messaging/ExchangeMgr.h | 5 - src/messaging/ReliableMessageContext.h | 19 +- src/messaging/tests/BUILD.gn | 11 +- src/messaging/tests/MessagingContext.h | 2 - .../tests/TestAbortExchangesForFabric.cpp | 235 ++++++++++++++++++ src/messaging/tests/TestExchangeMgr.cpp | 2 - src/messaging/tests/TestExchangeMgrDriver.cpp | 35 --- src/messaging/tests/TestMessagingLayer.h | 36 --- .../tests/TestReliableMessageProtocol.cpp | 2 - .../TestReliableMessageProtocolDriver.cpp | 35 --- src/transport/SecureSession.cpp | 30 --- src/transport/SecureSession.h | 10 - src/transport/SessionHolder.cpp | 11 +- src/transport/SessionHolder.h | 5 +- src/transport/SessionManager.cpp | 25 +- src/transport/SessionManager.h | 5 - 19 files changed, 334 insertions(+), 232 deletions(-) create mode 100644 src/messaging/tests/TestAbortExchangesForFabric.cpp delete mode 100644 src/messaging/tests/TestExchangeMgrDriver.cpp delete mode 100644 src/messaging/tests/TestMessagingLayer.h delete mode 100644 src/messaging/tests/TestReliableMessageProtocolDriver.cpp diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index b3bc08899ebfbb..1e66101304a611 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -325,9 +325,6 @@ ExchangeContext::~ExchangeContext() VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0); VerifyOrDie(!IsAckPending()); - if (ReleaseSessionOnDestruction() && mSession) - mSession->AsSecureSession()->MarkForEviction(); - #if CONFIG_DEVICE_LAYER && CHIP_DEVICE_CONFIG_ENABLE_SED // Make sure that the exchange withdraws the request for Sleepy End Device active mode. UpdateSEDIntervalMode(false); @@ -371,6 +368,11 @@ bool ExchangeContext::MatchExchange(const SessionHandle & session, const PacketH void ExchangeContext::OnSessionReleased() { + if (ShouldIgnoreSessionRelease()) + { + return; + } + if (mFlags.Has(Flags::kFlagClosed)) { // Exchange is already being closed. It may occur when closing an exchange after sending @@ -573,5 +575,31 @@ ExchangeMessageDispatch & ExchangeContext::GetMessageDispatch(bool isEphemeralEx return ApplicationExchangeDispatch::Instance(); } +void ExchangeContext::AbortAllOtherCommunicationOnFabric() +{ + if (!mSession || !mSession->IsSecureSession()) + { + ChipLogError(ExchangeManager, "AbortAllOtherCommunicationOnFabric called when we don't have a PASE/CASE session"); + return; + } + + // Save our session so it does not actually go away. + Optional session = mSession.Get(); + + SetIgnoreSessionRelease(true); + + GetExchangeMgr()->GetSessionManager()->ExpireAllPairingsForFabric(mSession->GetFabricIndex()); + + mSession.GrabExpiredSession(session.Value()); + + SetIgnoreSessionRelease(false); +} + +void ExchangeContext::ExchangeSessionHolder::GrabExpiredSession(const SessionHandle & session) +{ + VerifyOrDie(session->AsSecureSession()->IsPendingEviction()); + GrabUnchecked(session); +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index b59c68cba17fb1..c52ba48630a726 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -184,15 +184,33 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, // using this function is recommended. void SetResponseTimeout(Timeout timeout); + // This API is used by commands that need to shut down all existing + // sessions/exchanges on a fabric but need to make sure the response to the + // command still goes out on the exchange the command came in on. This API + // will ensure that all secure sessions for the fabric this exchanges is on + // are released except the one this exchange is using, and will release + // that session once this exchange is done sending the response. + // + // This API is a no-op if called on an exchange that is not using a + // SecureSession. + void AbortAllOtherCommunicationOnFabric(); + private: + class ExchangeSessionHolder : public SessionHolderWithDelegate + { + public: + ExchangeSessionHolder(ExchangeContext & exchange) : SessionHolderWithDelegate(exchange) {} + void GrabExpiredSession(const SessionHandle & session); + }; + Timeout mResponseTimeout{ 0 }; // Maximum time to wait for response (in milliseconds); 0 disables response timeout. ExchangeDelegate * mDelegate = nullptr; ExchangeManager * mExchangeMgr = nullptr; ExchangeMessageDispatch & mDispatch; - SessionHolderWithDelegate mSession; // The connection state - uint16_t mExchangeId; // Assigned exchange ID. + ExchangeSessionHolder mSession; // The connection state + uint16_t mExchangeId; // Assigned exchange ID. /** * Determine whether a response is currently expected for a message that was sent over @@ -274,7 +292,22 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, void UpdateSEDIntervalMode(bool activeMode); static ExchangeMessageDispatch & GetMessageDispatch(bool isEphemeralExchange, ExchangeDelegate * delegate); + + // If SetAutoReleaseSession() is called, this exchange must be using a SecureSession, and should + // evict it when the exchange is done with all its work (including any MRP traffic). + inline void SetIgnoreSessionRelease(bool ignore); + inline bool ShouldIgnoreSessionRelease(); }; +inline void ExchangeContext::SetIgnoreSessionRelease(bool ignore) +{ + mFlags.Set(Flags::kFlagIgnoreSessionRelease, ignore); +} + +inline bool ExchangeContext::ShouldIgnoreSessionRelease() +{ + return mFlags.Has(Flags::kFlagIgnoreSessionRelease); +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index c1d613d4aa39de..fcdcfeeecd5058 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -236,7 +236,14 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const packetHeader.GetDestinationGroupId().Value()); } - // Do not handle unsolicited messages on a inactive session. + // Do not handle messages that don't match an existing exchange on an + // inactive session, since we should not be creating new exchanges there. + if (!session->IsActiveSession()) + { + ChipLogProgress(ExchangeManager, "Dropping message on inactive session that does not match an existing exchange"); + return; + } + // If it's not a duplicate message, search for an unsolicited message handler if it is marked as being sent by an initiator. // Since we didn't find an existing exchange that matches the message, it must be an unsolicited message. However all // unsolicited messages must be marked as being from an initiator. @@ -376,23 +383,5 @@ void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * deleg }); } -void ExchangeManager::AbortExchangesForFabricExceptOne(FabricIndex fabricIndex, ExchangeContext * deferred) -{ - VerifyOrDie(deferred->HasSessionHandle() && deferred->GetSessionHandle()->IsSecureSession()); - - mContextPool.ForEachActiveObject([&](auto * ec) { - if (ec->HasSessionHandle() && ec->GetSessionHandle()->GetFabricIndex() == fabricIndex) - { - if (ec == deferred) - ec->SetAutoReleaseSession(); - else - ec->Abort(); - } - return Loop::Continue; - }); - - mSessionManager->ReleaseSessionsForFabricExceptOne(fabricIndex, deferred->GetSessionHandle()); -} - } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index 112cddd82c7ecb..d6b775436799f4 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -183,11 +183,6 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate */ void CloseAllContextsForDelegate(const ExchangeDelegate * delegate); - // This API is used by commands that need to shut down all existing exchanges on a fabric but need to make sure the response to - // the command still goes out on the exchange the command came in on. This API flags that one exchange to shut down its session - // when it's done. - void AbortExchangesForFabricExceptOne(FabricIndex fabricIndex, ExchangeContext * deferred); - SessionManager * GetSessionManager() const { return mSessionManager; } ReliableMessageMgr * GetReliableMessageMgr() { return &mReliableMessageMgr; }; diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index 9b2f8d67ab0539..886ed5c3ba341b 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -124,11 +124,6 @@ class ReliableMessageContext /// Determine whether this exchange is a EphemeralExchange for replying a StandaloneAck bool IsEphemeralExchange() const; - // If SetAutoReleaseSession() is called, this exchange must be using a SecureSession, and should - // evict it when the exchange is done with all its work (including any MRP traffic). - void SetAutoReleaseSession(); - bool ReleaseSessionOnDestruction(); - /** * Get the reliable message manager that corresponds to this reliable * message context. @@ -170,8 +165,8 @@ class ReliableMessageContext /// When set, signifies that the exchange created sorely for replying a StandaloneAck kFlagEphemeralExchange = (1u << 9), - /// When set, automatically release the session when this exchange is destroyed. - kFlagAutoReleaseSession = (1u << 10), + /// When set, ignore session being released, because we are releasing it ourselves. + kFlagIgnoreSessionRelease = (1u << 10), }; BitFlags mFlags; // Internal state flags @@ -253,15 +248,5 @@ inline bool ReliableMessageContext::IsEphemeralExchange() const return mFlags.Has(Flags::kFlagEphemeralExchange); } -inline void ReliableMessageContext::SetAutoReleaseSession() -{ - mFlags.Set(Flags::kFlagAutoReleaseSession, true); -} - -inline bool ReliableMessageContext::ReleaseSessionOnDestruction() -{ - return mFlags.Has(Flags::kFlagAutoReleaseSession); -} - } // namespace Messaging } // namespace chip diff --git a/src/messaging/tests/BUILD.gn b/src/messaging/tests/BUILD.gn index 10f34fca6d2f45..4b7d0a2585ec2e 100644 --- a/src/messaging/tests/BUILD.gn +++ b/src/messaging/tests/BUILD.gn @@ -43,11 +43,13 @@ static_library("helpers") { chip_test_suite("tests") { output_name = "libMessagingLayerTests" - sources = [ "TestMessagingLayer.h" ] + test_sources = [] if (chip_device_platform != "efr32") { # TODO(#10447): ReliableMessage Test has HF, and ExchangeMgr hangs on EFR32. - sources += [ + # And TestAbortExchangesForFabric does not link on EFR32 for some reason. + test_sources += [ + "TestAbortExchangesForFabric.cpp", "TestExchangeMgr.cpp", "TestReliableMessageProtocol.cpp", ] @@ -67,9 +69,4 @@ chip_test_suite("tests") { "${nlio_root}:nlio", "${nlunit_test_root}:nlunit-test", ] - - tests = [ - "TestExchangeMgr", - "TestReliableMessageProtocol", - ] } diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h index a759364de331e5..5ef51e1da99c6a 100644 --- a/src/messaging/tests/MessagingContext.h +++ b/src/messaging/tests/MessagingContext.h @@ -103,8 +103,6 @@ class MessagingContext : public PlatformMemoryUser static const uint16_t kAliceKeyId = 2; static const uint16_t kCharlieKeyId = 3; static const uint16_t kDavidKeyId = 4; - NodeId GetBobNodeId() const; - NodeId GetAliceNodeId() const; GroupId GetFriendsGroupId() const { return mFriendsGroupId; } SessionManager & GetSecureSessionManager() { return mSessionManager; } diff --git a/src/messaging/tests/TestAbortExchangesForFabric.cpp b/src/messaging/tests/TestAbortExchangesForFabric.cpp new file mode 100644 index 00000000000000..c83342b35e1fe2 --- /dev/null +++ b/src/messaging/tests/TestAbortExchangesForFabric.cpp @@ -0,0 +1,235 @@ +/* + * Copyright (c) 2022 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * This file implements unit tests for aborting existing exchanges (except + * one) for a fabric. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +using namespace chip; +using namespace chip::Messaging; +using namespace chip::System; +using namespace chip::Protocols; + +using TestContext = Test::LoopbackMessagingContext; + +class MockAppDelegate : public ExchangeDelegate +{ +public: + CHIP_ERROR OnMessageReceived(ExchangeContext * ec, const PayloadHeader & payloadHeader, + System::PacketBufferHandle && buffer) override + { + mOnMessageReceivedCalled = true; + return CHIP_NO_ERROR; + } + + void OnResponseTimeout(ExchangeContext * ec) override {} + + bool mOnMessageReceivedCalled = false; +}; + +void CheckAbortAllButOneExchange(nlTestSuite * inSuite, void * inContext) +{ + TestContext & ctx = *reinterpret_cast(inContext); + + // We want to have two sessions using the same fabric id that we use for + // creating our exchange contexts. That lets us test exchanges on the same + // session as the "special exchange" as well as on other sessions. + auto & sessionManager = ctx.GetSecureSessionManager(); + + // Use key ids that are not going to collide with anything else that ctx is + // doing. + // TODO: These should really be CASE sessions... + SessionHolder session1; + CHIP_ERROR err = + sessionManager.InjectPaseSessionWithTestKey(session1, 100, ctx.GetBobFabric()->GetNodeId(), 101, ctx.GetAliceFabricIndex(), + ctx.GetBobAddress(), CryptoContext::SessionRole::kInitiator); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + SessionHolder session1Reply; + err = sessionManager.InjectPaseSessionWithTestKey(session1Reply, 101, ctx.GetAliceFabric()->GetNodeId(), 100, + ctx.GetBobFabricIndex(), ctx.GetAliceAddress(), + CryptoContext::SessionRole::kResponder); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // TODO: Ideally this would go to a different peer, but we don't have that + // set up right now: only Alice and Bob have useful node ids and whatnot. + SessionHolder session2; + err = + sessionManager.InjectPaseSessionWithTestKey(session2, 200, ctx.GetBobFabric()->GetNodeId(), 201, ctx.GetAliceFabricIndex(), + ctx.GetBobAddress(), CryptoContext::SessionRole::kInitiator); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + SessionHolder session2Reply; + err = sessionManager.InjectPaseSessionWithTestKey(session2Reply, 201, ctx.GetAliceFabric()->GetNodeId(), 200, + ctx.GetBobFabricIndex(), ctx.GetAliceAddress(), + CryptoContext::SessionRole::kResponder); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + auto & exchangeMgr = ctx.GetExchangeManager(); + + MockAppDelegate delegate; + Echo::EchoServer server; + err = server.Init(&exchangeMgr); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + auto & loopback = ctx.GetLoopback(); + + auto trySendMessage = [&](ExchangeContext * exchange, SendMessageFlags flags) { + PacketBufferHandle buffer = MessagePacketBuffer::New(0); + NL_TEST_ASSERT(inSuite, !buffer.IsNull()); + return exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer), flags); + }; + + auto sendAndDropMessage = [&](ExchangeContext * exchange, SendMessageFlags flags) { + // Send a message on the given exchange with the given flags, make sure + // it's not delivered. + loopback.mNumMessagesToDrop = 1; + loopback.mDroppedMessageCount = 0; + + err = trySendMessage(exchange, flags); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(inSuite, !delegate.mOnMessageReceivedCalled); + NL_TEST_ASSERT(inSuite, loopback.mDroppedMessageCount == 1); + }; + + ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + + // We want to test three possible exchange states: + // 1) Closed but waiting for ack. + // 2) Waiting for a response. + // 3) Waiting for a send. + auto * waitingForAck1 = exchangeMgr.NewContext(session1.Get().Value(), &delegate); + NL_TEST_ASSERT(inSuite, waitingForAck1 != nullptr); + sendAndDropMessage(waitingForAck1, SendMessageFlags::kNone); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); + + auto * waitingForAck2 = exchangeMgr.NewContext(session2.Get().Value(), &delegate); + NL_TEST_ASSERT(inSuite, waitingForAck2 != nullptr); + sendAndDropMessage(waitingForAck2, SendMessageFlags::kNone); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 2); + + auto * waitingForIncomingMessage1 = exchangeMgr.NewContext(session1.Get().Value(), &delegate); + NL_TEST_ASSERT(inSuite, waitingForIncomingMessage1 != nullptr); + sendAndDropMessage(waitingForIncomingMessage1, SendMessageFlags::kExpectResponse); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 3); + + auto * waitingForIncomingMessage2 = exchangeMgr.NewContext(session2.Get().Value(), &delegate); + NL_TEST_ASSERT(inSuite, waitingForIncomingMessage2 != nullptr); + sendAndDropMessage(waitingForIncomingMessage2, SendMessageFlags::kExpectResponse); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 4); + + auto * waitingForSend1 = exchangeMgr.NewContext(session1.Get().Value(), &delegate); + NL_TEST_ASSERT(inSuite, waitingForSend1 != nullptr); + waitingForSend1->WillSendMessage(); + + auto * waitingForSend2 = exchangeMgr.NewContext(session2.Get().Value(), &delegate); + NL_TEST_ASSERT(inSuite, waitingForSend2 != nullptr); + waitingForSend2->WillSendMessage(); + + // Grab handles to our sessions now, before we evict things. + const auto & sessionHandle1 = session1.Get(); + const auto & sessionHandle2 = session2.Get(); + + NL_TEST_ASSERT(inSuite, session1); + NL_TEST_ASSERT(inSuite, session2); + auto * specialExhange = exchangeMgr.NewContext(session1.Get().Value(), &delegate); + specialExhange->AbortAllOtherCommunicationOnFabric(); + + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); + NL_TEST_ASSERT(inSuite, !session1); + NL_TEST_ASSERT(inSuite, !session2); + + NL_TEST_ASSERT(inSuite, exchangeMgr.NewContext(sessionHandle1.Value(), &delegate) == nullptr); + NL_TEST_ASSERT(inSuite, exchangeMgr.NewContext(sessionHandle2.Value(), &delegate) == nullptr); + + // Make sure we can't send messages on any of the other exchanges. + NL_TEST_ASSERT(inSuite, trySendMessage(waitingForSend1, SendMessageFlags::kExpectResponse) != CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, trySendMessage(waitingForSend2, SendMessageFlags::kExpectResponse) != CHIP_NO_ERROR); + + // Make sure we can send a message on the special exchange. + NL_TEST_ASSERT(inSuite, !delegate.mOnMessageReceivedCalled); + err = trySendMessage(specialExhange, SendMessageFlags::kNone); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + // Should be waiting for an ack now. + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); + + ctx.DrainAndServiceIO(); + + // Should not get an app-level response, since we are not expecting one. + NL_TEST_ASSERT(inSuite, !delegate.mOnMessageReceivedCalled); + // We should have gotten our ack. + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); + + waitingForSend1->Close(); + waitingForSend2->Close(); +} + +// Test Suite + +/** + * Test Suite that lists all the test functions. + */ +// clang-format off +const nlTest sTests[] = +{ + NL_TEST_DEF("Test aborting all but one exchange", CheckAbortAllButOneExchange), + + NL_TEST_SENTINEL() +}; +// clang-format on + +// clang-format off +nlTestSuite sSuite = +{ + "Test-AbortExchangesForFabric", + &sTests[0], + TestContext::Initialize, + TestContext::Finalize +}; +// clang-format on + +} // anonymous namespace + +/** + * Main + */ +int TestAbortExchangesForFabric() +{ + TestContext sContext; + + // Run test suit against one context + nlTestRunner(&sSuite, &sContext); + + return (nlTestRunnerStats(&sSuite)); +} + +CHIP_REGISTER_TEST_SUITE(TestAbortExchangesForFabric); diff --git a/src/messaging/tests/TestExchangeMgr.cpp b/src/messaging/tests/TestExchangeMgr.cpp index 52ecc256ecca94..05aa6cddbd63e4 100644 --- a/src/messaging/tests/TestExchangeMgr.cpp +++ b/src/messaging/tests/TestExchangeMgr.cpp @@ -21,8 +21,6 @@ * This file implements unit tests for the ExchangeManager implementation. */ -#include "TestMessagingLayer.h" - #include #include #include diff --git a/src/messaging/tests/TestExchangeMgrDriver.cpp b/src/messaging/tests/TestExchangeMgrDriver.cpp deleted file mode 100644 index c4699e8d27bb50..00000000000000 --- a/src/messaging/tests/TestExchangeMgrDriver.cpp +++ /dev/null @@ -1,35 +0,0 @@ -/* - * - * Copyright (c) 2020 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/** - * @file - * This file implements a standalone/native program executable - * test driver for the CHIP core library CHIP ExchangeManager tests. - * - */ - -#include "TestMessagingLayer.h" - -#include - -int main() -{ - // Generate machine-readable, comma-separated value (CSV) output. - nlTestSetOutputStyle(OUTPUT_CSV); - - return (TestExchangeMgr()); -} diff --git a/src/messaging/tests/TestMessagingLayer.h b/src/messaging/tests/TestMessagingLayer.h deleted file mode 100644 index 7bdcef314c4c0b..00000000000000 --- a/src/messaging/tests/TestMessagingLayer.h +++ /dev/null @@ -1,36 +0,0 @@ -/* - * - * Copyright (c) 2020 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/** - * @file - * This file declares test entry points for CHIP Messaging layer - * layer library unit tests. - * - */ - -#pragma once - -#ifdef __cplusplus -extern "C" { -#endif - -int TestExchangeMgr(void); -int TestReliableMessageProtocol(void); - -#ifdef __cplusplus -} -#endif diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 4ddf2c63919759..a0c204c919366e 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -22,8 +22,6 @@ * implementation. */ -#include "TestMessagingLayer.h" - #include #include #include diff --git a/src/messaging/tests/TestReliableMessageProtocolDriver.cpp b/src/messaging/tests/TestReliableMessageProtocolDriver.cpp deleted file mode 100644 index b7cac4537e3f62..00000000000000 --- a/src/messaging/tests/TestReliableMessageProtocolDriver.cpp +++ /dev/null @@ -1,35 +0,0 @@ -/* - * - * Copyright (c) 2020 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/** - * @file - * This file implements a standalone/native program executable test driver - * for the CHIP core library CHIP ReliableMessageProtocol tests. - * - */ - -#include "TestMessagingLayer.h" - -#include - -int main() -{ - // Generate machine-readable, comma-separated value (CSV) output. - nlTestSetOutputStyle(OUTPUT_CSV); - - return (TestReliableMessageProtocol()); -} diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp index 5af68d5fd35504..adb1db94872033 100644 --- a/src/transport/SecureSession.cpp +++ b/src/transport/SecureSession.cpp @@ -117,11 +117,6 @@ void SecureSession::MarkAsDefunct() // return; - case State::kInactive: - // - // Once a session is marked Inactive, we CANNOT bring it back to either being active or defunct. - // - FALLTHROUGH; case State::kPendingEviction: // // Once a session is headed for eviction, we CANNOT bring it back to either being active or defunct. @@ -148,8 +143,6 @@ void SecureSession::MarkForEviction() case State::kDefunct: FALLTHROUGH; case State::kActive: - FALLTHROUGH; - case State::kInactive: Release(); // Decrease the ref which is retained at Activate MoveToState(State::kPendingEviction); NotifySessionReleased(); @@ -161,29 +154,6 @@ void SecureSession::MarkForEviction() } } -void SecureSession::MarkInactive() -{ - ChipLogDetail(Inet, "SecureSession[%p]: MarkInactive Type:%d LSID:%d", this, to_underlying(mSecureSessionType), - mLocalSessionId); - ReferenceCountedHandle ref(*this); - switch (mState) - { - case State::kEstablishing: - VerifyOrDie(false); - return; - case State::kDefunct: - FALLTHROUGH; - case State::kActive: - // By setting this state, IsActiveSession() will return false, which prevents creating new exchanges. - mState = State::kInactive; - return; - case State::kInactive: - case State::kPendingEviction: - // Do nothing - return; - } -} - Access::SubjectDescriptor SecureSession::GetSubjectDescriptor() const { Access::SubjectDescriptor subjectDescriptor; diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index 6c68e9bc72211a..8f026ad83009a6 100644 --- a/src/transport/SecureSession.h +++ b/src/transport/SecureSession.h @@ -147,9 +147,6 @@ class SecureSession : public Session, public ReferenceCountedAsSecureSession()->IsEstablishing()) return false; - mSession.Emplace(session.mSession); - session->AddHolder(*this); + GrabUnchecked(session); return true; } @@ -96,9 +95,15 @@ bool SessionHolder::Grab(const SessionHandle & session) if (!session->IsActiveSession()) return false; + GrabUnchecked(session); + return true; +} + +void SessionHolder::GrabUnchecked(const SessionHandle & session) +{ + VerifyOrDie(!mSession.HasValue()); mSession.Emplace(session.mSession); session->AddHolder(*this); - return true; } void SessionHolder::Release() diff --git a/src/transport/SessionHolder.h b/src/transport/SessionHolder.h index 2886987685940f..8d830a267d763d 100644 --- a/src/transport/SessionHolder.h +++ b/src/transport/SessionHolder.h @@ -73,7 +73,10 @@ class SessionHolder : public IntrusiveListNodeBase<> // There is not delegate, nothing to do here virtual void DispatchSessionEvent(SessionDelegate::Event event) {} -private: +protected: + // Helper for use by the Grab methods. + void GrabUnchecked(const SessionHandle & session); + Optional> mSession; }; diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index 5be749e54393f8..db37077fa3869a 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -368,23 +368,6 @@ void SessionManager::ExpireAllPASEPairings() }); } -void SessionManager::ReleaseSessionsForFabricExceptOne(FabricIndex fabricIndex, const SessionHandle & deferred) -{ - VerifyOrDie(deferred->IsSecureSession()); - SecureSession * deferredSecureSession = deferred->AsSecureSession(); - - mSecureSessions.ForEachSession([&](auto session) { - if (session->GetPeer().GetFabricIndex() == fabricIndex) - { - if (session == deferredSecureSession) - session->MarkInactive(); - else - session->MarkForEviction(); - } - return Loop::Continue; - }); -} - Optional SessionManager::AllocateSession(SecureSession::Type secureSessionType, const ScopedNodeId & sessionEvictionHint) { @@ -553,7 +536,13 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & packetHea Transport::SecureSession * secureSession = session.Value()->AsSecureSession(); - if (!secureSession->IsDefunct() && !secureSession->IsActiveSession()) + // We need to allow through messages even on sessions that are pending + // evictions, because for some cases (UpdateNOC, RemoveFabric, etc) there + // can be a single exchange alive on the session waiting for a MRP ack, and + // we need to make sure to send the ack through. The exchange manager is + // responsible for ensuring that such messages do not lead to new exchange + // creation. + if (!secureSession->IsDefunct() && !secureSession->IsActiveSession() && !secureSession->IsPendingEviction()) { ChipLogError(Inet, "Secure transport received message on a session in an invalid state (state = '%s')", secureSession->GetStateStr()); diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 13e06fae8797d7..219fd4eb20c069 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -178,11 +178,6 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate void ExpireAllPairingsForFabric(FabricIndex fabric); void ExpireAllPASEPairings(); - // This API is used by commands that need to release all existing sessions on a fabric but need to make sure the response to the - // command still goes out on the exchange the command came in on. This API flags that the release of the session used by the - // exchange is deferred until the exchange is done. - void ReleaseSessionsForFabricExceptOne(FabricIndex fabricIndex, const SessionHandle & deferred); - /** * @brief * Return the System Layer pointer used by current SessionManager.