From 8226a28353d0f2551662912122589e6dc6cd4948 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 20 Jun 2022 15:16:07 -0400 Subject: [PATCH 1/6] 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 --- src/messaging/ExchangeContext.cpp | 22 +- src/messaging/ExchangeContext.h | 26 ++ src/messaging/ExchangeMgr.cpp | 18 -- src/messaging/ExchangeMgr.h | 5 - src/messaging/ReliableMessageContext.h | 15 -- 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 | 12 +- src/transport/SecureSession.h | 8 +- src/transport/SessionManager.cpp | 6 +- src/transport/SessionManager.h | 4 +- 17 files changed, 306 insertions(+), 168 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..67de9086d63d24 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -326,7 +326,13 @@ ExchangeContext::~ExchangeContext() VerifyOrDie(!IsAckPending()); if (ReleaseSessionOnDestruction() && mSession) - mSession->AsSecureSession()->MarkForEviction(); + { + // Move the session out of the older for the eviction, so it doesn't try + // to notify us in our destructor. + const auto & session = mSession.Get(); + mSession.Release(); + session.Value()->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. @@ -573,5 +579,19 @@ ExchangeMessageDispatch & ExchangeContext::GetMessageDispatch(bool isEphemeralEx return ApplicationExchangeDispatch::Instance(); } +void ExchangeContext::AbortAllOtherCommunicationOnFabric() +{ + if (!mSession || !mSession->IsSecureSession()) + { + return; + } + + GetExchangeMgr()->GetSessionManager()->ReleaseSessionsForFabricExceptOne(mSession->GetFabricIndex(), mSession.Get().Value()); + + mSession->AsSecureSession()->MarkInactive(mSession); + + SetAutoReleaseSession(); +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index b59c68cba17fb1..7131af44252438 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -184,6 +184,17 @@ 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: Timeout mResponseTimeout{ 0 }; // Maximum time to wait for response (in milliseconds); 0 disables response timeout. ExchangeDelegate * mDelegate = nullptr; @@ -274,7 +285,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 SetAutoReleaseSession(); + inline bool ReleaseSessionOnDestruction(); }; +inline void ExchangeContext::SetAutoReleaseSession() +{ + mFlags.Set(Flags::kFlagAutoReleaseSession, true); +} + +inline bool ExchangeContext::ReleaseSessionOnDestruction() +{ + return mFlags.Has(Flags::kFlagAutoReleaseSession); +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index c1d613d4aa39de..0063aa5d22264a 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -376,23 +376,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..c3f6ae33793562 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. @@ -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 8756da23d659ee..7db899ffa7bf6d 100644 --- a/src/transport/SecureSession.cpp +++ b/src/transport/SecureSession.cpp @@ -131,7 +131,7 @@ void SecureSession::MarkForEviction() } } -void SecureSession::MarkInactive() +void SecureSession::MarkInactive(SessionHolder & deferred) { ChipLogDetail(Inet, "SecureSession[%p]: MarkInactive Type:%d LSID:%d", this, to_underlying(mSecureSessionType), mLocalSessionId); @@ -144,12 +144,18 @@ void SecureSession::MarkInactive() case State::kDefunct: FALLTHROUGH; case State::kActive: + // Make sure we don't notify "deferred" that we're being removed. This + // ensures that "deferred" is in fact in our holder list. + RemoveHolder(deferred); // By setting this state, IsActiveSession() will return false, which prevents creating new exchanges. - mState = State::kInactive; + MoveToState(State::kInactive); + NotifySessionReleased(); + // Add the holder back. + AddHolder(deferred); return; case State::kInactive: case State::kPendingEviction: - // Do nothing + VerifyOrDie(false); return; } } diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index d8b798fe6988d3..45c05d02e422e2 100644 --- a/src/transport/SecureSession.h +++ b/src/transport/SecureSession.h @@ -170,8 +170,12 @@ class SecureSession : public Session, public ReferenceCountedGetPeer().GetFabricIndex() == fabricIndex) { - if (session == deferredSecureSession) - session->MarkInactive(); - else + if (session != deferredSecureSession) + { session->MarkForEviction(); + } } return Loop::Continue; }); diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index d50f9f28172058..9a95cd31b89faa 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -175,8 +175,8 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate 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. + // command still goes out on the exchange the command came in on. This API + // relies on the caller to handle releasing the "deferred" session. void ReleaseSessionsForFabricExceptOne(FabricIndex fabricIndex, const SessionHandle & deferred); /** From 89c4bac4b78abd224b16b7b48b2014fca6525e06 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 21 Jun 2022 09:36:03 -0400 Subject: [PATCH 2/6] Address review comments. --- src/messaging/ExchangeContext.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 67de9086d63d24..de89d937d1e376 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -327,9 +327,9 @@ ExchangeContext::~ExchangeContext() if (ReleaseSessionOnDestruction() && mSession) { - // Move the session out of the older for the eviction, so it doesn't try + // Move the session out of the holder for the eviction, so it doesn't try // to notify us in our destructor. - const auto & session = mSession.Get(); + Optional session = mSession.Get(); mSession.Release(); session.Value()->AsSecureSession()->MarkForEviction(); } @@ -583,6 +583,7 @@ void ExchangeContext::AbortAllOtherCommunicationOnFabric() { if (!mSession || !mSession->IsSecureSession()) { + ChipLogError(ExchangeManager, "AbortAllOtherCommunicationOnFabric called on a non-PASE/CASE session"); return; } From ca1e4391ff6c383d1ce501409d956b7af9d57b13 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 21 Jun 2022 11:33:13 -0400 Subject: [PATCH 3/6] 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. --- src/messaging/ExchangeMgr.cpp | 9 ++++++++- src/transport/SecureSession.cpp | 11 +++++++++-- src/transport/SecureSession.h | 1 + src/transport/SessionManager.cpp | 2 +- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 0063aa5d22264a..37901ae7951d04 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 a + // 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. diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp index 7db899ffa7bf6d..106ce1a6cba5de 100644 --- a/src/transport/SecureSession.cpp +++ b/src/transport/SecureSession.cpp @@ -46,6 +46,10 @@ const char * SecureSession::StateToString(State state) const return "kPendingEviction"; break; + case State::kInactive: + return "kInactive"; + break; + default: return "???"; break; @@ -89,9 +93,12 @@ void SecureSession::MarkAsDefunct() case State::kInactive: // - // Once a session is marked Inactive, we CANNOT bring it back to either being active or defunct. + // Once a session is marked Inactive, we CANNOT bring it back to either + // being active or defunct. But consumers may not really know this + // session is already inactive. Just ignore the call and stay in + // kInactive state. // - FALLTHROUGH; + return; case State::kPendingEviction: // // Once a session is headed for eviction, we CANNOT bring it back to either being active or defunct. diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index 45c05d02e422e2..3868a0063cd28f 100644 --- a/src/transport/SecureSession.h +++ b/src/transport/SecureSession.h @@ -144,6 +144,7 @@ class SecureSession : public Session, public ReferenceCountedAsSecureSession(); - if (!secureSession->IsDefunct() && !secureSession->IsActiveSession()) + if (!secureSession->IsDefunct() && !secureSession->IsActiveSession() && !secureSession->IsInactive()) { ChipLogError(Inet, "Secure transport received message on a session in an invalid state (state = '%s')", secureSession->GetStateStr()); From a4bbb6460890f39cfff2c7817f913af4b7375669 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 21 Jun 2022 11:36:20 -0400 Subject: [PATCH 4/6] Address review comment --- src/transport/SecureSession.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index 3868a0063cd28f..d844ce621bf005 100644 --- a/src/transport/SecureSession.h +++ b/src/transport/SecureSession.h @@ -173,7 +173,7 @@ class SecureSession : public Session, public ReferenceCounted Date: Tue, 21 Jun 2022 12:20:15 -0400 Subject: [PATCH 5/6] Address review comments --- src/messaging/ExchangeContext.cpp | 2 +- src/messaging/ExchangeMgr.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index de89d937d1e376..e85f6fc91d00ac 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -583,7 +583,7 @@ void ExchangeContext::AbortAllOtherCommunicationOnFabric() { if (!mSession || !mSession->IsSecureSession()) { - ChipLogError(ExchangeManager, "AbortAllOtherCommunicationOnFabric called on a non-PASE/CASE session"); + ChipLogError(ExchangeManager, "AbortAllOtherCommunicationOnFabric called when we don't have a PASE/CASE session"); return; } diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 37901ae7951d04..fcdcfeeecd5058 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -236,7 +236,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const packetHeader.GetDestinationGroupId().Value()); } - // Do not handle messages that don't match an existing exchange on a + // 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()) { From 9a73bfc74db68377f0bd468a772e3b84736a91ef Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 22 Jun 2022 12:57:23 -0400 Subject: [PATCH 6/6] Address Jerry's review comments. --- src/messaging/ExchangeContext.cpp | 31 ++++++++++++------- src/messaging/ExchangeContext.h | 23 +++++++++----- src/messaging/ReliableMessageContext.h | 4 +-- src/transport/SecureSession.cpp | 43 -------------------------- src/transport/SecureSession.h | 15 --------- src/transport/SessionHolder.cpp | 11 +++++-- src/transport/SessionHolder.h | 5 ++- src/transport/SessionManager.cpp | 25 +++++---------- src/transport/SessionManager.h | 5 --- 9 files changed, 55 insertions(+), 107 deletions(-) diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index e85f6fc91d00ac..1e66101304a611 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -325,15 +325,6 @@ ExchangeContext::~ExchangeContext() VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0); VerifyOrDie(!IsAckPending()); - if (ReleaseSessionOnDestruction() && mSession) - { - // Move the session out of the holder for the eviction, so it doesn't try - // to notify us in our destructor. - Optional session = mSession.Get(); - mSession.Release(); - session.Value()->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); @@ -377,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 @@ -587,11 +583,22 @@ void ExchangeContext::AbortAllOtherCommunicationOnFabric() return; } - GetExchangeMgr()->GetSessionManager()->ReleaseSessionsForFabricExceptOne(mSession->GetFabricIndex(), mSession.Get().Value()); + // Save our session so it does not actually go away. + Optional session = mSession.Get(); + + SetIgnoreSessionRelease(true); + + GetExchangeMgr()->GetSessionManager()->ExpireAllPairingsForFabric(mSession->GetFabricIndex()); - mSession->AsSecureSession()->MarkInactive(mSession); + mSession.GrabExpiredSession(session.Value()); - SetAutoReleaseSession(); + SetIgnoreSessionRelease(false); +} + +void ExchangeContext::ExchangeSessionHolder::GrabExpiredSession(const SessionHandle & session) +{ + VerifyOrDie(session->AsSecureSession()->IsPendingEviction()); + GrabUnchecked(session); } } // namespace Messaging diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index 7131af44252438..c52ba48630a726 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -196,14 +196,21 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, 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 @@ -288,18 +295,18 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, // 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 SetAutoReleaseSession(); - inline bool ReleaseSessionOnDestruction(); + inline void SetIgnoreSessionRelease(bool ignore); + inline bool ShouldIgnoreSessionRelease(); }; -inline void ExchangeContext::SetAutoReleaseSession() +inline void ExchangeContext::SetIgnoreSessionRelease(bool ignore) { - mFlags.Set(Flags::kFlagAutoReleaseSession, true); + mFlags.Set(Flags::kFlagIgnoreSessionRelease, ignore); } -inline bool ExchangeContext::ReleaseSessionOnDestruction() +inline bool ExchangeContext::ShouldIgnoreSessionRelease() { - return mFlags.Has(Flags::kFlagAutoReleaseSession); + return mFlags.Has(Flags::kFlagIgnoreSessionRelease); } } // namespace Messaging diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index c3f6ae33793562..886ed5c3ba341b 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -165,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 diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp index 106ce1a6cba5de..5683a24e8265e9 100644 --- a/src/transport/SecureSession.cpp +++ b/src/transport/SecureSession.cpp @@ -46,10 +46,6 @@ const char * SecureSession::StateToString(State state) const return "kPendingEviction"; break; - case State::kInactive: - return "kInactive"; - break; - default: return "???"; break; @@ -91,14 +87,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. But consumers may not really know this - // session is already inactive. Just ignore the call and stay in - // kInactive state. - // - return; case State::kPendingEviction: // // Once a session is headed for eviction, we CANNOT bring it back to either being active or defunct. @@ -125,8 +113,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(); @@ -138,35 +124,6 @@ void SecureSession::MarkForEviction() } } -void SecureSession::MarkInactive(SessionHolder & deferred) -{ - 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: - // Make sure we don't notify "deferred" that we're being removed. This - // ensures that "deferred" is in fact in our holder list. - RemoveHolder(deferred); - // By setting this state, IsActiveSession() will return false, which prevents creating new exchanges. - MoveToState(State::kInactive); - NotifySessionReleased(); - // Add the holder back. - AddHolder(deferred); - return; - case State::kInactive: - case State::kPendingEviction: - VerifyOrDie(false); - return; - } -} - Access::SubjectDescriptor SecureSession::GetSubjectDescriptor() const { Access::SubjectDescriptor subjectDescriptor; diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index d844ce621bf005..b611791e351ada 100644 --- a/src/transport/SecureSession.h +++ b/src/transport/SecureSession.h @@ -144,7 +144,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 a9510a4a84fcb9..d4cbb955c938ca 100644 --- a/src/transport/SessionHolder.h +++ b/src/transport/SessionHolder.h @@ -69,7 +69,10 @@ class SessionHolder : public SessionDelegate, 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 e1a3de91fbd723..b8bf6be68eaa00 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->MarkForEviction(); - } - } - return Loop::Continue; - }); -} - Optional SessionManager::AllocateSession(SecureSession::Type secureSessionType, const ScopedNodeId & sessionEvictionHint) { @@ -532,7 +515,13 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & packetHea Transport::SecureSession * secureSession = session.Value()->AsSecureSession(); - if (!secureSession->IsDefunct() && !secureSession->IsActiveSession() && !secureSession->IsInactive()) + // 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 9a95cd31b89faa..77255db8112db1 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -174,11 +174,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 - // relies on the caller to handle releasing the "deferred" session. - void ReleaseSessionsForFabricExceptOne(FabricIndex fabricIndex, const SessionHandle & deferred); - /** * @brief * Return the System Layer pointer used by current SessionManager.