Address feedback from #22197 (#22175)

* Address feedback from #22197 * Allocating and deallocating partialDacVerifier before assignment * Avoiding returning a BOOL from onNocChainGenerationComplete * Allocating partialDacVerifier in initWithFactory and checking if allocation failed * Handling potentially dangling *cppCommissioner, dispatching GetCommissioningParams call on chipWorkQueue * Update src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm Co-authored-by: Boris Zbarsky <[email protected]> * Update src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm Co-authored-by: Boris Zbarsky <[email protected]> * Update src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Sep 1, 2022 · e9984f8 · e9984f8
1 parent 7920d88
commit e9984f8
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 13 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -60,6 +60,7 @@
 static NSString * const kErrorOperationalCredentialsInit = @"Init failure while creating operational credentials delegate";
 static NSString * const kErrorOperationalKeypairInit = @"Init failure while creating operational keypair bridge";
 static NSString * const kErrorPairingInit = @"Init failure while creating a pairing delegate";
+static NSString * const kErrorPartialDacVerifierInit = @"Init failure while creating a partial DAC verifier";
 static NSString * const kErrorPairDevice = @"Failure while pairing the device";
 static NSString * const kErrorUnpairDevice = @"Failure while unpairing the device";
 static NSString * const kErrorStopPairing = @"Failure while trying to stop the pairing process";
@@ -106,6 +107,11 @@ - (instancetype)initWithFactory:(MTRControllerFactory *)factory queue:(dispatch_
             return nil;
         }
 
+        _partialDACVerifier = new chip::Credentials::PartialDACVerifier();
+        if ([self checkForInitError:(_partialDACVerifier != nullptr) logMsg:kErrorPartialDacVerifierInit]) {
+            return nil;
+        }
+
         _operationalCredentialsDelegate = new MTROperationalCredentialsDelegate();
         if ([self checkForInitError:(_operationalCredentialsDelegate != nullptr) logMsg:kErrorOperationalCredentialsInit]) {
             return nil;
@@ -145,6 +151,9 @@ - (void)shutDownCppController
         _cppCommissioner->Shutdown();
         delete _cppCommissioner;
         _cppCommissioner = nullptr;
+        if (_operationalCredentialsDelegate != nil) {
+            _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr);
+        }
     }
 }
 
@@ -160,6 +169,11 @@ - (void)cleanup
         _operationalCredentialsDelegate = nullptr;
     }
 
+    if (_partialDACVerifier) {
+        delete _partialDACVerifier;
+        _partialDACVerifier = nullptr;
+    }
+
     if (_pairingDelegateBridge) {
         delete _pairingDelegateBridge;
         _pairingDelegateBridge = nullptr;

diff --git a/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h b/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h
@@ -22,6 +22,9 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+typedef void (^MTRNOCChainGenerationCompleteHandler)(NSData * operationalCertificate, NSData * intermediateCertificate,
+    NSData * rootCertificate, NSData * _Nullable ipk, NSNumber * _Nullable adminSubject, NSError * __autoreleasing * error);
+
 @protocol MTRNOCChainIssuer <NSObject>
 @required
 
@@ -43,9 +46,7 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)onNOCChainGenerationNeeded:(CSRInfo *)csrInfo
                    attestationInfo:(AttestationInfo *)attestationInfo
-      onNOCChainGenerationComplete:(void (^)(NSData * operationalCertificate, NSData * intermediateCertificate,
-                                       NSData * rootCertificate, NSData * ipk, NSNumber * adminSubject,
-                                       NSError * __autoreleasing * error))onNOCChainGenerationComplete;
+      onNOCChainGenerationComplete:(MTRNOCChainGenerationCompleteHandler)onNOCChainGenerationComplete;
 
 @end
 

diff --git a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h
@@ -57,7 +57,10 @@ class MTROperationalCredentialsDelegate : public chip::Controller::OperationalCr
     void SetDeviceID(chip::NodeId deviceId) { mDeviceBeingPaired = deviceId; }
     void ResetDeviceID() { mDeviceBeingPaired = chip::kUndefinedNodeId; }
 
-    void SetDeviceCommissioner(chip::Controller::DeviceCommissioner * cppCommissioner) { mCppCommissioner = cppCommissioner; }
+    void SetDeviceCommissioner(chip::Controller::DeviceCommissioner * _Nullable cppCommissioner)
+    {
+        mCppCommissioner = cppCommissioner;
+    }
 
     chip::Optional<chip::Controller::CommissioningParameters> GetCommissioningParameters()
     {

diff --git a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm
@@ -147,6 +147,7 @@
     const chip::ByteSpan & DAC, const chip::ByteSpan & PAI,
     chip::Callback::Callback<chip::Controller::OnNOCChainGeneration> * onCompletion)
 {
+    VerifyOrReturnError(mCppCommissioner != nullptr, CHIP_ERROR_INCORRECT_STATE);
     mOnNOCCompletionCallback = onCompletion;
 
     TLVReader reader;
@@ -178,12 +179,8 @@
     chip::ByteSpan firmwareInfoSpan;
     chip::Credentials::DeviceAttestationVendorReservedDeconstructor vendorReserved;
 
-    __block chip::Optional<chip::Controller::CommissioningParameters> commissioningParameters;
-    // Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of
-    // commissioning
-    dispatch_sync(mChipWorkQueue, ^{
-        commissioningParameters = mCppCommissioner->GetCommissioningParameters();
-    });
+    chip::Optional<chip::Controller::CommissioningParameters> commissioningParameters
+        = mCppCommissioner->GetCommissioningParameters();
     VerifyOrReturnError(commissioningParameters.HasValue(), CHIP_ERROR_INCORRECT_STATE);
 
     // Attestation Elements, nonce and signature will have a value in Commissioning Params as the CSR needs a signature or else we
@@ -231,9 +228,11 @@
         return;
     }
 
-    // use ipk and adminSubject from CommissioningParameters if not passed in.
-    // Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of
-    // commissioning
+    if (mCppCommissioner == nullptr) {
+        setNSError(CHIP_ERROR_INCORRECT_STATE, error);
+        return;
+    }
+
     __block chip::Optional<chip::Controller::CommissioningParameters> commissioningParameters;
     dispatch_sync(mChipWorkQueue, ^{
         commissioningParameters = mCppCommissioner->GetCommissioningParameters();