Make MTRServerAttribute threadsafe. (#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.
diff --git a/src/darwin/Framework/CHIP/MTRUnfairLock.h b/src/darwin/Framework/CHIP/MTRUnfairLock.h
new file mode 100644
index 0000000..c046ee6
--- /dev/null
+++ b/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;
+};
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm
index 5a69c10..6b9ced0 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm
+++ b/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 @@
return nil;
}
+ _lock = OS_UNFAIR_LOCK_INIT;
_attributeID = attributeID;
_requiredReadPrivilege = requiredReadPrivilege;
_writable = writable;
@@ -121,7 +131,10 @@
}
}
- // 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 @@
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 @@
- (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
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute_Internal.h b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute_Internal.h
index f6b423f..647be68 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute_Internal.h
+++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute_Internal.h
@@ -38,6 +38,19 @@
- (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 @@
* 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
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm
index 8de78a6..1d331c7 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm
+++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm
@@ -176,11 +176,6 @@
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 @@
}
}
+ 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 @@
// 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))];
}
}
diff --git a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj
index 0683838..2abac83 100644
--- a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj
+++ b/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 */,
