From 3baa12cc7601b962ae6a32ce3d15858e48423a19 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Mon, 5 Dec 2022 15:59:06 -0500
Subject: [PATCH] Stop calling into the device attestation delegate on the
 Matter work queue. (#23867)

Also add some documentation to MTRStorage and MTRKeypair about expected callee
behavior.

Partially addresses https://github.com/project-chip/connectedhomeip/issues/23277
---
 src/darwin/Framework/CHIP/MTRCommissioningParameters.h |  3 ++-
 .../Framework/CHIP/MTRDeviceAttestationDelegate.h      |  4 +---
 .../CHIP/MTRDeviceAttestationDelegateBridge.h          |  6 +++---
 src/darwin/Framework/CHIP/MTRDeviceController.mm       |  2 +-
 src/darwin/Framework/CHIP/MTRKeypair.h                 | 10 ++++++++++
 src/darwin/Framework/CHIP/MTRStorage.h                 |  3 +++
 6 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/darwin/Framework/CHIP/MTRCommissioningParameters.h b/src/darwin/Framework/CHIP/MTRCommissioningParameters.h
index ff115df8a8eade..1359178ec9f5c7 100644
--- a/src/darwin/Framework/CHIP/MTRCommissioningParameters.h
+++ b/src/darwin/Framework/CHIP/MTRCommissioningParameters.h
@@ -60,6 +60,8 @@ NS_ASSUME_NONNULL_BEGIN
  * An optional delegate that can be notified upon completion of device
  * attestation.  See documentation for MTRDeviceAttestationDelegate for
  * details.
+ *
+ * The delegate methods will be invoked on an arbitrary thread.
  */
 @property (nonatomic, strong, nullable) id<MTRDeviceAttestationDelegate> deviceAttestationDelegate;
 /**
@@ -68,7 +70,6 @@ NS_ASSUME_NONNULL_BEGIN
  *
  * If nil, the fail-safe will not be extended before calling into the
  * deviceAttestationDelegate.
-
  */
 @property (nonatomic, copy, nullable) NSNumber * failSafeExpiryTimeout MTR_NEWLY_AVAILABLE;
 
diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h
index d54fdeb577adb7..375805f092d793 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h
+++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h
@@ -30,9 +30,7 @@ NS_ASSUME_NONNULL_BEGIN
 @end
 
 /**
- * The protocol definition for the MTRDeviceAttestationDelegate
- *
- * All delegate methods will be called on the callers queue.
+ * The protocol definition for the MTRDeviceAttestationDelegate.
  */
 @protocol MTRDeviceAttestationDelegate <NSObject>
 @optional
diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h
index 30eff23345c7e7..201137d63b3bc4 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h
+++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h
@@ -28,12 +28,12 @@ NS_ASSUME_NONNULL_BEGIN
 class MTRDeviceAttestationDelegateBridge : public chip::Credentials::DeviceAttestationDelegate {
 public:
     MTRDeviceAttestationDelegateBridge(MTRDeviceController * deviceController,
-        id<MTRDeviceAttestationDelegate> deviceAttestationDelegate, dispatch_queue_t queue,
-        chip::Optional<uint16_t> expiryTimeoutSecs, bool shouldWaitAfterDeviceAttestation = false)
+        id<MTRDeviceAttestationDelegate> deviceAttestationDelegate, chip::Optional<uint16_t> expiryTimeoutSecs,
+        bool shouldWaitAfterDeviceAttestation = false)
         : mResult(chip::Credentials::AttestationVerificationResult::kSuccess)
         , mDeviceController(deviceController)
         , mDeviceAttestationDelegate(deviceAttestationDelegate)
-        , mQueue(queue)
+        , mQueue(dispatch_queue_create("com.csa.matter.framework.device_attestation.workqueue", DISPATCH_QUEUE_SERIAL))
         , mExpiryTimeoutSecs(expiryTimeoutSecs)
         , mShouldWaitAfterDeviceAttestation(shouldWaitAfterDeviceAttestation)
     {
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
index c3e1c710e8f861..168a23fb32ac19 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceController.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -439,7 +439,7 @@ - (BOOL)commissionNodeWithID:(NSNumber *)nodeID
                 shouldWaitAfterDeviceAttestation = YES;
             }
             _deviceAttestationDelegateBridge = new MTRDeviceAttestationDelegateBridge(
-                self, commissioningParams.deviceAttestationDelegate, _chipWorkQueue, timeoutSecs, shouldWaitAfterDeviceAttestation);
+                self, commissioningParams.deviceAttestationDelegate, timeoutSecs, shouldWaitAfterDeviceAttestation);
             params.SetDeviceAttestationDelegate(_deviceAttestationDelegateBridge);
         }
 
diff --git a/src/darwin/Framework/CHIP/MTRKeypair.h b/src/darwin/Framework/CHIP/MTRKeypair.h
index b0affe4f5ba678..a4e4521b2864a6 100644
--- a/src/darwin/Framework/CHIP/MTRKeypair.h
+++ b/src/darwin/Framework/CHIP/MTRKeypair.h
@@ -20,6 +20,16 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+/**
+ * This protocol is used by the Matter framework to sign messages with a private
+ * key and verify signatures with a public key.
+ *
+ * The Matter framework may call keypair methods from arbitrary threads and
+ * concurrently.
+ *
+ * Implementations of the keypair methods must not call into any Matter
+ * framework APIs.
+ */
 @protocol MTRKeypair <NSObject>
 @required
 /**
diff --git a/src/darwin/Framework/CHIP/MTRStorage.h b/src/darwin/Framework/CHIP/MTRStorage.h
index 445f55ef38e493..97813fdbb957e9 100644
--- a/src/darwin/Framework/CHIP/MTRStorage.h
+++ b/src/darwin/Framework/CHIP/MTRStorage.h
@@ -24,6 +24,9 @@ NS_ASSUME_NONNULL_BEGIN
  *
  * The Matter framework may call storage methods from arbitrary threads, but
  * will not call storage methods concurrently.
+ *
+ * Implementations of the storage methods must not call into any Matter
+ * framework APIs.
  */
 MTR_NEWLY_AVAILABLE
 @protocol MTRStorage <NSObject>