Make MTRServerAttribute threadsafe. (project-chip#31970)

* Make MTRServerAttribute threadsafe.

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

* Address review comments.

Author: bzbarsky-apple

src/darwin/Framework/CHIP/MTRUnfairLock.h

@@ -0,0 +1,37 @@
+/**
+ *    Copyright (c) 2024 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+/**
+ * RAII wrapper around os_unfair_lock.
+ */
+
+#import <os/lock.h>
+
+#include <mutex>
+
+template <>
+class std::lock_guard<os_unfair_lock>
+{
+public:
+    explicit lock_guard(os_unfair_lock & lock) : mLock(lock) { os_unfair_lock_lock(&mLock); }
+    ~lock_guard() { os_unfair_lock_unlock(&mLock); }
+
+    lock_guard(const lock_guard &)     = delete;
+    void operator=(const lock_guard &) = delete;
+
+private:
+    os_unfair_lock & mLock;
+};

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

@@ -20,7 +20,9 @@
 #import "MTRLogging_Internal.h"
 #import "MTRServerAttribute_Internal.h"
 #import "MTRServerEndpoint_Internal.h"
+#import "MTRUnfairLock.h"
 #import "NSDataSpanConversion.h"
+
 #import <Matter/MTRServerAttribute.h>
 
 #include <app/reporting/reporting.h>
@@ -32,7 +34,14 @@
 
 MTR_DIRECT_MEMBERS
 @implementation MTRServerAttribute {
+    // _lock always protects access to _deviceController, _value, and
+    // _parentCluster.  _serializedValue is protected when we are modifying it
+    // directly while we have no _deviceController.  Once we have one,
+    // _serializedValue is only modified on the Matter thread.
+    os_unfair_lock _lock;
     MTRDeviceController * __weak _deviceController;
+    NSDictionary<NSString *, id> * _value;
+    app::ConcreteClusterPath _parentCluster;
 }
 
 - (nullable instancetype)initAttributeWithID:(NSNumber *)attributeID initialValue:(NSDictionary<NSString *, id> *)value requiredReadPrivilege:(MTRAccessControlEntryPrivilege)requiredReadPrivilege writable:(BOOL)writable
@@ -66,6 +75,7 @@ - (nullable instancetype)initWithAttributeID:(NSNumber *)attributeID value:(NSDi
         return nil;
     }
 
+    _lock = OS_UNFAIR_LOCK_INIT;
     _attributeID = attributeID;
     _requiredReadPrivilege = requiredReadPrivilege;
     _writable = writable;
@@ -121,7 +131,10 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate
         }
     }
 
-    // We serialized properly, so should be good to go on the value.
+    // We serialized properly, so should be good to go on the value.  Lock
+    // around our ivar accesses.
+    std::lock_guard lock(_lock);
+
     _value = [value copy];
 
     MTRDeviceController * deviceController = _deviceController;
@@ -147,8 +160,16 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate
     return YES;
 }
 
+- (NSDictionary<NSString *, id> *)value
+{
+    std::lock_guard lock(_lock);
+    return [_value copy];
+}
+
 - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
 {
+    std::lock_guard lock(_lock);
+
     MTRDeviceController * existingController = _deviceController;
     if (existingController != nil) {
 #if MTR_PER_CONTROLLER_STORAGE_ENABLED
@@ -167,7 +188,34 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
 
 - (void)invalidate
 {
+    std::lock_guard lock(_lock);
+
     _deviceController = nil;
 }
 
+- (BOOL)addToCluster:(const app::ConcreteClusterPath &)cluster
+{
+    std::lock_guard lock(_lock);
+
+    if (_parentCluster.mClusterId != kInvalidClusterId) {
+        MTR_LOG_ERROR("Cannot add attribute to cluster " ChipLogFormatMEI "; already added to cluster " ChipLogFormatMEI, ChipLogValueMEI(cluster.mClusterId), ChipLogValueMEI(_parentCluster.mClusterId));
+        return NO;
+    }
+
+    _parentCluster = cluster;
+    return YES;
+}
+
+- (void)updateParentCluster:(const app::ConcreteClusterPath &)cluster
+{
+    std::lock_guard lock(_lock);
+    _parentCluster = cluster;
+}
+
+- (const chip::app::ConcreteClusterPath &)parentCluster
+{
+    std::lock_guard lock(_lock);
+    return _parentCluster;
+}
+
 @end

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

@@ -37,6 +37,19 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)invalidate;
 
+/**
+ * Add the attribute to a cluster with the given cluster path.  Will return NO
+ * if the attribute is already added to a cluster.
+ */
+- (BOOL)addToCluster:(const chip::app::ConcreteClusterPath &)cluster;
+
+/**
+ * Update the parent cluster path of the attribute.  Should only be done for
+ * attributes that are already added to a cluster, when the endpoint id needs to
+ * be updated.
+ */
+- (void)updateParentCluster:(const chip::app::ConcreteClusterPath &)cluster;
+
 /**
  * serializedValue is either an NSData or an NSArray<NSData *>, depending on
  * whether the attribute is list-typed.
@@ -47,7 +60,7 @@ NS_ASSUME_NONNULL_BEGIN
  * parentCluster will have kInvalidClusterId for the cluster ID until the
  * attribute is added to a cluster.
  */
-@property (nonatomic, assign) chip::app::ConcreteClusterPath parentCluster;
+@property (nonatomic, assign, readonly) const chip::app::ConcreteClusterPath & parentCluster;
 
 @end
 

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

@@ -176,11 +176,6 @@ - (BOOL)addAttribute:(MTRServerAttribute *)attribute
         return NO;
     }
 
-    if (attribute.parentCluster.mClusterId != kInvalidClusterId) {
-        MTR_LOG_ERROR("Cannot add attribute to cluster %llu; already added to cluster %" PRIu32, _clusterID.unsignedLongLongValue, attribute.parentCluster.mClusterId);
-        return NO;
-    }
-
     auto attributeID = attribute.attributeID.unsignedLongLongValue;
     if (attributeID == MTRAttributeIDTypeGlobalAttributeAttributeListID || attributeID == MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID || attributeID == MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID || attributeID == MTRAttributeIDTypeGlobalAttributeClusterRevisionID) {
         MTR_LOG_ERROR("Cannot add global attribute %llx on cluster %llx", attributeID, _clusterID.unsignedLongLongValue);
@@ -201,8 +196,11 @@ - (BOOL)addAttribute:(MTRServerAttribute *)attribute
         }
     }
 
+    if (![attribute addToCluster:ConcreteClusterPath(_parentEndpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue))]) {
+        return NO;
+    }
+
     [_attributes addObject:attribute];
-    attribute.parentCluster = ConcreteClusterPath(_parentEndpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue));
     return YES;
 }
 
@@ -374,7 +372,7 @@ - (void)setParentEndpoint:(EndpointId)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.parentCluster = ConcreteClusterPath(endpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue));
+        [attr updateParentCluster:ConcreteClusterPath(endpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue))];
     }
 }
 

src/darwin/Framework/Matter.xcodeproj/project.pbxproj

@@ -170,6 +170,7 @@
 		51565CB22A7AD77600469F18 /* MTRDeviceControllerDataStore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51565CB02A7AD77600469F18 /* MTRDeviceControllerDataStore.mm */; };
 		51565CB42A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h in Headers */ = {isa = PBXBuildFile; fileRef = 51565CB32A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		51565CB62A7B0D6600469F18 /* MTRDeviceControllerParameters.h in Headers */ = {isa = PBXBuildFile; fileRef = 51565CB52A7B0D6600469F18 /* MTRDeviceControllerParameters.h */; settings = {ATTRIBUTES = (Public, ); }; };
+		515BE4ED2B72C0C5000BC1FD /* MTRUnfairLock.h in Headers */ = {isa = PBXBuildFile; fileRef = 515BE4EC2B72C0C5000BC1FD /* MTRUnfairLock.h */; };
 		515C1C6F284F9FFB00A48F0C /* MTRFramework.mm in Sources */ = {isa = PBXBuildFile; fileRef = 515C1C6D284F9FFB00A48F0C /* MTRFramework.mm */; };
 		515C1C70284F9FFB00A48F0C /* MTRFramework.h in Headers */ = {isa = PBXBuildFile; fileRef = 515C1C6E284F9FFB00A48F0C /* MTRFramework.h */; };
 		516411312B6BF70300E67C05 /* DataModelHandler.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 516415FE2B6B132200D5CE11 /* DataModelHandler.cpp */; };
@@ -567,6 +568,7 @@
 		51565CB02A7AD77600469F18 /* MTRDeviceControllerDataStore.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerDataStore.mm; sourceTree = "<group>"; };
 		51565CB32A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerStorageDelegate.h; sourceTree = "<group>"; };
 		51565CB52A7B0D6600469F18 /* MTRDeviceControllerParameters.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerParameters.h; sourceTree = "<group>"; };
+		515BE4EC2B72C0C5000BC1FD /* MTRUnfairLock.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRUnfairLock.h; sourceTree = "<group>"; };
 		515C1C6D284F9FFB00A48F0C /* MTRFramework.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRFramework.mm; sourceTree = "<group>"; };
 		515C1C6E284F9FFB00A48F0C /* MTRFramework.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRFramework.h; sourceTree = "<group>"; };
 		516415FA2B6ACA8300D5CE11 /* MTRServerAccessControl.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRServerAccessControl.mm; sourceTree = "<group>"; };
@@ -1358,6 +1360,7 @@
 				5173A47229C0E2ED00F67F48 /* MTRFabricInfo_Internal.h */,
 				5173A47429C0E2ED00F67F48 /* MTRFabricInfo.h */,
 				5173A47329C0E2ED00F67F48 /* MTRFabricInfo.mm */,
+				515BE4EC2B72C0C5000BC1FD /* MTRUnfairLock.h */,
 				3D69867E29382E58007314E7 /* Resources */,
 			);
 			path = CHIP;
@@ -1622,6 +1625,7 @@
 				51E51FC0282AD37A00FC978D /* MTRDeviceControllerStartupParams_Internal.h in Headers */,
 				3DECCB702934AECD00585AEC /* MTRLogging.h in Headers */,
 				1E4D654E29C208DD00BC3478 /* MTRCommissionableBrowserResult.h in Headers */,
+				515BE4ED2B72C0C5000BC1FD /* MTRUnfairLock.h in Headers */,
 				998F286F26D55EC5001846C6 /* MTRP256KeypairBridge.h in Headers */,
 				2C222ADF255C811800E446B9 /* MTRBaseDevice_Internal.h in Headers */,
 				514C7A022B64223400DD6D7B /* MTRServerEndpoint_Internal.h in Headers */,