diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp index 7d248901680c14..0d84e4a3aab876 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalDeviceProxy.cpp @@ -317,12 +317,21 @@ void OperationalDeviceProxy::OnSessionEstablished(const SessionHandle & session) CHIP_ERROR OperationalDeviceProxy::Disconnect() { ReturnErrorCodeIf(mState != State::SecureConnected, CHIP_ERROR_INCORRECT_STATE); + if (mSecureSession) { - mInitParams.sessionManager->ExpirePairing(mSecureSession.Get().Value()); + // + // Mark the session as defunct to signal that we no longer want to use this + // session anymore for further interactions to this peer. However, if we receive + // messages back from that peer on the defunct session, it will bring it back into an active + // state again. + // + mSecureSession.Get().Value()->AsSecureSession()->MarkAsDefunct(); } - MoveToState(State::HasAddress); + mSecureSession.Release(); + + MoveToState(State::HasAddress); return CHIP_NO_ERROR; } diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 00dd13ee8a1830..24942a2b39d158 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -66,7 +66,7 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv ChipLogError(AppServer, "Failsafe timer expired"); if (mPASESession) { - mPASESession->AsSecureSession()->MarkForRemoval(); + mPASESession->AsSecureSession()->MarkForEviction(); } HandleFailedAttempt(CHIP_ERROR_TIMEOUT); } @@ -177,7 +177,7 @@ void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & sess { ChipLogError(AppServer, "Error arming failsafe on PASE session establishment completion"); // Don't allow a PASE session to hang around without a fail-safe. - session->AsSecureSession()->MarkForRemoval(); + session->AsSecureSession()->MarkForEviction(); HandleFailedAttempt(err); } } diff --git a/src/app/tests/suites/certification/Test_TC_WNCV_4_5.yaml b/src/app/tests/suites/certification/Test_TC_WNCV_4_5.yaml index cd8d97952666a6..9c0cd339514797 100644 --- a/src/app/tests/suites/certification/Test_TC_WNCV_4_5.yaml +++ b/src/app/tests/suites/certification/Test_TC_WNCV_4_5.yaml @@ -50,13 +50,17 @@ tests: - name: "LiftPercentageValue" value: 90 + # + # TODO: For some reason, the window-covering server impl arms a 5s timer before it actually + # processes the GoToLiftPercentage command's effect on the current tile value. + # - label: "1b: TH Waits for 100ms-1s" cluster: "DelayCommands" command: "WaitForMs" arguments: values: - name: "ms" - value: 500 + value: 5000 - label: "1c: TH sends StopMotion command to DUT" command: "StopMotion" @@ -83,13 +87,17 @@ tests: - name: "TiltPercentageValue" value: 90 + # + # TODO: For some reason, the window-covering server impl arms a 5s timer before it actually + # processes the GoToTiltPercentage command's effect on the current tile value. + # - label: "2b: TH Waits for 100ms-1s" cluster: "DelayCommands" command: "WaitForMs" arguments: values: - name: "ms" - value: 500 + value: 5000 - label: "2c: TH sends StopMotion command to DUT" command: "StopMotion" diff --git a/src/controller/CommissioneeDeviceProxy.cpp b/src/controller/CommissioneeDeviceProxy.cpp index 5554860e955e95..dec9800012153b 100644 --- a/src/controller/CommissioneeDeviceProxy.cpp +++ b/src/controller/CommissioneeDeviceProxy.cpp @@ -61,8 +61,9 @@ CHIP_ERROR CommissioneeDeviceProxy::CloseSession() ReturnErrorCodeIf(mState != ConnectionState::SecureConnected, CHIP_ERROR_INCORRECT_STATE); if (mSecureSession) { - mSessionManager->ExpirePairing(mSecureSession.Get().Value()); + mSecureSession->AsSecureSession()->MarkForEviction(); } + mState = ConnectionState::NotConnected; mPairing.Clear(); return CHIP_NO_ERROR; @@ -97,6 +98,8 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres CHIP_ERROR CommissioneeDeviceProxy::SetConnected(const SessionHandle & session) { VerifyOrReturnError(mState == ConnectionState::Connecting, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(session->AsSecureSession()->IsPASESession(), CHIP_ERROR_INVALID_ARGUMENT); + if (!mSecureSession.Grab(session)) { mState = ConnectionState::NotConnected; diff --git a/src/controller/python/ChipDeviceController-StorageDelegate.cpp b/src/controller/python/ChipDeviceController-StorageDelegate.cpp index 49fb532b24f177..2f66fee2b476f7 100644 --- a/src/controller/python/ChipDeviceController-StorageDelegate.cpp +++ b/src/controller/python/ChipDeviceController-StorageDelegate.cpp @@ -126,6 +126,7 @@ CHIP_ERROR StorageAdapter::SyncDeleteKeyValue(const char * key) return err; } + ChipLogDetail(Controller, "StorageAdapter::DeleteKeyValue: Key = %s", key); mDeleteKeyCb(mContext, key); return CHIP_NO_ERROR; } diff --git a/src/controller/python/chip/storage/__init__.py b/src/controller/python/chip/storage/__init__.py index c19e6aa41f6136..10cbcb5bec3216 100644 --- a/src/controller/python/chip/storage/__init__.py +++ b/src/controller/python/chip/storage/__init__.py @@ -36,7 +36,7 @@ None, py_object, c_char_p, POINTER(c_char), c_uint16) _SyncGetKeyValueCbFunct = CFUNCTYPE( None, py_object, c_char_p, POINTER(c_char), POINTER(c_uint16)) -_SyncDeleteKeyValueCbFunct = CFUNCTYPE(None, py_object, POINTER(c_char_p)) +_SyncDeleteKeyValueCbFunct = CFUNCTYPE(None, py_object, c_char_p) @_SyncSetKeyValueCbFunct diff --git a/src/controller/python/test/test_scripts/base.py b/src/controller/python/test/test_scripts/base.py index d992772ad93308..0c1764a63e4c66 100644 --- a/src/controller/python/test/test_scripts/base.py +++ b/src/controller/python/test/test_scripts/base.py @@ -177,6 +177,7 @@ def __init__(self, nodeid: int, paaTrustStorePath: str, testCommissioner: bool = self.controllerNodeId = nodeid self.logger = logger self.paaTrustStorePath = paaTrustStorePath + logging.getLogger().setLevel(logging.DEBUG) def _WaitForOneDiscoveredDevice(self, timeoutSeconds: int = 2): print("Waiting for device responses...") @@ -356,6 +357,40 @@ def TestFailsafe(self, nodeid: int): return True return False + async def TestCaseEviction(self, nodeid: int): + self.logger.info("Testing CASE eviction") + + minimumCASESessionsPerFabric = 3 + minimumSupportedFabrics = 16 + + # + # This test exercises the ability to allocate more sessions than are supported in the + # pool configuration. By going beyond (minimumSupportedFabrics * minimumCASESessionsPerFabric), + # it starts to test out the eviction logic. This specific test does not validate the specifics + # of eviction, just that allocation and CASE session establishment proceeds successfully on both + # the controller and target. + # + for x in range(minimumSupportedFabrics * minimumCASESessionsPerFabric * 3): + self.devCtrl.CloseSession(nodeid) + await self.devCtrl.ReadAttribute(nodeid, [(Clusters.Basic.Attributes.ClusterRevision)]) + + self.logger.info("Testing CASE defunct logic") + + # + # This tests establishing a subscription on a given CASE session, then mark it defunct (to simulate + # encountering a transport timeout on the session). + # + # At the max interval, we should still have a valid subscription. + # + sub = await self.devCtrl.ReadAttribute(nodeid, [(Clusters.Basic.Attributes.ClusterRevision)], reportInterval=(0, 2)) + await asyncio.sleep(2) + self.devCtrl.CloseSession(nodeid) + await asyncio.sleep(4) + + sub.Shutdown() + + return True + async def TestMultiFabric(self, ip: str, setuppin: int, nodeid: int): self.logger.info("Opening Commissioning Window") @@ -823,6 +858,7 @@ def run(self): # Thread join timed out self.logger.error(f"Failed to join change thread") return False + return True if receivedUpdate == 5 else False except Exception as ex: diff --git a/src/controller/python/test/test_scripts/mobile-device-test.py b/src/controller/python/test/test_scripts/mobile-device-test.py index f20f15338325f2..9ed69d9d5d19b3 100755 --- a/src/controller/python/test/test_scripts/mobile-device-test.py +++ b/src/controller/python/test/test_scripts/mobile-device-test.py @@ -76,6 +76,19 @@ def ethernet_commissioning(test: BaseTestHelper, discriminator: int, setup_pin: nodeid=device_nodeid), "Failed to finish key exchange") + # + # Run this before the MultiFabric test, since it will result in the resultant CASE session + # on fabric2 to be evicted (due to the stressful nature of that test) on the target. + # + # It doesn't actually evict the CASE session for fabric2 on the client, since we prioritize + # defunct sessions for eviction first, which means our CASE session on fabric2 remains preserved + # throughout the stress test. This results in a mis-match later. + # + # TODO: Once we implement fabric-adjusted LRU, we should see if this issue remains (it shouldn't) + # + logger.info("Testing CASE Eviction") + FailIfNot(asyncio.run(test.TestCaseEviction(device_nodeid)), "Failed TestCaseEviction") + ok = asyncio.run(test.TestMultiFabric(ip=address, setuppin=20202021, nodeid=1)) diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 5084ee22b3d832..6cdb67957cdc57 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -650,11 +650,16 @@ * secure sessions. This controls the maximum number of concurrent * established secure sessions across all supported transports. * - * This is sized to cover the sum of the following: + * This is sized by default to cover the sum of the following: * - At least 3 CASE sessions / fabric (Spec Ref: 4.13.2.8) * - 1 reserved slot for CASEServer as a responder. * - 1 reserved slot for PASE. * + * NOTE: On heap-based platforms, there is no pre-allocation of the pool. + * Due to the use of an LRU-scheme to manage sessions, the actual active + * size of the pool will grow up to the value of this define, + * after which, it will remain at or around this size indefinitely. + * */ #ifndef CHIP_CONFIG_SECURE_SESSION_POOL_SIZE #define CHIP_CONFIG_SECURE_SESSION_POOL_SIZE (CHIP_CONFIG_MAX_FABRICS * 3 + 2) diff --git a/src/lib/core/CHIPPersistentStorageDelegate.h b/src/lib/core/CHIPPersistentStorageDelegate.h index df74b82b3d8e3e..3f556d7f798950 100644 --- a/src/lib/core/CHIPPersistentStorageDelegate.h +++ b/src/lib/core/CHIPPersistentStorageDelegate.h @@ -93,7 +93,7 @@ class DLL_EXPORT PersistentStorageDelegate * * @param[in] key Key to be deleted * - * @return CHIP_NO_ERROR on success, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is not found in storage, + * @return CHIP_NO_ERROR on success, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if the key is not found in storage, * or another CHIP_ERROR value from implementation on failure. */ virtual CHIP_ERROR SyncDeleteKeyValue(const char * key) = 0; diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index 81a9e0e255c12b..d17f252062d893 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -99,6 +99,7 @@ static_library("support") { "SafeInt.h", "SerializableIntegerSet.cpp", "SerializableIntegerSet.h", + "SortUtils.h", "StateMachine.h", "ThreadOperationalDataset.cpp", "ThreadOperationalDataset.h", diff --git a/src/lib/support/CodeUtils.h b/src/lib/support/CodeUtils.h index a1687f9075059d..ae61f5211aae4b 100644 --- a/src/lib/support/CodeUtils.h +++ b/src/lib/support/CodeUtils.h @@ -285,12 +285,29 @@ constexpr inline const _T & max(const _T & a, const _T & b) * @param[in] expr A Boolean expression to be evaluated. * @param[in] code A value to return if @a expr is false. */ -#define VerifyOrReturnError(expr, code) \ +#define VerifyOrReturnError(expr, code) VerifyOrReturnValue(expr, code) + +/** + * @def VerifyOrReturnValue(expr, value) + * + * @brief + * Returns a specified value if expression evaluates to false + * + * Example usage: + * + * @code + * VerifyOrReturnError(param != nullptr, Foo()); + * @endcode + * + * @param[in] expr A Boolean expression to be evaluated. + * @param[in] value A value to return if @a expr is false. + */ +#define VerifyOrReturnValue(expr, value) \ do \ { \ if (!(expr)) \ { \ - return code; \ + return (value); \ } \ } while (false) diff --git a/src/lib/support/SortUtils.h b/src/lib/support/SortUtils.h new file mode 100644 index 00000000000000..27ab1852d3da1a --- /dev/null +++ b/src/lib/support/SortUtils.h @@ -0,0 +1,71 @@ +/* + * + * Copyright (c) 2020-2021 Project CHIP Authors + * Copyright (c) 2013-2017 Nest Labs, Inc. + * 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. + */ + +#pragma once + +#include +#include +#include + +namespace chip { +namespace Sorting { + +/** + * + * Impements the bubble sort algorithm to sort an array + * of items of size 'n'. + * + * T should be swappable using std::swap (i.e implements Swappable). + * + * The provided comparison function SHALL have the following signature: + * + * bool Compare(const T& a, const T& b); + * + * If a is deemed less than (i.e is ordered before) b, return true. + * Else, return false. + * + * This is a stable sort. + * + * NOTE: This has a worst case time complexity of O(n^2). This + * is well-suited for small arrays where the time trade-off is + * acceptable compared to the lower flash cost for the simple sorting + * implementation. + * + */ +template +void BubbleSort(T * items, size_t n, CompareFunc f) +{ + for (size_t i = 0; i < (n - 1); i++) + { + for (size_t j = 0; j < (n - i - 1); j++) + { + const T & a = items[j + 1]; + const T & b = items[j]; + + if (f(a, b)) + { + std::swap(items[j], items[j + 1]); + } + } + } +} + +} // namespace Sorting + +} // namespace chip diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 422a2f88e34ce4..b3bc08899ebfbb 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -326,7 +326,7 @@ ExchangeContext::~ExchangeContext() VerifyOrDie(!IsAckPending()); if (ReleaseSessionOnDestruction() && mSession) - mSession->AsSecureSession()->MarkForRemoval(); + 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. diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index b01d5b4fe4cb5c..c1d613d4aa39de 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -240,7 +240,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const // 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. - if (session->IsActiveSession() && !msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator()) + if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator()) { // Search for an unsolicited message handler that can handle the message. Prefer handlers that can explicitly // handle the message type over handlers that handle all messages for a profile. diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp index dd6db9fea07c15..a93bae5566a3e0 100644 --- a/src/messaging/tests/MessagingContext.cpp +++ b/src/messaging/tests/MessagingContext.cpp @@ -155,7 +155,7 @@ void MessagingContext::ExpireSessionBobToAlice() { if (mSessionBobToAlice) { - mSessionManager.ExpirePairing(mSessionBobToAlice.Get().Value()); + mSessionBobToAlice.Get().Value()->AsSecureSession()->MarkForEviction(); } } @@ -163,7 +163,7 @@ void MessagingContext::ExpireSessionAliceToBob() { if (mSessionAliceToBob) { - mSessionManager.ExpirePairing(mSessionAliceToBob.Get().Value()); + mSessionAliceToBob.Get().Value()->AsSecureSession()->MarkForEviction(); } } diff --git a/src/messaging/tests/TestExchangeMgr.cpp b/src/messaging/tests/TestExchangeMgr.cpp index 39d14dab1477cc..52ecc256ecca94 100644 --- a/src/messaging/tests/TestExchangeMgr.cpp +++ b/src/messaging/tests/TestExchangeMgr.cpp @@ -120,7 +120,7 @@ void CheckSessionExpirationBasics(nlTestSuite * inSuite, void * inContext) ExchangeContext * ec1 = ctx.NewExchangeToBob(&sendDelegate); // Expire the session this exchange is supposedly on. - ctx.GetSecureSessionManager().ExpirePairing(ec1->GetSessionHandle()); + ec1->GetSessionHandle()->AsSecureSession()->MarkForEviction(); MockAppDelegate receiveDelegate; CHIP_ERROR err = @@ -156,7 +156,7 @@ void CheckSessionExpirationTimeout(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, !sendDelegate.IsOnResponseTimeoutCalled); // Expire the session this exchange is supposedly on. This should close the exchange. - ctx.GetSecureSessionManager().ExpirePairing(ec1->GetSessionHandle()); + ec1->GetSessionHandle()->AsSecureSession()->MarkForEviction(); NL_TEST_ASSERT(inSuite, sendDelegate.IsOnResponseTimeoutCalled); // recreate closed session. diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp index 5751cd56850eac..d2e8879de4a882 100644 --- a/src/protocols/secure_channel/CASEServer.cpp +++ b/src/protocols/secure_channel/CASEServer.cpp @@ -103,6 +103,17 @@ void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyE GetSession().Clear(); + // + // This releases our reference to a previously pinned session. If that was a successfully established session and is now + // active, this will have no effect (the session will remain in the session table). + // + // If we previously held a session still in the pairing state, it means PairingSession was owning that session. Since it + // gave up its reference by the time we got here, releasing the pinned session here will actually result in it being + // de-allocated since no one else is holding onto this session. This will mean that when we get to allocating a session below, + // we'll at least have one free session available in the session table, and won't need to evict an arbitrary session. + // + mPinnedSecureSession.ClearValue(); + // // Indicate to the underlying CASE session to prepare for session establishment requests coming its way. This will // involve allocating a SecureSession that will be held until it's needed for the next CASE session handshake. @@ -125,7 +136,7 @@ void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyE CHIP_NO_ERROR); // - // PairingSession::mSecureSessionHolder is a weak-reference. If MarkForRemoval is called on this session, the session is + // PairingSession::mSecureSessionHolder is a weak-reference. If MarkForEviction is called on this session, the session is // going to get de-allocated from underneath us. This session that has just been allocated should *never* get evicted, and // remain available till the next hand-shake is received. // @@ -146,7 +157,17 @@ void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyE void CASEServer::OnSessionEstablishmentError(CHIP_ERROR err) { ChipLogError(Inet, "CASE Session establishment failed: %s", ErrorStr(err)); - PrepareForSessionEstablishment(); + + // + // We're not allowed to call methods that will eventually result in calling SessionManager::AllocateSecureSession + // from a SessionDelegate::OnSessionReleased callback. Schedule the preparation as an async work item. + // + mSessionManager->SystemLayer()->ScheduleWork( + [](auto * systemLayer, auto * appState) { + CASEServer * _this = static_cast(appState); + _this->PrepareForSessionEstablishment(); + }, + this); } void CASEServer::OnSessionEstablished(const SessionHandle & session) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 2a84ec920838e6..3e3097d6cf7cf1 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -193,7 +193,7 @@ CHIP_ERROR CASESession::PrepareForSessionEstablishment(SessionManager & sessionManager, FabricTable * fabricTable, SessionResumptionStorage * sessionResumptionStorage, Credentials::CertificateValidityPolicy * policy, - SessionEstablishmentDelegate * delegate, ScopedNodeId previouslyEstablishedPeer, + SessionEstablishmentDelegate * delegate, const ScopedNodeId & previouslyEstablishedPeer, Optional mrpConfig) { // Below VerifyOrReturnError is not SuccessOrExit since we only want to goto `exit:` after diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index 01601ba8d2676c..65644e5c08c475 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -87,7 +87,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, CHIP_ERROR PrepareForSessionEstablishment( SessionManager & sessionManager, FabricTable * fabricTable, SessionResumptionStorage * sessionResumptionStorage, Credentials::CertificateValidityPolicy * policy, SessionEstablishmentDelegate * delegate, - ScopedNodeId previouslyEstablishedPeer, + const ScopedNodeId & previouslyEstablishedPeer, Optional mrpConfig = Optional::Missing()); /** diff --git a/src/protocols/secure_channel/PairingSession.cpp b/src/protocols/secure_channel/PairingSession.cpp index 8b12fa69974b80..ef87bedf3051c9 100644 --- a/src/protocols/secure_channel/PairingSession.cpp +++ b/src/protocols/secure_channel/PairingSession.cpp @@ -34,14 +34,10 @@ CHIP_ERROR PairingSession::AllocateSecureSession(SessionManager & sessionManager CHIP_ERROR PairingSession::ActivateSecureSession(const Transport::PeerAddress & peerAddress) { - // TODO(17568): Replace with proper expiry logic. This current method makes sure there - // are not multiple sessions established, until eventual exhaustion of the resources - // for CASE sessions. Current method is quick fix for #17698, cannot remain. - mSessionManager->ExpireAllPairingsForPeerExceptPending(GetPeer()); - // Prepare SecureSession fields, including key derivation, first, before activation Transport::SecureSession * secureSession = mSecureSessionHolder->AsSecureSession(); ReturnErrorOnFailure(DeriveSecureSession(secureSession->GetCryptoContext())); + uint16_t peerSessionId = GetPeerSessionId(); secureSession->SetPeerAddress(peerAddress); secureSession->GetSessionMessageCounter().GetPeerMessageCounter().SetCounter(Transport::PeerMessageCounter::kInitialSyncValue); @@ -50,7 +46,7 @@ CHIP_ERROR PairingSession::ActivateSecureSession(const Transport::PeerAddress & // a partially valid session. secureSession->Activate(GetLocalScopedNodeId(), GetPeer(), GetPeerCATs(), peerSessionId, mRemoteMRPConfig); - ChipLogDetail(Inet, "New secure session created for device " ChipLogFormatScopedNodeId ", LSID:%d PSID:%d!", + ChipLogDetail(Inet, "New secure session activated for device " ChipLogFormatScopedNodeId ", LSID:%d PSID:%d!", ChipLogValueScopedNodeId(GetPeer()), secureSession->GetLocalSessionId(), peerSessionId); return CHIP_NO_ERROR; @@ -146,7 +142,7 @@ bool PairingSession::IsSessionEstablishmentInProgress() } Transport::SecureSession * secureSession = mSecureSessionHolder->AsSecureSession(); - return secureSession->IsPairing(); + return secureSession->IsEstablishing(); } void PairingSession::Clear() diff --git a/src/protocols/secure_channel/UnsolicitedStatusHandler.cpp b/src/protocols/secure_channel/UnsolicitedStatusHandler.cpp index b04a1bcf557529..cb3757983d5ae8 100644 --- a/src/protocols/secure_channel/UnsolicitedStatusHandler.cpp +++ b/src/protocols/secure_channel/UnsolicitedStatusHandler.cpp @@ -52,7 +52,7 @@ CHIP_ERROR UnsolicitedStatusHandler::OnMessageReceived(ExchangeContext * ec, con if (report.GetGeneralCode() == GeneralStatusCode::kSuccess && report.GetProtocolCode() == kProtocolCodeCloseSession) { ChipLogProgress(ExchangeManager, "Received CloseSession status message, closing session"); - session->AsSecureSession()->MarkForRemoval(); + session->AsSecureSession()->MarkForEviction(); return CHIP_NO_ERROR; } diff --git a/src/transport/BUILD.gn b/src/transport/BUILD.gn index e70679a50cb61d..155326ab95eda1 100644 --- a/src/transport/BUILD.gn +++ b/src/transport/BUILD.gn @@ -34,6 +34,7 @@ static_library("transport") { "SecureMessageCodec.h", "SecureSession.cpp", "SecureSession.h", + "SecureSessionTable.cpp", "SecureSessionTable.h", "Session.cpp", "Session.h", diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp index 9363d92b323f21..8756da23d659ee 100644 --- a/src/transport/SecureSession.cpp +++ b/src/transport/SecureSession.cpp @@ -26,25 +26,106 @@ void SecureSessionDeleter::Release(SecureSession * entry) entry->mTable.ReleaseSession(entry); } -void SecureSession::MarkForRemoval() +const char * SecureSession::StateToString(State state) const { - ChipLogDetail(Inet, "SecureSession[%p]: MarkForRemoval Type:%d LSID:%d", this, to_underlying(mSecureSessionType), + switch (state) + { + case State::kEstablishing: + return "kEstablishing"; + break; + + case State::kActive: + return "kActive"; + break; + + case State::kDefunct: + return "kDefunct"; + break; + + case State::kPendingEviction: + return "kPendingEviction"; + break; + + default: + return "???"; + break; + } +} + +void SecureSession::MoveToState(State targetState) +{ + if (mState != targetState) + { + ChipLogProgress(SecureChannel, "SecureSession[%p]: Moving from state '%s' --> '%s'", this, StateToString(mState), + StateToString(targetState)); + mState = targetState; + } +} + +void SecureSession::MarkAsDefunct() +{ + ChipLogDetail(Inet, "SecureSession[%p]: MarkAsDefunct Type:%d LSID:%d", this, to_underlying(mSecureSessionType), mLocalSessionId); ReferenceCountedHandle ref(*this); + switch (mState) { - case State::kPairing: - mState = State::kPendingRemoval; + case State::kEstablishing: + // + // A session can only be marked as defunct from the state of Active. + // + VerifyOrDie(false); + return; + + case State::kActive: + MoveToState(State::kDefunct); + return; + + case State::kDefunct: + // + // Do nothing + // + 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. + // + VerifyOrDie(false); + return; + } +} + +void SecureSession::MarkForEviction() +{ + ChipLogDetail(Inet, "SecureSession[%p]: MarkForEviction Type:%d LSID:%d", this, to_underlying(mSecureSessionType), + mLocalSessionId); + ReferenceCountedHandle ref(*this); + + switch (mState) + { + case State::kEstablishing: + MoveToState(State::kPendingEviction); // Interrupt the pairing NotifySessionReleased(); return; + + case State::kDefunct: + FALLTHROUGH; case State::kActive: + FALLTHROUGH; case State::kInactive: Release(); // Decrease the ref which is retained at Activate - mState = State::kPendingRemoval; + MoveToState(State::kPendingEviction); NotifySessionReleased(); return; - case State::kPendingRemoval: + + case State::kPendingEviction: // Do nothing return; } @@ -57,15 +138,17 @@ void SecureSession::MarkInactive() ReferenceCountedHandle ref(*this); switch (mState) { - case State::kPairing: + 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::kPendingRemoval: + case State::kPendingEviction: // Do nothing return; } diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index 829b179965a8b2..d8b798fe6988d3 100644 --- a/src/transport/SecureSession.h +++ b/src/transport/SecureSession.h @@ -71,10 +71,11 @@ class SecureSession : public Session, public ReferenceCounted +#include +#include +#include + +namespace chip { +namespace Transport { + +Optional SecureSessionTable::CreateNewSecureSessionForTest(SecureSession::Type secureSessionType, + uint16_t localSessionId, NodeId localNodeId, + NodeId peerNodeId, CATValues peerCATs, + uint16_t peerSessionId, FabricIndex fabricIndex, + const ReliableMessageProtocolConfig & config) +{ + if (secureSessionType == SecureSession::Type::kCASE) + { + if ((fabricIndex == kUndefinedFabricIndex) || (localNodeId == kUndefinedNodeId) || (peerNodeId == kUndefinedNodeId)) + { + return Optional::Missing(); + } + } + else if (secureSessionType == SecureSession::Type::kPASE) + { + if ((fabricIndex != kUndefinedFabricIndex) || (localNodeId != kUndefinedNodeId) || (peerNodeId != kUndefinedNodeId)) + { + // TODO: This secure session type is infeasible! We must fix the tests + if (false) + { + return Optional::Missing(); + } + + (void) fabricIndex; + } + } + + SecureSession * result = mEntries.CreateObject(*this, secureSessionType, localSessionId, localNodeId, peerNodeId, peerCATs, + peerSessionId, fabricIndex, config); + return result != nullptr ? MakeOptional(*result) : Optional::Missing(); +} + +Optional SecureSessionTable::CreateNewSecureSession(SecureSession::Type secureSessionType, + ScopedNodeId sessionEvictionHint) +{ + Optional rv = Optional::Missing(); + SecureSession * allocated = nullptr; + + auto sessionId = FindUnusedSessionId(); + VerifyOrReturnValue(sessionId.HasValue(), Optional::Missing()); + + // + // We allocate a new session out of the pool if we have space in it. If we don't, we need + // to run the eviction algorithm to get a free slot. We shall ALWAYS be guaranteed to evict + // an existing session in the table in normal operating circumstances. + // + if (mEntries.Allocated() < CHIP_CONFIG_SECURE_SESSION_POOL_SIZE) + { + allocated = mEntries.CreateObject(*this, secureSessionType, sessionId.Value()); + } + else + { + allocated = EvictAndAllocate(sessionId.Value(), secureSessionType, sessionEvictionHint); + } + + VerifyOrReturnValue(allocated != nullptr, Optional::Missing()); + + rv = MakeOptional(*allocated); + mNextSessionId = sessionId.Value() == kMaxSessionID ? static_cast(kUnsecuredSessionId + 1) + : static_cast(sessionId.Value() + 1); + + return rv; +} + +SecureSession * SecureSessionTable::EvictAndAllocate(uint16_t localSessionId, SecureSession::Type secureSessionType, + const ScopedNodeId & sessionEvictionHint) +{ + VerifyOrDieWithMsg(!mRunningEvictionLogic, SecureChannel, + "EvictAndAllocate isn't re-entrant, yet someone called us while we're already running"); + + mRunningEvictionLogic = true; + + auto cleanup = MakeDefer([this]() { mRunningEvictionLogic = false; }); + + ChipLogProgress(SecureChannel, "Evicting a slot for session with LSID: %d, type: %u", localSessionId, + (uint8_t) secureSessionType); + + VerifyOrDie(mEntries.Allocated() <= CHIP_CONFIG_SECURE_SESSION_POOL_SIZE); + + // + // Create a temporary list of objects each of which points to a session in the existing + // session table, but are swappable. This allows them to then be used with a sorting algorithm + // without affecting the sessions in the table itself. + // + Platform::ScopedMemoryBufferWithSize sortableSessions; + sortableSessions.Calloc(mEntries.Allocated()); + if (!sortableSessions) + { + VerifyOrDieWithMsg(false, SecureChannel, "We couldn't allocate a session!"); + return nullptr; + } + + int index = 0; + ForEachSession([&index, &sortableSessions](auto session) { + sortableSessions.Get()[index].mSession = session; + index++; + return Loop::Continue; + }); + + auto sortableSessionSpan = Span(sortableSessions.Get(), mEntries.Allocated()); + EvictionPolicyContext policyContext(sortableSessionSpan, sessionEvictionHint); + + DefaultEvictionPolicy(policyContext); + ChipLogProgress(SecureChannel, "Sorted sessions for eviction..."); + + auto numSessions = mEntries.Allocated(); + +#if CHIP_DETAIL_LOGGING + ChipLogDetail(SecureChannel, "Sorted Eviction Candidates (ranked from best candidate to worst):"); + for (auto * session = sortableSessions.Get(); session != (sortableSessions.Get() + numSessions); session++) + { + ChipLogDetail(SecureChannel, "\t%ld: [%p] -- State: '%s', ActivityTime: %lu", + static_cast(session - sortableSessions.Get()), session->mSession, session->mSession->GetStateStr(), + static_cast(session->mSession->GetLastActivityTime().count())); + } +#endif + + for (auto * session = sortableSessions.Get(); session != (sortableSessions.Get() + numSessions); session++) + { + if (session->mSession->IsPendingEviction()) + { + continue; + } + + ChipLogProgress(SecureChannel, "Candidate Session[%p] - Attempting to evict...", session->mSession); + + auto prevCount = mEntries.Allocated(); + + // + // SessionHolders act like weak-refs on a session, but since they do still add to the ref-count of a SecureSession, we + // cannot actually tell whether there are truly any strong-refs (SessionHandles) on this session because if we did, we'd + // avoid evicting it since it's pointless to do so. + // + // However, we don't actually have SessionHolders implemented correctly as weak-refs, requiring us to go ahead and 'try' to + // evict it, and see if it still remains in the table. If it does, we have to try the next one. If it doesn't, we know we've + // earned a free spot. + // + // See #19495. + // + session->mSession->MarkForEviction(); + + auto newCount = mEntries.Allocated(); + + if (newCount < prevCount) + { + ChipLogProgress(SecureChannel, "Successfully evicted a session!"); + auto * retSession = mEntries.CreateObject(*this, secureSessionType, localSessionId); + VerifyOrDie(session != nullptr); + return retSession; + } + } + + VerifyOrDieWithMsg(false, SecureChannel, "We couldn't find any session to evict at all, something's wrong!"); + return nullptr; +} + +void SecureSessionTable::DefaultEvictionPolicy(EvictionPolicyContext & evictionContext) +{ + evictionContext.Sort([](const auto & a, const auto & b) { + int aStateScore = 0, bStateScore = 0; + + auto assignStateScore = [](auto & score, const auto & session) { + if (session.IsDefunct()) + { + score = 2; + } + else if (session.IsActiveSession()) + { + score = 1; + } + else + { + score = 0; + } + }; + + assignStateScore(aStateScore, *a.mSession); + assignStateScore(bStateScore, *b.mSession); + + return ((aStateScore > bStateScore) ? true : (a->GetLastActivityTime() < b->GetLastActivityTime())); + }); +} + +Optional SecureSessionTable::FindSecureSessionByLocalKey(uint16_t localSessionId) +{ + SecureSession * result = nullptr; + mEntries.ForEachActiveObject([&](auto session) { + if (session->GetLocalSessionId() == localSessionId) + { + result = session; + return Loop::Break; + } + return Loop::Continue; + }); + return result != nullptr ? MakeOptional(*result) : Optional::Missing(); +} + +Optional SecureSessionTable::FindUnusedSessionId() +{ + uint16_t candidate_base = 0; + uint64_t candidate_mask = 0; + for (uint32_t i = 0; i <= kMaxSessionID; i += 64) + { + // candidate_base is the base session ID we are searching from. + // We have a 64-bit mask anchored at this ID and iterate over the + // whole session table, setting bits in the mask for in-use IDs. + // If we can iterate through the entire session table and have + // any bits clear in the mask, we have available session IDs. + candidate_base = static_cast(i + mNextSessionId); + candidate_mask = 0; + { + uint16_t shift = static_cast(kUnsecuredSessionId - candidate_base); + if (shift <= 63) + { + candidate_mask |= (1ULL << shift); // kUnsecuredSessionId is never available + } + } + mEntries.ForEachActiveObject([&](auto session) { + uint16_t shift = static_cast(session->GetLocalSessionId() - candidate_base); + if (shift <= 63) + { + candidate_mask |= (1ULL << shift); + } + if (candidate_mask == UINT64_MAX) + { + return Loop::Break; // No bits clear means this bucket is full. + } + return Loop::Continue; + }); + if (candidate_mask != UINT64_MAX) + { + break; // Any bit clear means we have an available ID in this bucket. + } + } + if (candidate_mask != UINT64_MAX) + { + uint16_t offset = 0; + while (candidate_mask & 1) + { + candidate_mask >>= 1; + ++offset; + } + uint16_t available = static_cast(candidate_base + offset); + return MakeOptional(available); + } + + return NullOptional; +} + +} // namespace Transport +} // namespace chip diff --git a/src/transport/SecureSessionTable.h b/src/transport/SecureSessionTable.h index 3fcbf86cd14abe..b084f83ab013a2 100644 --- a/src/transport/SecureSessionTable.h +++ b/src/transport/SecureSessionTable.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -63,36 +64,7 @@ class SecureSessionTable Optional CreateNewSecureSessionForTest(SecureSession::Type secureSessionType, uint16_t localSessionId, NodeId localNodeId, NodeId peerNodeId, CATValues peerCATs, uint16_t peerSessionId, FabricIndex fabricIndex, - const ReliableMessageProtocolConfig & config) - { - if (secureSessionType == SecureSession::Type::kCASE) - { - if ((fabricIndex == kUndefinedFabricIndex) || (localNodeId == kUndefinedNodeId) || (peerNodeId == kUndefinedNodeId)) - { - return Optional::Missing(); - } - } - else if (secureSessionType == SecureSession::Type::kPASE) - { - if ((fabricIndex != kUndefinedFabricIndex) || (localNodeId != kUndefinedNodeId) || (peerNodeId != kUndefinedNodeId)) - { - // TODO: This secure session type is infeasible! We must fix the tests - if (false) - { - return Optional::Missing(); - } - else - { - (void) fabricIndex; - } - } - } - - SecureSession * result = mEntries.CreateObject(*this, secureSessionType, localSessionId, localNodeId, peerNodeId, peerCATs, - peerSessionId, fabricIndex, config); - return result != nullptr ? MakeOptional(*result) : Optional::Missing(); - } - + const ReliableMessageProtocolConfig & config); /** * Allocate a new secure session out of the internal resource pool with a * non-colliding session ID and increments mNextSessionId to give a clue to @@ -102,20 +74,7 @@ class SecureSessionTable * @returns allocated session, or NullOptional on failure */ CHECK_RETURN_VALUE - Optional CreateNewSecureSession(SecureSession::Type secureSessionType) - { - Optional rv = Optional::Missing(); - auto sessionId = FindUnusedSessionId(); - SecureSession * allocated = nullptr; - VerifyOrExit(sessionId.HasValue(), rv = Optional::Missing()); - allocated = mEntries.CreateObject(*this, secureSessionType, sessionId.Value()); - VerifyOrExit(allocated != nullptr, rv = Optional::Missing()); - rv = MakeOptional(*allocated); - mNextSessionId = sessionId.Value() == kMaxSessionID ? static_cast(kUnsecuredSessionId + 1) - : static_cast(sessionId.Value() + 1); - exit: - return rv; - } + Optional CreateNewSecureSession(SecureSession::Type secureSessionType, ScopedNodeId sessionEvictionHint); void ReleaseSession(SecureSession * session) { mEntries.ReleaseObject(session); } @@ -133,21 +92,100 @@ class SecureSessionTable * @return the session if found, NullOptional if not found */ CHECK_RETURN_VALUE - Optional FindSecureSessionByLocalKey(uint16_t localSessionId) - { - SecureSession * result = nullptr; - mEntries.ForEachActiveObject([&](auto session) { - if (session->GetLocalSessionId() == localSessionId) - { - result = session; - return Loop::Break; - } - return Loop::Continue; - }); - return result != nullptr ? MakeOptional(*result) : Optional::Missing(); - } + Optional FindSecureSessionByLocalKey(uint16_t localSessionId); private: + friend class TestSecureSessionTable; + + /** + * This provides a sortable wrapper for a SecureSession object. A SecureSession + * isn't directly sortable since it is not swappable (i.e meet criteria for ValueSwappable). + * + * However, this wrapper has a stable pointer to a SecureSession while being swappable with + * another instance of it. + * + */ + struct SortableSession + { + public: + void swap(SortableSession & other) + { + SortableSession tmp(other); + other.mSession = mSession; + mSession = tmp.mSession; + } + + const Transport::SecureSession * operator->() const { return mSession; } + + private: + SecureSession * mSession; + friend class SecureSessionTable; + }; + + /** + * + * Encapsulates all the necessary context for an eviction policy callback + * to implement its specific policy. The context is provided to the callee + * with the expectation that it'll call Sort() with a comparator function provided + * to get the list of sessions sorted in the desired order. + * + */ + class EvictionPolicyContext + { + public: + /* + * Called by the policy implementor to sort the list of sessions given a comparator + * function. The provided function shall have the following signature: + * + * bool CompareFunc(const SortableSession &a, const SortableSession &b); + * + * If a is a better candidate than b, true should be returned. Else, return false. + * + * NOTE: Sort() can be called multiple times. + * + */ + template + void Sort(CompareFunc func) + { + Sorting::BubbleSort(mSessionList.begin(), mSessionList.size(), func); + } + + const ScopedNodeId & GetSessionEvictionHint() const { return mSessionEvictionHint; } + + private: + EvictionPolicyContext(Span sessionList, ScopedNodeId sessionEvictionHint) + { + mSessionList = sessionList; + mSessionEvictionHint = sessionEvictionHint; + } + + friend class SecureSessionTable; + Span mSessionList; + ScopedNodeId mSessionEvictionHint; + }; + + /** + * + * This implements the following eviction policy: + * + * - Sessions are sorted with their state as the primary sort key and activity time as the secondary + * sort key. + * - The primary sort key places defunct sessions ahead of active ones, ahead of anything else. + * - The secondary sort key places older sessions ahead of newer sessions. This ensures + * we're prioritizing reaping less active sessions over more recently active sessions (activity + * in either TX or RX). + * + */ + void DefaultEvictionPolicy(EvictionPolicyContext & evictionContext); + + /** + * + * Evicts a session from the session table using the DefaultEvictionPolicy implementation. + * + */ + SecureSession * EvictAndAllocate(uint16_t localSessionId, SecureSession::Type secureSessionType, + const ScopedNodeId & sessionEvictionHint); + /** * Find an available session ID that is unused in the secure session table. * @@ -162,59 +200,10 @@ class SecureSessionTable * @return an unused session ID if any is found, else NullOptional */ CHECK_RETURN_VALUE - Optional FindUnusedSessionId() - { - uint16_t candidate_base = 0; - uint64_t candidate_mask = 0; - for (uint32_t i = 0; i <= kMaxSessionID; i += 64) - { - // candidate_base is the base session ID we are searching from. - // We have a 64-bit mask anchored at this ID and iterate over the - // whole session table, setting bits in the mask for in-use IDs. - // If we can iterate through the entire session table and have - // any bits clear in the mask, we have available session IDs. - candidate_base = static_cast(i + mNextSessionId); - candidate_mask = 0; - { - uint16_t shift = static_cast(kUnsecuredSessionId - candidate_base); - if (shift <= 63) - { - candidate_mask |= (1ULL << shift); // kUnsecuredSessionId is never available - } - } - mEntries.ForEachActiveObject([&](auto session) { - uint16_t shift = static_cast(session->GetLocalSessionId() - candidate_base); - if (shift <= 63) - { - candidate_mask |= (1ULL << shift); - } - if (candidate_mask == UINT64_MAX) - { - return Loop::Break; // No bits clear means this bucket is full. - } - return Loop::Continue; - }); - if (candidate_mask != UINT64_MAX) - { - break; // Any bit clear means we have an available ID in this bucket. - } - } - if (candidate_mask != UINT64_MAX) - { - uint16_t offset = 0; - while (candidate_mask & 1) - { - candidate_mask >>= 1; - ++offset; - } - uint16_t available = static_cast(candidate_base + offset); - return MakeOptional(available); - } - - return NullOptional; - } + Optional FindUnusedSessionId(); - BitMapObjectPool mEntries; + bool mRunningEvictionLogic = false; + ObjectPool mEntries; uint16_t mNextSessionId = 0; }; diff --git a/src/transport/SessionDelegate.h b/src/transport/SessionDelegate.h index 763848dfbb85eb..6cdfb9e742bd64 100644 --- a/src/transport/SessionDelegate.h +++ b/src/transport/SessionDelegate.h @@ -45,7 +45,8 @@ class DLL_EXPORT SessionDelegate /** * @brief - * Called when a session is releasing + * Called when a session is releasing. Callees SHALL NOT make synchronous calls into SessionManager to allocate a new session. + * If they desire to do so, it MUST be done asynchronously. */ virtual void OnSessionReleased() = 0; diff --git a/src/transport/SessionHolder.cpp b/src/transport/SessionHolder.cpp index 97498cf9dc41fb..708cdb4f8e6930 100644 --- a/src/transport/SessionHolder.cpp +++ b/src/transport/SessionHolder.cpp @@ -81,7 +81,7 @@ bool SessionHolder::GrabPairingSession(const SessionHandle & session) if (!session->IsSecureSession()) return false; - if (!session->AsSecureSession()->IsPairing()) + if (!session->AsSecureSession()->IsEstablishing()) return false; mSession.Emplace(session.mSession); diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index 193f0d222eb88c..9f212752b4337d 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -333,31 +333,12 @@ CHIP_ERROR SessionManager::SendPreparedMessage(const SessionHandle & sessionHand return CHIP_ERROR_INCORRECT_STATE; } -void SessionManager::ExpirePairing(const SessionHandle & sessionHandle) -{ - sessionHandle->AsSecureSession()->MarkForRemoval(); -} - void SessionManager::ExpireAllPairings(const ScopedNodeId & node) { mSecureSessions.ForEachSession([&](auto session) { if (session->GetPeer() == node) { - session->MarkForRemoval(); - } - return Loop::Continue; - }); -} - -void SessionManager::ExpireAllPairingsForPeerExceptPending(const ScopedNodeId & node) -{ - mSecureSessions.ForEachSession([&](auto session) { - if ((session->GetPeer() == node) && session->IsActiveSession() && - (session->GetSecureSessionType() == SecureSession::Type::kCASE)) - { - ChipLogDetail(Inet, "Expired/released previous local session ID %u for peer " ChipLogFormatScopedNodeId, - static_cast(session->GetLocalSessionId()), ChipLogValueScopedNodeId(session->GetPeer())); - session->MarkForRemoval(); + session->MarkForEviction(); } return Loop::Continue; }); @@ -369,7 +350,7 @@ void SessionManager::ExpireAllPairingsForFabric(FabricIndex fabric) mSecureSessions.ForEachSession([&](auto session) { if (session->GetFabricIndex() == fabric) { - session->MarkForRemoval(); + session->MarkForEviction(); } return Loop::Continue; }); @@ -381,7 +362,7 @@ void SessionManager::ExpireAllPASEPairings() mSecureSessions.ForEachSession([&](auto session) { if (session->GetSecureSessionType() == Transport::SecureSession::Type::kPASE) { - session->MarkForRemoval(); + session->MarkForEviction(); } return Loop::Continue; }); @@ -398,7 +379,7 @@ void SessionManager::ReleaseSessionsForFabricExceptOne(FabricIndex fabricIndex, if (session == deferredSecureSession) session->MarkInactive(); else - session->MarkForRemoval(); + session->MarkForEviction(); } return Loop::Continue; }); @@ -407,12 +388,7 @@ void SessionManager::ReleaseSessionsForFabricExceptOne(FabricIndex fabricIndex, Optional SessionManager::AllocateSession(SecureSession::Type secureSessionType, const ScopedNodeId & sessionEvictionHint) { - // - // This is currently not being utilized yet but will be once session eviction logic is added. - // - (void) sessionEvictionHint; - - return mSecureSessions.CreateNewSecureSession(secureSessionType); + return mSecureSessions.CreateNewSecureSession(secureSessionType, sessionEvictionHint); } CHIP_ERROR SessionManager::InjectPaseSessionWithTestKey(SessionHolder & sessionHolder, uint16_t localSessionId, NodeId peerNodeId, @@ -555,6 +531,14 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & packetHea } Transport::SecureSession * secureSession = session.Value()->AsSecureSession(); + + if (!secureSession->IsDefunct() && !secureSession->IsActiveSession()) + { + ChipLogError(Inet, "Secure transport received message on a session in an invalid state (state = '%s')", + secureSession->GetStateStr()); + return; + } + // Decrypt and verify the message before message counter verification or any further processing. CryptoContext::NonceStorage nonce; // PASE Sessions use the undefined node ID of all zeroes, since there is no node ID to use @@ -751,13 +735,20 @@ Optional SessionManager::FindSecureSessionForNode(ScopedNodeId pe const Optional & type) { SecureSession * found = nullptr; + mSecureSessions.ForEachSession([&peerNodeId, &type, &found](auto session) { if (session->IsActiveSession() && session->GetPeer() == peerNodeId && (!type.HasValue() || type.Value() == session->GetSecureSessionType())) { - found = session; - return Loop::Break; + // + // Select the active session with the most recent activity to return back to the caller. + // + if ((found && (found->GetLastActivityTime() > session->GetLastActivityTime())) || !found) + { + found = session; + } } + return Loop::Continue; }); diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index d88031f670f4ac..d50f9f28172058 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -170,9 +170,7 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate Optional AllocateSession(Transport::SecureSession::Type secureSessionType, const ScopedNodeId & sessionEvictionHint); - void ExpirePairing(const SessionHandle & session); void ExpireAllPairings(const ScopedNodeId & node); - void ExpireAllPairingsForPeerExceptPending(const ScopedNodeId & node); void ExpireAllPairingsForFabric(FabricIndex fabric); void ExpireAllPASEPairings(); diff --git a/src/transport/tests/TestPeerConnections.cpp b/src/transport/tests/TestPeerConnections.cpp index b7227c52f287c5..10459afc456d3f 100644 --- a/src/transport/tests/TestPeerConnections.cpp +++ b/src/transport/tests/TestPeerConnections.cpp @@ -53,11 +53,8 @@ const NodeId kCasePeer1NodeId = 123; const NodeId kCasePeer2NodeId = 6; const FabricIndex kFabricIndex = 8; -const NodeId kPasePeerNodeId = kUndefinedNodeId; // PASE is always undefined - const CATValues kPeer1CATs = { { 0xABCD0001, 0xABCE0100, 0xABCD0020 } }; const CATValues kPeer2CATs = { { 0xABCD0012, kUndefinedCAT, kUndefinedCAT } }; -const CATValues kPeer3CATs; void TestBasicFunctionality(nlTestSuite * inSuite, void * inContext) { @@ -94,9 +91,9 @@ void TestBasicFunctionality(nlTestSuite * inSuite, void * inContext) peerCATs = optionalSession.Value()->AsSecureSession()->GetPeerCATs(); NL_TEST_ASSERT(inSuite, memcmp(&peerCATs, &kPeer2CATs, sizeof(CATValues)) == 0); - // This define guard will be needed when migrating SecureSessionTable to ObjectPool - //#if !CHIP_SYSTEM_CONFIG_POOL_USE_HEAP - // If not using a heap, we can fill the SecureSessionTable + // + // Fill up the session table. + // for (int i = 2; i < CHIP_CONFIG_SECURE_SESSION_POOL_SIZE; ++i) { sessions[i] = connections.CreateNewSecureSessionForTest(SecureSession::Type::kCASE, @@ -105,10 +102,6 @@ void TestBasicFunctionality(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, sessions[i].HasValue()); } - // Insufficient space for new connections. - optionalSession = connections.CreateNewSecureSessionForTest(SecureSession::Type::kPASE, 6, kLocalNodeId, kPasePeerNodeId, - kPeer3CATs, 5, kUndefinedFabricIndex, GetLocalMRPConfig()); - NL_TEST_ASSERT(inSuite, !optionalSession.HasValue()); //#endif System::Clock::Internal::SetSystemClockForTesting(realClock); } @@ -146,6 +139,18 @@ struct ExpiredCallInfo PeerAddress lastCallPeerAddress = PeerAddress::Uninitialized(); }; +int Initialize(void * apSuite) +{ + VerifyOrReturnError(chip::Platform::MemoryInit() == CHIP_NO_ERROR, FAILURE); + return SUCCESS; +} + +int Finalize(void * aContext) +{ + chip::Platform::MemoryShutdown(); + return SUCCESS; +} + } // namespace // clang-format off @@ -159,7 +164,7 @@ static const nlTest sTests[] = int TestPeerConnectionsFn(void) { - nlTestSuite theSuite = { "Transport-SecureSessionTable", &sTests[0], nullptr, nullptr }; + nlTestSuite theSuite = { "Transport-SecureSessionTable", &sTests[0], Initialize, Finalize }; nlTestRunner(&theSuite, nullptr); return nlTestRunnerStats(&theSuite); } diff --git a/src/transport/tests/TestSessionManager.cpp b/src/transport/tests/TestSessionManager.cpp index a665391af4a25a..f6a9f4d7d32b5c 100644 --- a/src/transport/tests/TestSessionManager.cpp +++ b/src/transport/tests/TestSessionManager.cpp @@ -660,7 +660,7 @@ static void RandomSessionIdAllocatorOffset(nlTestSuite * inSuite, SessionManager Transport::SecureSession::Type::kPASE, ScopedNodeId(NodeIdFromPAKEKeyId(kDefaultCommissioningPasscodeId), kUndefinedFabricIndex)); NL_TEST_ASSERT(inSuite, handle.HasValue()); - sessionManager.ExpirePairing(handle.Value()); + handle.Value()->AsSecureSession()->MarkForEviction(); } } @@ -716,7 +716,7 @@ void SessionAllocationTest(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, sessionId - prevSessionId == 1 || (sessionId == 1 && prevSessionId == 65535)); NL_TEST_ASSERT(inSuite, sessionId != 0); prevSessionId = sessionId; - sessionManager.ExpirePairing(handle.Value()); + handle.Value()->AsSecureSession()->MarkForEviction(); } // Verify that the allocator does not give colliding IDs. @@ -730,7 +730,7 @@ void SessionAllocationTest(nlTestSuite * inSuite, void * inContext) uint16_t sessionIds[numHandles]; for (size_t h = 0; h < numHandles; ++h) { - constexpr int maxOffset = 5000; + constexpr int maxOffset = 100; handles[h] = sessionManager.AllocateSession( Transport::SecureSession::Type::kPASE, ScopedNodeId(NodeIdFromPAKEKeyId(kDefaultCommissioningPasscodeId), kUndefinedFabricIndex)); @@ -758,13 +758,13 @@ void SessionAllocationTest(nlTestSuite * inSuite, void * inContext) { NL_TEST_ASSERT(inSuite, potentialCollision != sessionIds[h]); } - sessionManager.ExpirePairing(handle.Value()); + handle.Value()->AsSecureSession()->MarkForEviction(); } // Free our allocated sessions. for (size_t h = 0; h < numHandles; ++h) { - sessionManager.ExpirePairing(handles[h].Value()); + handles[h].Value()->AsSecureSession()->MarkForEviction(); } } diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index d447f25448a2ce..c752b5374afe17 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -40289,7 +40289,7 @@ class Test_TC_WNCV_4_5Suite : public TestCommand LogStep(3, "1b: TH Waits for 100ms-1s"); ListFreer listFreer; chip::app::Clusters::DelayCommands::Commands::WaitForMs::Type value; - value.ms = 500UL; + value.ms = 5000UL; return WaitForMs(kIdentityAlpha, value); } case 4: { @@ -40323,7 +40323,7 @@ class Test_TC_WNCV_4_5Suite : public TestCommand LogStep(7, "2b: TH Waits for 100ms-1s"); ListFreer listFreer; chip::app::Clusters::DelayCommands::Commands::WaitForMs::Type value; - value.ms = 500UL; + value.ms = 5000UL; return WaitForMs(kIdentityAlpha, value); } case 8: { diff --git a/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h b/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h index 853373946c8f61..d174dd39aef95c 100644 --- a/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h +++ b/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h @@ -60021,7 +60021,7 @@ class Test_TC_WNCV_4_5 : public TestCommandBridge { CHIP_ERROR Test1bThWaitsFor100ms1s_3() { chip::app::Clusters::DelayCommands::Commands::WaitForMs::Type value; - value.ms = 500UL; + value.ms = 5000UL; return WaitForMs("alpha", value); } @@ -60072,7 +60072,7 @@ class Test_TC_WNCV_4_5 : public TestCommandBridge { CHIP_ERROR Test2bThWaitsFor100ms1s_7() { chip::app::Clusters::DelayCommands::Commands::WaitForMs::Type value; - value.ms = 500UL; + value.ms = 5000UL; return WaitForMs("alpha", value); }