From 1335460aa9c061dc6eabc06132e273310f27f203 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 6 Apr 2023 20:40:13 -0400 Subject: [PATCH] Fix restart of controllers that use fabric table storage. (#26004) Right now controller shutdown clears out the fabric table entry in memory, but not in storage. After that memory and storage are out of sync, and various things that rely on the storage (like starting a new controller for the same fabric without providing all the certs and whatnot) end up failing. The fix is to make the Forget() call in controller shutdown configurable, so that consumers who rely on the storage can skip it. --- src/controller/CHIPDeviceController.cpp | 11 +++++++--- src/controller/CHIPDeviceController.h | 13 ++++++++++++ .../CHIPDeviceControllerFactory.cpp | 1 + src/controller/CHIPDeviceControllerFactory.h | 11 ++++++++++ .../Framework/CHIP/MTRDeviceController.mm | 5 +++++ .../Framework/CHIPTests/MTRControllerTests.m | 21 +++++++++++++++++++ .../Framework/CHIPTests/MTRTestStorage.h | 1 + .../Framework/CHIPTests/MTRTestStorage.m | 5 +++++ 8 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index dd776940654462..2ae83b320839db 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -143,6 +143,8 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) mSystemState = params.systemState->Retain(); mState = State::Initialized; + mRemoveFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown; + if (GetFabricIndex() != kUndefinedFabricIndex) { ChipLogProgress(Controller, @@ -348,10 +350,13 @@ void DeviceController::Shutdown() // existing sessions too? mSystemState->SessionMgr()->ExpireAllSessionsForFabric(mFabricIndex); - FabricTable * fabricTable = mSystemState->Fabrics(); - if (fabricTable != nullptr) + if (mRemoveFromFabricTableOnShutdown) { - fabricTable->Forget(mFabricIndex); + FabricTable * fabricTable = mSystemState->Fabrics(); + if (fabricTable != nullptr) + { + fabricTable->Forget(mFabricIndex); + } } } diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 04c24db25a0eea..c3f6cd3ff49f41 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -128,6 +128,17 @@ struct ControllerInitParams // bool enableServerInteractions = false; + /** + * Controls whether shutdown of the controller removes the corresponding + * entry from the fabric table. For now the removal is just from the + * in-memory table, not from storage, which means that after controller + * shutdown the storage and the in-memory fabric table will be out of sync. + * This is acceptable for implementations that don't actually store any of + * the fabric table information, but if someone wants a true removal at some + * point another option will need to be added here. + */ + bool removeFromFabricTableOnShutdown = true; + chip::VendorId controllerVendorId; }; @@ -330,6 +341,8 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController FabricIndex mFabricIndex = kUndefinedFabricIndex; + bool mRemoveFromFabricTableOnShutdown = true; + // TODO(cecille): Make this configuarable. static constexpr int kMaxCommissionableNodes = 10; Dnssd::DiscoveredNodeData mCommissionableNodes[kMaxCommissionableNodes]; diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 7180566e36507c..4cc20bfc3f10a7 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -286,6 +286,7 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll controllerParams.controllerICAC = params.controllerICAC; controllerParams.controllerRCAC = params.controllerRCAC; controllerParams.permitMultiControllerFabrics = params.permitMultiControllerFabrics; + controllerParams.removeFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown; controllerParams.systemState = mSystemState; controllerParams.controllerVendorId = params.controllerVendorId; diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index fd77c3779017a9..1fa0563b6bd8e1 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -90,6 +90,17 @@ struct SetupParams // bool enableServerInteractions = false; + /** + * Controls whether shutdown of the controller removes the corresponding + * entry from the fabric table. For now the removal is just from the + * in-memory table, not from storage, which means that after controller + * shutdown the storage and the in-memory fabric table will be out of sync. + * This is acceptable for implementations that don't actually store any of + * the fabric table information, but if someone wants a true removal at some + * point another option will need to be added here. + */ + bool removeFromFabricTableOnShutdown = true; + Credentials::DeviceAttestationVerifier * deviceAttestationVerifier = nullptr; CommissioningDelegate * defaultCommissioner = nullptr; }; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index a436003d7605b9..2192e9bda46e81 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -336,6 +336,11 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams } commissionerParams.controllerVendorId = static_cast([startupParams.vendorID unsignedShortValue]); commissionerParams.enableServerInteractions = startupParams.advertiseOperational; + // We don't want to remove things from the fabric table on controller + // shutdown, since our controller setup depends on being able to fetch + // fabric information for the relevant fabric indices on controller + // bring-up. + commissionerParams.removeFromFabricTableOnShutdown = false; commissionerParams.deviceAttestationVerifier = _factory.deviceAttestationVerifier; auto & factory = chip::Controller::DeviceControllerFactory::GetInstance(); diff --git a/src/darwin/Framework/CHIPTests/MTRControllerTests.m b/src/darwin/Framework/CHIPTests/MTRControllerTests.m index 06dfb7a2beb3b7..d28beef64ccb42 100644 --- a/src/darwin/Framework/CHIPTests/MTRControllerTests.m +++ b/src/darwin/Framework/CHIPTests/MTRControllerTests.m @@ -391,8 +391,29 @@ - (void)testControllerStartControllersOnTwoFabricIds XCTAssertNotNil(controller2); XCTAssertTrue([controller2 isRunning]); + // Verify that we can't start on an existing fabric while we have a + // controller on that fabric already. XCTAssertNil([factory createControllerOnExistingFabric:params2 error:nil]); + // Now test restarting the controller on the first fabric while the + // controller on the second fabric is still running. + [controller1 shutdown]; + XCTAssertFalse([controller1 isRunning]); + + controller1 = [factory createControllerOnExistingFabric:params1 error:nil]; + XCTAssertNotNil(controller1); + XCTAssertTrue([controller1 isRunning]); + + // Now test restarting the controller on the second fabric while the + // controller on the first fabric is still running. + [controller2 shutdown]; + XCTAssertFalse([controller2 isRunning]); + + controller2 = [factory createControllerOnExistingFabric:params2 error:nil]; + XCTAssertNotNil(controller2); + XCTAssertTrue([controller2 isRunning]); + + // Shut down everything. [controller1 shutdown]; XCTAssertFalse([controller1 isRunning]); diff --git a/src/darwin/Framework/CHIPTests/MTRTestStorage.h b/src/darwin/Framework/CHIPTests/MTRTestStorage.h index ba5e386be23b8c..8e0a5d6859180a 100644 --- a/src/darwin/Framework/CHIPTests/MTRTestStorage.h +++ b/src/darwin/Framework/CHIPTests/MTRTestStorage.h @@ -23,6 +23,7 @@ NS_ASSUME_NONNULL_BEGIN - (nullable NSData *)storageDataForKey:(NSString *)key; - (BOOL)setStorageData:(NSData *)value forKey:(NSString *)key; - (BOOL)removeStorageDataForKey:(NSString *)key; +- (NSString *)dumpStorageToString; @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIPTests/MTRTestStorage.m b/src/darwin/Framework/CHIPTests/MTRTestStorage.m index b03b3509891d39..8d80bd59d8b381 100644 --- a/src/darwin/Framework/CHIPTests/MTRTestStorage.m +++ b/src/darwin/Framework/CHIPTests/MTRTestStorage.m @@ -52,4 +52,9 @@ - (instancetype)init return self; } +- (NSString *)dumpStorageToString +{ + return [NSString stringWithFormat:@"%@", _values]; +} + @end