Fix crash if commissioning is canceled while waiting on an attestation delegate. (#32974)

When we extend the fail-safe after device attestation before calling into the
delegate, we ended up with a dangling mInvokeCancelFn after the command finished
and until CommissioningStageComplete().  That last would not happen until our
delegate called back into us to continue commissioning.

If an attempt was made to cancel commissioning during that time interval, we
would end up crashing.

Author: bzbarsky-apple

src/controller/CHIPDeviceController.cpp

@@ -1230,31 +1230,41 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(
 }
 
 void DeviceCommissioner::OnArmFailSafeExtendedForDeviceAttestation(
-    void * context, const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data)
+    void * context, const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType &)
 {
-    // If this function starts using "data", need to fix ExtendArmFailSafeForDeviceAttestation accordingly.
+    ChipLogProgress(Controller, "Successfully extended fail-safe timer to handle DA failure");
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
 
-    if (!commissioner->mDeviceBeingCommissioned)
+    // We have completed our command invoke, but we're not going to finish the
+    // commissioning step until our client examines the attestation
+    // information.  Clear out mInvokeCancelFn (which points at the
+    // CommandSender we just finished using) now, so it's not dangling.
+    commissioner->mInvokeCancelFn = nullptr;
+
+    commissioner->HandleDeviceAttestationCompleted();
+}
+
+void DeviceCommissioner::HandleDeviceAttestationCompleted()
+{
+    if (!mDeviceBeingCommissioned)
     {
         return;
     }
 
-    auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters();
+    auto & params                                                      = mDefaultCommissioner->GetCommissioningParameters();
     Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
     if (deviceAttestationDelegate)
     {
         ChipLogProgress(Controller, "Device attestation completed, delegating continuation to client");
-        deviceAttestationDelegate->OnDeviceAttestationCompleted(commissioner, commissioner->mDeviceBeingCommissioned,
-                                                                *commissioner->mAttestationDeviceInfo,
-                                                                commissioner->mAttestationResult);
+        deviceAttestationDelegate->OnDeviceAttestationCompleted(this, mDeviceBeingCommissioned, *mAttestationDeviceInfo,
+                                                                mAttestationResult);
     }
     else
     {
         ChipLogProgress(Controller, "Device attestation failed and no delegate set, failing commissioning");
         CommissioningDelegate::CommissioningReport report;
-        report.Set<AttestationErrorInfo>(commissioner->mAttestationResult);
-        commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
+        report.Set<AttestationErrorInfo>(mAttestationResult);
+        CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
     }
 }
 
@@ -1371,9 +1381,7 @@ void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials
 
     if (!waitForFailsafeExtension)
     {
-        // Callee does not use data argument.
-        const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType data;
-        OnArmFailSafeExtendedForDeviceAttestation(this, data);
+        HandleDeviceAttestationCompleted();
     }
 }
 

src/controller/CHIPDeviceController.h

@@ -956,6 +956,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     static void OnArmFailSafeExtendedForDeviceAttestation(
         void * context, const chip::app::Clusters::GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data);
     static void OnFailedToExtendedArmFailSafeDeviceAttestation(void * context, CHIP_ERROR error);
+    void HandleDeviceAttestationCompleted();
 
     static void OnICDManagementRegisterClientResponse(
         void * context, const app::Clusters::IcdManagement::Commands::RegisterClientResponse::DecodableType & data);

src/darwin/Framework/CHIPTests/MTRPairingTests.m

@@ -48,18 +48,29 @@
 // commissioning flows that have such a delegate.
 @interface NoOpAttestationDelegate : NSObject <MTRDeviceAttestationDelegate>
 @property (nonatomic) XCTestExpectation * expectation;
+@property (nonatomic) BOOL blockCommissioning;
 
 - (instancetype)initWithExpectation:(XCTestExpectation *)expectation;
+// If blockCommissioning is YES, this delegate will never proceed from
+// its attestation verification callback.
+- (instancetype)initWithExpectation:(XCTestExpectation *)expectation blockCommissioning:(BOOL)blockCommissioning;
 @end
 
 @implementation NoOpAttestationDelegate
+
 - (instancetype)initWithExpectation:(XCTestExpectation *)expectation
+{
+    return [self initWithExpectation:expectation blockCommissioning:NO];
+}
+
+- (instancetype)initWithExpectation:(XCTestExpectation *)expectation blockCommissioning:(BOOL)blockCommissioning;
 {
     if (!(self = [super init])) {
         return nil;
     }
 
     _expectation = expectation;
+    _blockCommissioning = blockCommissioning;
     return self;
 }
 
@@ -74,7 +85,10 @@ - (void)deviceAttestationCompletedForController:(MTRDeviceController *)controlle
     XCTAssertEqualObjects(attestationDeviceInfo.productID, @(0x8001));
     XCTAssertEqualObjects(attestationDeviceInfo.basicInformationVendorID, @(0xFFF1));
     XCTAssertEqualObjects(attestationDeviceInfo.basicInformationProductID, @(0x8000));
-    [controller continueCommissioningDevice:opaqueDeviceHandle ignoreAttestationFailure:NO error:nil];
+
+    if (!self.blockCommissioning) {
+        [controller continueCommissioningDevice:opaqueDeviceHandle ignoreAttestationFailure:NO error:nil];
+    }
 }
 
 @end
@@ -241,7 +255,7 @@ - (void)test004_PairWithAttestationDelegateFailsafeExtensionLong
     [self waitForExpectations:@[ expectation ] timeout:kTimeoutInSeconds];
 }
 
-- (void)doPairingAndWaitForProgress:(NSString *)trigger
+- (void)doPairingAndWaitForProgress:(NSString *)trigger attestationDelegate:(nullable id<MTRDeviceAttestationDelegate>)attestationDelegate
 {
     XCTestExpectation * expectation = [self expectationWithDescription:@"Trigger message seen"];
     expectation.assertForOverFulfill = NO;
@@ -251,9 +265,19 @@ - (void)doPairingAndWaitForProgress:(NSString *)trigger
         }
     });
 
+    XCTestExpectation * attestationExpectation;
+    if (attestationDelegate == nil) {
+        attestationExpectation = [self expectationWithDescription:@"Attestation delegate called"];
+        attestationDelegate = [[NoOpAttestationDelegate alloc] initWithExpectation:attestationExpectation];
+    }
+
+    // Make sure we exercise the codepath that has an attestation delegate and
+    // extends the fail-safe while waiting for that delegate.  And make sure our
+    // fail-safe extension is long enough that we actually trigger a fail-safe
+    // extension (so longer than the 1-minute default).
     __auto_type * controllerDelegate = [[MTRPairingTestControllerDelegate alloc] initWithExpectation:nil
-                                                                                 attestationDelegate:nil
-                                                                                   failSafeExtension:nil];
+                                                                                 attestationDelegate:attestationDelegate
+                                                                                   failSafeExtension:@(90)];
     [sController setDeviceControllerDelegate:controllerDelegate queue:dispatch_get_main_queue()];
     self.controllerDelegate = controllerDelegate;
 
@@ -264,13 +288,17 @@ - (void)doPairingAndWaitForProgress:(NSString *)trigger
     XCTAssertNil(error);
 
     [self waitForExpectations:@[ expectation ] timeout:kPairingTimeoutInSeconds];
+
+    if (attestationExpectation) {
+        [self waitForExpectations:@[ attestationExpectation ] timeout:kTimeoutInSeconds];
+    }
     MTRSetLogCallback(0, nil);
 }
 
-- (void)doPairingTestAfterCancellationAtProgress:(NSString *)trigger
+- (void)doPairingTestAfterCancellationAtProgress:(NSString *)trigger attestationDelegate:(nullable id<MTRDeviceAttestationDelegate>)attestationDelegate
 {
     // Run pairing up and wait for the trigger
-    [self doPairingAndWaitForProgress:trigger];
+    [self doPairingAndWaitForProgress:trigger attestationDelegate:attestationDelegate];
 
     // Call StopPairing and wait for the commissioningComplete callback
     XCTestExpectation * expectation = [self expectationWithDescription:@"commissioningComplete delegate method called"];
@@ -289,6 +317,11 @@ - (void)doPairingTestAfterCancellationAtProgress:(NSString *)trigger
     [self doPairingTestWithAttestationDelegate:nil failSafeExtension:nil];
 }
 
+- (void)doPairingTestAfterCancellationAtProgress:(NSString *)trigger
+{
+    [self doPairingTestAfterCancellationAtProgress:trigger attestationDelegate:nil];
+}
+
 - (void)test005_pairingAfterCancellation_ReadCommissioningInfo
 {
     // @"Sending read request for commissioning information"
@@ -306,4 +339,16 @@ - (void)test007_pairingAfterCancellation_FindOperational
     [self doPairingTestAfterCancellationAtProgress:@"FindOrEstablishSession:"];
 }
 
+- (void)test008_pairingAfterCancellation_DeviceAttestationVerification
+{
+    // Cancel pairing while we are waiting for our client to decide what to do
+    // with the attestation information we got.
+    XCTestExpectation * attestationExpectation = [self expectationWithDescription:@"Blocking attestation delegate called"];
+    __auto_type * attestationDelegate = [[NoOpAttestationDelegate alloc] initWithExpectation:attestationExpectation blockCommissioning:YES];
+
+    [self doPairingTestAfterCancellationAtProgress:@"Successfully extended fail-safe timer to handle DA failure" attestationDelegate:attestationDelegate];
+
+    [self waitForExpectations:@[ attestationExpectation ] timeout:kTimeoutInSeconds];
+}
+
 @end