From 3400632bb8076d7c340bbf0ed9783e296f9b940e Mon Sep 17 00:00:00 2001 From: Jean-Francois Penven <67962328+jepenven-silabs@users.noreply.github.com> Date: Tue, 8 Mar 2022 14:57:39 -0500 Subject: [PATCH] [Group] Fabric removal sync (#15874) * Fabric removal sync --- src/app/server/Server.cpp | 4 ++ src/app/server/Server.h | 35 +++++++++++ .../CHIPDeviceControllerFactory.cpp | 4 ++ src/controller/CHIPDeviceControllerFactory.h | 36 +++++++++++ src/credentials/FabricTable.cpp | 16 +++++ src/credentials/FabricTable.h | 4 ++ src/credentials/GroupDataProvider.h | 2 +- src/transport/GroupPeerMessageCounter.cpp | 59 ++++++++++++++----- src/transport/GroupPeerMessageCounter.h | 3 + src/transport/SessionManager.cpp | 11 ++++ src/transport/SessionManager.h | 5 ++ .../tests/TestGroupMessageCounter.cpp | 24 +++++++- 12 files changed, 187 insertions(+), 16 deletions(-) diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index da6d876be031db..0f6666fed07297 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -184,6 +184,10 @@ CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint err = mSessions.Init(&DeviceLayer::SystemLayer(), &mTransports, &mMessageCounterManager, &mDeviceStorage, &GetFabricTable()); SuccessOrExit(err); + err = mFabricDelegate.Init(&mSessions); + SuccessOrExit(err); + mFabrics.AddFabricDelegate(&mFabricDelegate); + err = mExchangeMgr.Init(&mSessions); SuccessOrExit(err); err = mMessageCounterManager.Init(&mExchangeMgr); diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 03dffb5f0175e3..a0aab30f1e7e3d 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -170,6 +170,40 @@ class Server ServerTransportMgr * mTransports; }; + class ServerFabricDelegate final : public FabricTableDelegate + { + public: + ServerFabricDelegate() {} + + CHIP_ERROR Init(SessionManager * sessionManager) + { + VerifyOrReturnError(sessionManager != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + + mSessionManager = sessionManager; + return CHIP_NO_ERROR; + }; + + void OnFabricDeletedFromStorage(CompressedFabricId compressedId, FabricIndex fabricIndex) override + { + (void) compressedId; + if (mSessionManager != nullptr) + { + mSessionManager->FabricRemoved(fabricIndex); + } + Credentials::GroupDataProvider * groupDataProvider = Credentials::GetGroupDataProvider(); + if (groupDataProvider != nullptr) + { + groupDataProvider->RemoveFabric(fabricIndex); + } + }; + void OnFabricRetrievedFromStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } + + void OnFabricPersistedToStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } + + private: + SessionManager * mSessionManager = nullptr; + }; + #if CONFIG_NETWORK_LAYER_BLE Ble::BleLayer * mBleLayer = nullptr; #endif @@ -198,6 +232,7 @@ class Server Credentials::GroupDataProviderImpl mGroupsProvider; app::DefaultAttributePersistenceProvider mAttributePersister; GroupDataProviderListener mListener; + ServerFabricDelegate mFabricDelegate; Access::AccessControl mAccessControl; diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 4bcd34ea55d9ac..4fc754997f3ee4 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -146,6 +146,10 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) ReturnErrorOnFailure(stateParams.fabricTable->Init(params.fabricIndependentStorage)); + auto delegate = chip::Platform::MakeUnique(stateParams.sessionMgr); + ReturnErrorOnFailure(stateParams.fabricTable->AddFabricDelegate(delegate.get())); + delegate.release(); + ReturnErrorOnFailure(stateParams.sessionMgr->Init(stateParams.systemLayer, stateParams.transportMgr, stateParams.messageCounterManager, params.fabricIndependentStorage, stateParams.fabricTable)); diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index 723f7ae97b5bc3..0477aa3d4a69cd 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -31,6 +31,7 @@ #include #include +#include #include namespace chip { @@ -137,6 +138,41 @@ class DeviceControllerFactory // void ReleaseSystemState() { mSystemState->Release(); } + class ControllerFabricDelegate final : public FabricTableDelegate + { + public: + ControllerFabricDelegate() {} + ControllerFabricDelegate(SessionManager * sessionManager) : FabricTableDelegate(true), mSessionManager(sessionManager) {} + + CHIP_ERROR Init(SessionManager * sessionManager) + { + VerifyOrReturnError(sessionManager != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + + mSessionManager = sessionManager; + return CHIP_NO_ERROR; + }; + + void OnFabricDeletedFromStorage(CompressedFabricId compressedId, FabricIndex fabricIndex) override + { + if (mSessionManager != nullptr) + { + mSessionManager->FabricRemoved(fabricIndex); + } + Credentials::GroupDataProvider * groupDataProvider = Credentials::GetGroupDataProvider(); + if (groupDataProvider != nullptr) + { + groupDataProvider->RemoveFabric(fabricIndex); + } + }; + + void OnFabricRetrievedFromStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } + + void OnFabricPersistedToStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } + + private: + SessionManager * mSessionManager = nullptr; + }; + private: DeviceControllerFactory(){}; void PopulateInitParams(ControllerInitParams & controllerParams, const SetupParams & params); diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index a60f3b93ea0cd9..6d1187b079c51c 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -401,6 +401,20 @@ CHIP_ERROR FabricInfo::MatchDestinationID(const ByteSpan & targetDestinationId, return CHIP_ERROR_CERT_NOT_TRUSTED; } +FabricTable::~FabricTable() +{ + FabricTableDelegate * delegate = mDelegate; + while (delegate) + { + FabricTableDelegate * temp = delegate->mNext; + if (delegate->mOwnedByFabricTable) + { + chip::Platform::Delete(delegate); + } + delegate = temp; + } +} + void FabricTable::ReleaseFabricIndex(FabricIndex fabricIndex) { FabricInfo * fabric = FindFabricWithIndex(fabricIndex); @@ -629,6 +643,7 @@ CHIP_ERROR FabricTable::Delete(FabricIndex index) mFabricCount--; } ChipLogProgress(Discovery, "Fabric (%d) deleted. Calling OnFabricDeletedFromStorage", index); + FabricTableDelegate * delegate = mDelegate; while (delegate) { @@ -651,6 +666,7 @@ void FabricTable::DeleteAllFabrics() CHIP_ERROR FabricTable::Init(PersistentStorageDelegate * storage) { VerifyOrReturnError(storage != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + mStorage = storage; ChipLogDetail(Discovery, "Init fabric pairing table with server storage"); diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index a394d129b9813a..8a9ea3b4d3b813 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -274,6 +274,7 @@ class DLL_EXPORT FabricTableDelegate friend class FabricTable; public: + FabricTableDelegate(bool ownedByFabricTable = false) : mOwnedByFabricTable(ownedByFabricTable) {} virtual ~FabricTableDelegate() {} /** * Gets called when a fabric is deleted from KVS store. @@ -292,6 +293,7 @@ class DLL_EXPORT FabricTableDelegate private: FabricTableDelegate * mNext = nullptr; + bool mOwnedByFabricTable = false; }; /** @@ -365,6 +367,8 @@ class DLL_EXPORT FabricTable { public: FabricTable() { Reset(); } + ~FabricTable(); + CHIP_ERROR Store(FabricIndex index); CHIP_ERROR LoadFromStorage(FabricInfo * info); diff --git a/src/credentials/GroupDataProvider.h b/src/credentials/GroupDataProvider.h index 8ae5a80f947417..3bc44a84f85fe8 100644 --- a/src/credentials/GroupDataProvider.h +++ b/src/credentials/GroupDataProvider.h @@ -166,7 +166,7 @@ class GroupDataProvider /** * Callback invoked when an existing group is removed. * - * @param[in] removed_state GroupInfo structure of the removed group. + * @param[in] old_group GroupInfo structure of the removed group. */ virtual void OnGroupRemoved(FabricIndex fabric_index, const GroupInfo & old_group) = 0; }; diff --git a/src/transport/GroupPeerMessageCounter.cpp b/src/transport/GroupPeerMessageCounter.cpp index d6e1287a599e5f..51ad13a47c6554 100644 --- a/src/transport/GroupPeerMessageCounter.cpp +++ b/src/transport/GroupPeerMessageCounter.cpp @@ -151,20 +151,7 @@ CHIP_ERROR GroupPeerTable::RemovePeer(FabricIndex fabricIndex, NodeId nodeId, bo { if (mGroupFabrics[fabricIt].mDataPeerCount == 0 && mGroupFabrics[fabricIt].mControlPeerCount == 0) { - mGroupFabrics[fabricIt].mFabricIndex = kUndefinedFabricIndex; - // To maintain logic integrity Fabric array cannot have empty slot in between data - // Find the last non empty element - for (uint32_t i = CHIP_CONFIG_MAX_FABRICS - 1; i > fabricIt; i--) - { - if (mGroupFabrics[i].mFabricIndex != kUndefinedFabricIndex) - { - // Logic works since all buffer are static - // move it up front - new (&mGroupFabrics[fabricIt]) GroupFabric(mGroupFabrics[i]); - new (&mGroupFabrics[i]) GroupFabric(); - break; - } - } + RemoveAndCompactFabric(fabricIt); } } @@ -172,6 +159,28 @@ CHIP_ERROR GroupPeerTable::RemovePeer(FabricIndex fabricIndex, NodeId nodeId, bo return err; } +CHIP_ERROR GroupPeerTable::FabricRemoved(FabricIndex fabricIndex) +{ + CHIP_ERROR err = CHIP_ERROR_NOT_FOUND; + + if (fabricIndex == kUndefinedFabricIndex) + { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + for (uint32_t it = 0; it < CHIP_CONFIG_MAX_FABRICS; it++) + { + if (fabricIndex == mGroupFabrics[it].mFabricIndex) + { + RemoveAndCompactFabric(it); + return CHIP_NO_ERROR; + } + } + + // Cannot find Fabric to remove + return err; +} + bool GroupPeerTable::RemoveSpecificPeer(GroupSender * list, NodeId nodeId, uint32_t size) { bool removed = false; @@ -222,6 +231,28 @@ void GroupPeerTable::CompactPeers(GroupSender * list, uint32_t size) } } +void GroupPeerTable::RemoveAndCompactFabric(uint32_t tableIndex) +{ + if (tableIndex >= CHIP_CONFIG_MAX_FABRICS) + { + return; + } + mGroupFabrics[tableIndex].mFabricIndex = kUndefinedFabricIndex; + // To maintain logic integrity Fabric array cannot have empty slot in between data + // Find the last non empty element + for (uint32_t i = CHIP_CONFIG_MAX_FABRICS - 1; i > tableIndex; i--) + { + if (mGroupFabrics[i].mFabricIndex != kUndefinedFabricIndex) + { + // Logic works since all buffer are static + // move it up front + new (&mGroupFabrics[tableIndex]) GroupFabric(mGroupFabrics[i]); + new (&mGroupFabrics[i]) GroupFabric(); + break; + } + } +} + GroupOutgoingCounters::GroupOutgoingCounters(chip::PersistentStorageDelegate * storage_delegate) { Init(storage_delegate); diff --git a/src/transport/GroupPeerMessageCounter.h b/src/transport/GroupPeerMessageCounter.h index a77dcaf41fd423..6c2b278fcfa6b8 100644 --- a/src/transport/GroupPeerMessageCounter.h +++ b/src/transport/GroupPeerMessageCounter.h @@ -62,10 +62,13 @@ class GroupPeerTable // Used in case of MCSP failure CHIP_ERROR RemovePeer(FabricIndex fabricIndex, NodeId nodeId, bool isControl); + CHIP_ERROR FabricRemoved(FabricIndex fabricIndex); + // Protected for Unit Tests inheritance protected: bool RemoveSpecificPeer(GroupSender * list, NodeId nodeId, uint32_t size); void CompactPeers(GroupSender * list, uint32_t size); + void RemoveAndCompactFabric(uint32_t tableIndex); GroupFabric mGroupFabrics[CHIP_CONFIG_MAX_FABRICS]; }; diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index 58c5ee2ace5ad7..2ccbe5bf3ab778 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -117,6 +117,17 @@ void SessionManager::Shutdown() mCB = nullptr; } +/** + * @brief Notification that a fabric was removed. + * This function doesn't call ExpireAllPairingsForFabric + * since the CASE session might still be open to send a response + * on the removed fabric. + */ +void SessionManager::FabricRemoved(FabricIndex fabricIndex) +{ + mGroupPeerMsgCounter.FabricRemoved(fabricIndex); +} + CHIP_ERROR SessionManager::PrepareMessage(const SessionHandle & sessionHandle, PayloadHeader & payloadHeader, System::PacketBufferHandle && message, EncryptedPacketBufferHandle & preparedMessage) { diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 554a009ddbfe76..f96a1367a3b4d1 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -197,6 +197,11 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate */ void Shutdown(); + /** + * @brief Notification that a fabric was removed. + */ + void FabricRemoved(FabricIndex fabricIndex); + TransportMgrBase * GetTransportManager() const { return mTransportMgr; } /** diff --git a/src/transport/tests/TestGroupMessageCounter.cpp b/src/transport/tests/TestGroupMessageCounter.cpp index 726ed5c2683392..6ec7b4d66f86af 100644 --- a/src/transport/tests/TestGroupMessageCounter.cpp +++ b/src/transport/tests/TestGroupMessageCounter.cpp @@ -134,7 +134,7 @@ void RemovePeerTest(nlTestSuite * inSuite, void * inContext) FabricIndex fabricIndex = 1; CHIP_ERROR err = CHIP_NO_ERROR; chip::Transport::PeerMessageCounter * counter = nullptr; - chip::Transport::GroupPeerTable mGroupPeerMsgCounter; + TestGroupPeerTable mGroupPeerMsgCounter; // Fill table up (max fabric and mac peer) for (uint32_t it = 0; it < CHIP_CONFIG_MAX_FABRICS; it++) @@ -165,6 +165,28 @@ void RemovePeerTest(nlTestSuite * inSuite, void * inContext) // Try re-adding the previous peer without any error err = mGroupPeerMsgCounter.FindOrAddPeer(99, 99, true, counter); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = mGroupPeerMsgCounter.FindOrAddPeer(104, 99, true, counter); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = mGroupPeerMsgCounter.FindOrAddPeer(105, 99, true, counter); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = mGroupPeerMsgCounter.FindOrAddPeer(106, 99, true, counter); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // Fabric removal test + err = mGroupPeerMsgCounter.FabricRemoved(123); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_NOT_FOUND); + + err = mGroupPeerMsgCounter.FabricRemoved(99); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = mGroupPeerMsgCounter.FabricRemoved(99); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_NOT_FOUND); + + // Verify that the Fabric List was compacted. + NL_TEST_ASSERT(inSuite, 106 == mGroupPeerMsgCounter.GetFabricIndexAt(0)); } void PeerRetrievalTest(nlTestSuite * inSuite, void * inContext)