Better handling for the multiple fabric tables live during controller…

… startup. (#19578) We should be using fabric indices with the right fabric table, not assuming they have matching fabric indices.
project-chip · Jun 14, 2022 · 1a2c719 · 1a2c719
1 parent 104d1c4
commit 1a2c719
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 18 deletions.
diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm
@@ -730,7 +730,9 @@ @implementation CHIPDeviceController (InternalMethods)
     return fabricIdx;
 }
 
-- (CHIP_ERROR)isRunningOnFabric:(chip::FabricInfo *)fabric isRunning:(BOOL *)isRunning
+- (CHIP_ERROR)isRunningOnFabric:(chip::FabricTable *)fabricTable
+                    fabricIndex:(chip::FabricIndex)fabricIndex
+                      isRunning:(BOOL *)isRunning
 {
     if (![self isRunning]) {
         *isRunning = NO;
@@ -743,26 +745,26 @@ - (CHIP_ERROR)isRunningOnFabric:(chip::FabricInfo *)fabric isRunning:(BOOL *)isR
         return CHIP_ERROR_INCORRECT_STATE;
     }
 
-    if (ourFabric->GetFabricId() != fabric->GetFabricId()) {
+    chip::FabricInfo * otherFabric = fabricTable->FindFabricWithIndex(fabricIndex);
+    if (!otherFabric) {
+        // Also surprising!
+        return CHIP_ERROR_INCORRECT_STATE;
+    }
+
+    if (ourFabric->GetFabricId() != otherFabric->GetFabricId()) {
         *isRunning = NO;
         return CHIP_NO_ERROR;
     }
 
-    // "fabric" comes from a different fabric table for the moment, but it's a
-    // fabric table that is: (1) readonly, (2) has stack lifetime, and (3) uses
-    // the same storage as _cppCommissioner->GetFabricTable(), so it turns out
-    // that in practice they have the same fabric indices and this logic is OK.
-    //
-    // TODO: stop relying on that.
-    const chip::FabricTable * fabricTable = _cppCommissioner->GetFabricTable();
-    if (!fabricTable) {
+    const chip::FabricTable * ourFabricTable = _cppCommissioner->GetFabricTable();
+    if (!ourFabricTable) {
         // Surprising as well!
         return CHIP_ERROR_INCORRECT_STATE;
     }
 
     chip::Crypto::P256PublicKey ourRootPublicKey, otherRootPublicKey;
-    ReturnErrorOnFailure(fabricTable->FetchRootPubkey(ourFabric->GetFabricIndex(), ourRootPublicKey));
-    ReturnErrorOnFailure(fabricTable->FetchRootPubkey(fabric->GetFabricIndex(), otherRootPublicKey));
+    ReturnErrorOnFailure(ourFabricTable->FetchRootPubkey(ourFabric->GetFabricIndex(), ourRootPublicKey));
+    ReturnErrorOnFailure(fabricTable->FetchRootPubkey(otherFabric->GetFabricIndex(), otherRootPublicKey));
 
     *isRunning = (ourRootPublicKey.Matches(otherRootPublicKey));
     return CHIP_NO_ERROR;

diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController_Internal.h b/src/darwin/Framework/CHIP/CHIPDeviceController_Internal.h
@@ -33,7 +33,7 @@
 @class MatterControllerFactory;
 
 namespace chip {
-class FabricInfo;
+class FabricTable;
 } // namespace chip
 
 NS_ASSUME_NONNULL_BEGIN
@@ -66,16 +66,19 @@ NS_ASSUME_NONNULL_BEGIN
 - (instancetype)initWithFactory:(MatterControllerFactory *)factory queue:(dispatch_queue_t)queue;
 
 /**
- * Check whether this controller is running on the given fabric.  The FabricInfo
- * might be from a one-off fabric table.  This method MUST be called from the
- * Matter work queue.
+ * Check whether this controller is running on the given fabric, as represented
+ * by the provided FabricTable and fabric index.  The provided fabric table may
+ * not be the same as the fabric table this controller is using. This method
+ * MUST be called from the Matter work queue.
  *
  * Might return failure, in which case we don't know whether it's running on the
  * given fabric.  Otherwise it will set *isRunning to the right boolean value.
  *
  * Only MatterControllerFactory should be calling this.
  */
-- (CHIP_ERROR)isRunningOnFabric:(chip::FabricInfo *)fabric isRunning:(BOOL *)isRunning;
+- (CHIP_ERROR)isRunningOnFabric:(chip::FabricTable *)fabricTable
+                    fabricIndex:(chip::FabricIndex)fabricIndex
+                      isRunning:(BOOL *)isRunning;
 
 @end
 

diff --git a/src/darwin/Framework/CHIP/MatterControllerFactory.mm b/src/darwin/Framework/CHIP/MatterControllerFactory.mm
@@ -305,7 +305,8 @@ - (CHIPDeviceController * _Nullable)startControllerOnExistingFabric:(CHIPDeviceC
 
         for (CHIPDeviceController * existing in _controllers) {
             BOOL isRunning = YES; // assume the worst
-            if ([existing isRunningOnFabric:fabric isRunning:&isRunning] != CHIP_NO_ERROR) {
+            if ([existing isRunningOnFabric:&fabricTable fabricIndex:fabric->GetFabricIndex() isRunning:&isRunning]
+                != CHIP_NO_ERROR) {
                 CHIP_LOG_ERROR("Can't tell what fabric a controller is running on.  Not safe to start.");
                 return;
             }