Make MTRServerCluster threadsafe.

If two API clients are both touching the same instance of MTRServerCluster on
different threads, we should handle that correctly.

Author: bzbarsky-apple

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm

@@ -20,11 +20,12 @@
 #import "MTRServerAttribute_Internal.h"
 #import "MTRServerCluster_Internal.h"
 #import "MTRServerEndpoint_Internal.h"
+#import "MTRUnfairLock.h"
+#import "NSDataSpanConversion.h"
+
 #import <Matter/MTRClusterConstants.h>
 #import <Matter/MTRServerCluster.h>
 
-#import "NSDataSpanConversion.h"
-
 #include <app/AttributeAccessInterface.h>
 #include <app/clusters/descriptor/descriptor.h>
 #include <app/data-model/PreEncodedValue.h>
@@ -71,11 +72,28 @@ @implementation MTRServerCluster {
     std::unique_ptr<MTRServerAttributeAccessInterface> _attributeAccessInterface;
     // We can't use something like std::unique_ptr<EmberAfAttributeMetadata[]>
     // because EmberAfAttributeMetadata does not have a default constructor, so
-    // we can't alloc and then initializer later.
+    // we can't alloc and then initialize later.
     std::vector<EmberAfAttributeMetadata> _matterAttributeMetadata;
 
     std::unique_ptr<CommandId[]> _matterAcceptedCommandList;
     std::unique_ptr<CommandId[]> _matterGeneratedCommandList;
+
+    NSSet<MTRAccessGrant *> * _matterAccessGrants;
+
+    chip::EndpointId _parentEndpoint;
+
+    // _acceptedCommands and _generatedCommands are touched directly by our API
+    // consumer.
+    NSArray<NSNumber *> * _acceptedCommands;
+    NSArray<NSNumber *> * _generatedCommands;
+
+    /**
+     * _lock always protects access to: _accessGrants, _attributes,
+     * _deviceController, _attributeAccessInterface, _matterAttributeMetadata,
+     * _matterAccessGrants, _parentEndpoint, _acceptedCommands,
+     * _generatedCommands, _matterAcceptedCommandList, _matterGeneratedCommandList.
+     */
+    os_unfair_lock _lock;
 }
 
 - (nullable instancetype)initWithClusterID:(NSNumber *)clusterID revision:(NSNumber *)revision
@@ -117,6 +135,7 @@ - (instancetype)initInternalWithClusterID:(NSNumber *)clusterID revision:(NSNumb
         return nil;
     }
 
+    _lock = OS_UNFAIR_LOCK_INIT;
     _clusterID = [clusterID copy];
     _clusterRevision = [revision copy];
     _accessGrants = [[NSMutableSet alloc] init];
@@ -141,6 +160,8 @@ - (instancetype)initInternalWithClusterID:(NSNumber *)clusterID revision:(NSNumb
 
 - (void)updateMatterAccessGrants
 {
+    os_unfair_lock_assert_owner(&_lock);
+
     MTRDeviceController * deviceController = _deviceController;
     if (deviceController == nil) {
         // _matterAccessGrants will be updated when we get bound to a controller.
@@ -149,27 +170,41 @@ - (void)updateMatterAccessGrants
 
     NSSet * grants = [_accessGrants copy];
     [deviceController asyncDispatchToMatterQueue:^{
+        std::lock_guard lock(self->_lock);
         self->_matterAccessGrants = grants;
     }
                                     errorHandler:nil];
 }
 
 - (void)addAccessGrant:(MTRAccessGrant *)accessGrant
 {
+    std::lock_guard lock(self->_lock);
+
     [_accessGrants addObject:accessGrant];
 
     [self updateMatterAccessGrants];
 }
 
 - (void)removeAccessGrant:(MTRAccessGrant *)accessGrant;
 {
+    std::lock_guard lock(self->_lock);
+
     [_accessGrants removeObject:accessGrant];
 
     [self updateMatterAccessGrants];
 }
 
+- (NSArray<MTRAccessGrant *> *)matterAccessGrants
+{
+    std::lock_guard lock(self->_lock);
+
+    return [_matterAccessGrants allObjects];
+}
+
 - (BOOL)addAttribute:(MTRServerAttribute *)attribute
 {
+    std::lock_guard lock(self->_lock);
+
     MTRDeviceController * deviceController = _deviceController;
     if (deviceController != nil) {
         MTR_LOG_ERROR("Cannot add attribute on cluster %llx which is already in use", _clusterID.unsignedLongLongValue);
@@ -213,6 +248,8 @@ - (BOOL)addAttribute:(MTRServerAttribute *)attribute
 
 - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
 {
+    std::lock_guard lock(self->_lock);
+
     MTRDeviceController * existingController = _deviceController;
     if (existingController != nil) {
 #if MTR_PER_CONTROLLER_STORAGE_ENABLED
@@ -313,14 +350,16 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
 
     _deviceController = controller;
 
-    MTR_LOG_DEFAULT("Associated %@, attribute count %llu, with controller", self,
+    MTR_LOG_DEFAULT("Associated %@, attribute count %llu, with controller", [self _descriptionWhileLocked],
         static_cast<unsigned long long>(attributeCount));
 
     return YES;
 }
 
 - (void)invalidate
 {
+    std::lock_guard lock(_lock);
+
     // Undo any work associateWithController did.
     for (MTRServerAttribute * attr in _attributes) {
         [attr invalidate];
@@ -342,6 +381,8 @@ - (void)registerMatterCluster
 {
     assertChipStackLockedByCurrentThread();
 
+    std::lock_guard lock(_lock);
+
     if (!registerAttributeAccessOverride(_attributeAccessInterface.get())) {
         // This should only happen if we somehow managed to register an
         // AttributeAccessInterface for the same (endpoint, cluster) pair.
@@ -354,45 +395,93 @@ - (void)unregisterMatterCluster
 {
     assertChipStackLockedByCurrentThread();
 
+    std::lock_guard lock(_lock);
+
     if (_attributeAccessInterface != nullptr) {
         unregisterAttributeAccessOverride(_attributeAccessInterface.get());
     }
 }
 
 - (NSArray<MTRAccessGrant *> *)accessGrants
 {
+    std::lock_guard lock(_lock);
+
     return [_accessGrants allObjects];
 }
 
 - (NSArray<MTRServerAttribute *> *)attributes
 {
+    std::lock_guard lock(_lock);
+
     return [_attributes copy];
 }
 
-- (void)setParentEndpoint:(EndpointId)endpoint
+- (BOOL)addToEndpoint:(chip::EndpointId)endpoint
 {
+    std::lock_guard lock(_lock);
+
+    if (_parentEndpoint != kInvalidEndpointId) {
+        MTR_LOG_ERROR("Cannot add cluster " ChipLogFormatMEI " to endpoint %" PRIu32 "; already added to endpoint %" PRIu32,
+            ChipLogValueMEI(_clusterID.unsignedLongLongValue), endpoint, _parentEndpoint);
+        return NO;
+    }
+
     _parentEndpoint = endpoint;
     // Update it on all the attributes, in case the attributes were added to us
     // before we were added to the endpoint.
     for (MTRServerAttribute * attr in _attributes) {
         [attr updateParentCluster:ConcreteClusterPath(endpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue))];
     }
+    return YES;
+}
+
+- (chip::EndpointId)parentEndpoint
+{
+    std::lock_guard lock(_lock);
+    return _parentEndpoint;
 }
 
 - (Span<const EmberAfAttributeMetadata>)matterAttributeMetadata
 {
     // This is always called after our _matterAttributeMetadata has been set up
     // by associateWithController.
+    std::lock_guard lock(_lock);
     return Span<const EmberAfAttributeMetadata>(_matterAttributeMetadata.data(), _matterAttributeMetadata.size());
 }
 
+- (void)setAcceptedCommands:(NSArray<NSNumber *> *)acceptedCommands
+{
+    std::lock_guard lock(_lock);
+    _acceptedCommands = [acceptedCommands copy];
+}
+
+- (NSArray<NSNumber *> *)acceptedCommands
+{
+    std::lock_guard lock(_lock);
+    return [_acceptedCommands copy];
+}
+
+- (void)setGeneratedCommands:(NSArray<NSNumber *> *)generatedCommands
+{
+    std::lock_guard lock(_lock);
+    _generatedCommands = [generatedCommands copy];
+}
+
+- (NSArray<NSNumber *> *)generatedCommands
+{
+    std::lock_guard lock(_lock);
+    return [_generatedCommands copy];
+}
+
 - (CommandId *)matterAcceptedCommands
 {
+    std::lock_guard lock(_lock);
     return _matterAcceptedCommandList.get();
 }
 
 - (CommandId *)matterGeneratedCommands
 {
+    std::lock_guard lock(_lock);
     return _matterGeneratedCommandList.get();
 }
 
@@ -413,6 +502,13 @@ - (CommandId *)matterGeneratedCommands
 
 - (NSString *)description
 {
+    std::lock_guard lock(_lock);
+    return [self _descriptionWhileLocked];
+}
+
+- (NSString *)_descriptionWhileLocked
+{
+    os_unfair_lock_assert_owner(&_lock);
     return [NSString stringWithFormat:@"<MTRServerCluster endpoint %u, id " ChipLogFormatMEI ">",
                      _parentEndpoint, ChipLogValueMEI(_clusterID.unsignedLongLongValue)];
 }

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h

@@ -57,15 +57,21 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)invalidate;
 
 /**
- * The access grants the Matter stack can observe.  Only modified while in
- * Initializing state or on the Matter queue.
+ * Add the cluster to an endpoint with the given endpoint ID.  Will return NO
+ * if the cluster is already added to an endpoint.
  */
-@property (nonatomic, strong, readonly) NSSet<MTRAccessGrant *> * matterAccessGrants;
+- (BOOL)addToEndpoint:(chip::EndpointId)endpoint;
+
+/**
+ * The access grants the Matter stack can observe.  Only modified while
+ * associating with a controller or on the Matter queue.
+ */
+@property (nonatomic, copy, readonly) NSArray<MTRAccessGrant *> * matterAccessGrants;
 
 /**
  * parentEndpoint will be kInvalidEndpointId until the cluster is added to an endpoint.
  */
-@property (nonatomic, assign) chip::EndpointId parentEndpoint;
+@property (nonatomic, assign, readonly) chip::EndpointId parentEndpoint;
 
 /**
  * The attribute metadata for the cluster.  Only valid after associateWithController: has succeeded.

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm

@@ -154,20 +154,18 @@ - (BOOL)addServerCluster:(MTRServerCluster *)serverCluster
         return NO;
     }
 
-    if (serverCluster.parentEndpoint != kInvalidEndpointId) {
-        MTR_LOG_ERROR("Cannot add cluster to endpoint %llu; already added to endpoint %" PRIu32, _endpointID.unsignedLongLongValue, serverCluster.parentEndpoint);
-        return NO;
-    }
-
     for (MTRServerCluster * existingCluster in _serverClusters) {
         if ([existingCluster.clusterID isEqual:serverCluster.clusterID]) {
-            MTR_LOG_ERROR("Cannot add second cluster with ID %llx on endpoint %llu", serverCluster.clusterID.unsignedLongLongValue, _endpointID.unsignedLongLongValue);
+            MTR_LOG_ERROR("Cannot add second cluster with ID " ChipLogFormatMEI " on endpoint %llu", ChipLogValueMEI(serverCluster.clusterID.unsignedLongLongValue), _endpointID.unsignedLongLongValue);
             return NO;
         }
     }
 
+    if (![serverCluster addToEndpoint:static_cast<EndpointId>(_endpointID.unsignedLongLongValue)]) {
+        return NO;
+    }
     [_serverClusters addObject:serverCluster];
-    serverCluster.parentEndpoint = static_cast<EndpointId>(_endpointID.unsignedLongLongValue);
+
     return YES;
 }
 
@@ -401,7 +399,7 @@ - (void)invalidate
     NSMutableArray<MTRAccessGrant *> * grants = [[_matterAccessGrants allObjects] mutableCopy];
     for (MTRServerCluster * cluster in _serverClusters) {
         if ([cluster.clusterID isEqual:clusterID]) {
-            [grants addObjectsFromArray:[cluster.matterAccessGrants allObjects]];
+            [grants addObjectsFromArray:cluster.matterAccessGrants];
         }
     }