From a2c53a27dbbad2f5ed2d83335e0d0c7817c59baa Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 22 Feb 2024 12:55:03 -0500 Subject: [PATCH] Make MTRServerCluster threadsafe. (#32245) * Make MTRServerCluster threadsafe. If two API clients are both touching the same instance of MTRServerCluster on different threads, we should handle that correctly. * Address review comments. --- .../CHIP/ServerEndpoint/MTRServerAttribute.h | 10 +- .../CHIP/ServerEndpoint/MTRServerCluster.h | 10 +- .../CHIP/ServerEndpoint/MTRServerCluster.mm | 104 +++++++++++++++++- .../MTRServerCluster_Internal.h | 24 ++-- .../CHIP/ServerEndpoint/MTRServerEndpoint.mm | 14 +-- 5 files changed, 132 insertions(+), 30 deletions(-) diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h index 7d8c27b74b55d3..f9c7e7ca6dfe4b 100644 --- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h +++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h @@ -24,6 +24,8 @@ NS_ASSUME_NONNULL_BEGIN * A representation of an attribute implemented on a server cluster by an * MTRDeviceController. An attribute has an identifier and a value, and may or * may not be writable. + * + * MTRServerAttribute's API can be accessed from any thread. */ NS_SWIFT_SENDABLE MTR_NEWLY_AVAILABLE @@ -51,13 +53,13 @@ MTR_NEWLY_AVAILABLE */ - (BOOL)setValue:(NSDictionary *)value; -@property (nonatomic, copy, readonly) NSNumber * attributeID; -@property (nonatomic, copy, readonly) NSDictionary * value; +@property (atomic, copy, readonly) NSNumber * attributeID; +@property (atomic, copy, readonly) NSDictionary * value; /** * The privilege level necessary to read this attribute. */ -@property (nonatomic, assign, readonly) MTRAccessControlEntryPrivilege requiredReadPrivilege; -@property (nonatomic, assign, readonly, getter=isWritable) BOOL writable; +@property (atomic, assign, readonly) MTRAccessControlEntryPrivilege requiredReadPrivilege; +@property (atomic, assign, readonly, getter=isWritable) BOOL writable; @end diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h index 02b0f4b274374d..591d3afec59779 100644 --- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h +++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h @@ -23,6 +23,8 @@ NS_ASSUME_NONNULL_BEGIN /** * A representation of a server cluster implemented by an MTRDeviceController. + * + * MTRServerCluster's API can be accessed from any thread. */ NS_SWIFT_SENDABLE MTR_NEWLY_AVAILABLE @@ -90,9 +92,9 @@ MTR_NEWLY_AVAILABLE */ + (MTRServerCluster *)newDescriptorCluster; -@property (nonatomic, copy, readonly) NSNumber * clusterID; +@property (atomic, copy, readonly) NSNumber * clusterID; -@property (nonatomic, copy, readonly) NSNumber * clusterRevision; +@property (atomic, copy, readonly) NSNumber * clusterRevision; /** * The list of entities that are allowed to access this cluster instance. This @@ -100,12 +102,12 @@ MTR_NEWLY_AVAILABLE * * Defaults to empty list, which means no additional access grants. */ -@property (nonatomic, copy, readonly) NSArray * accessGrants; +@property (atomic, copy, readonly) NSArray * accessGrants; /** * The list of attributes supported by the cluster. */ -@property (nonatomic, copy, readonly) NSArray * attributes; +@property (atomic, copy, readonly) NSArray * attributes; @end diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm index c0fba9e9bda64d..7439c0ff6a453a 100644 --- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm +++ b/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 #import -#import "NSDataSpanConversion.h" - #include #include #include @@ -71,11 +72,26 @@ @implementation MTRServerCluster { std::unique_ptr _attributeAccessInterface; // We can't use something like std::unique_ptr // 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 _matterAttributeMetadata; std::unique_ptr _matterAcceptedCommandList; std::unique_ptr _matterGeneratedCommandList; + + NSSet * _matterAccessGrants; + + chip::EndpointId _parentEndpoint; + + // _acceptedCommands and _generatedCommands are touched directly by our API + // consumer. + NSArray * _acceptedCommands; + NSArray * _generatedCommands; + + /** + * _lock always protects access to all our mutable ivars (the ones that are + * modified after init). + */ + os_unfair_lock _lock; } - (nullable instancetype)initWithClusterID:(NSNumber *)clusterID revision:(NSNumber *)revision @@ -117,6 +133,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 +158,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,6 +168,7 @@ - (void)updateMatterAccessGrants NSSet * grants = [_accessGrants copy]; [deviceController asyncDispatchToMatterQueue:^{ + std::lock_guard lock(self->_lock); self->_matterAccessGrants = grants; } errorHandler:nil]; @@ -156,6 +176,8 @@ - (void)updateMatterAccessGrants - (void)addAccessGrant:(MTRAccessGrant *)accessGrant { + std::lock_guard lock(self->_lock); + [_accessGrants addObject:accessGrant]; [self updateMatterAccessGrants]; @@ -163,13 +185,24 @@ - (void)addAccessGrant:(MTRAccessGrant *)accessGrant - (void)removeAccessGrant:(MTRAccessGrant *)accessGrant; { + std::lock_guard lock(self->_lock); + [_accessGrants removeObject:accessGrant]; [self updateMatterAccessGrants]; } +- (NSArray *)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 +246,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,7 +348,7 @@ - (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(attributeCount)); return YES; @@ -321,6 +356,8 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller - (void)invalidate { + std::lock_guard lock(_lock); + // Undo any work associateWithController did. for (MTRServerAttribute * attr in _attributes) { [attr invalidate]; @@ -342,6 +379,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,6 +393,8 @@ - (void)unregisterMatterCluster { assertChipStackLockedByCurrentThread(); + std::lock_guard lock(_lock); + if (_attributeAccessInterface != nullptr) { unregisterAttributeAccessOverride(_attributeAccessInterface.get()); } @@ -361,38 +402,84 @@ - (void)unregisterMatterCluster - (NSArray *)accessGrants { + std::lock_guard lock(_lock); + return [_accessGrants allObjects]; } - (NSArray *)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.unsignedLongLongValue))]; } + return YES; +} + +- (chip::EndpointId)parentEndpoint +{ + std::lock_guard lock(_lock); + return _parentEndpoint; } - (Span)matterAttributeMetadata { // This is always called after our _matterAttributeMetadata has been set up // by associateWithController. + std::lock_guard lock(_lock); return Span(_matterAttributeMetadata.data(), _matterAttributeMetadata.size()); } +- (void)setAcceptedCommands:(NSArray *)acceptedCommands +{ + std::lock_guard lock(_lock); + _acceptedCommands = [acceptedCommands copy]; +} + +- (NSArray *)acceptedCommands +{ + std::lock_guard lock(_lock); + return [_acceptedCommands copy]; +} + +- (void)setGeneratedCommands:(NSArray *)generatedCommands +{ + std::lock_guard lock(_lock); + _generatedCommands = [generatedCommands copy]; +} + +- (NSArray *)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 +500,13 @@ - (CommandId *)matterGeneratedCommands - (NSString *)description { + std::lock_guard lock(_lock); + return [self _descriptionWhileLocked]; +} + +- (NSString *)_descriptionWhileLocked +{ + os_unfair_lock_assert_owner(&_lock); return [NSString stringWithFormat:@"", _parentEndpoint, ChipLogValueMEI(_clusterID.unsignedLongLongValue)]; } diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h index 4ae3d0db348533..253885b7d82e98 100644 --- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h +++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h @@ -57,40 +57,46 @@ 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 * 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 (atomic, copy, readonly) NSArray * matterAccessGrants; /** * parentEndpoint will be kInvalidEndpointId until the cluster is added to an endpoint. */ -@property (nonatomic, assign) chip::EndpointId parentEndpoint; +@property (atomic, assign, readonly) chip::EndpointId parentEndpoint; /** * The attribute metadata for the cluster. Only valid after associateWithController: has succeeded. */ -@property (nonatomic, assign, readonly) chip::Span matterAttributeMetadata; +@property (atomic, assign, readonly) chip::Span matterAttributeMetadata; /** * The list of accepted command IDs. */ -@property (nonatomic, copy, nullable) NSArray * acceptedCommands; +@property (atomic, copy, nullable) NSArray * acceptedCommands; /** * The list of generated command IDs. */ -@property (nonatomic, copy, nullable) NSArray * generatedCommands; +@property (atomic, copy, nullable) NSArray * generatedCommands; /** * The list of accepted commands IDs in the format the Matter stack needs. */ -@property (nonatomic, assign, nullable, readonly) chip::CommandId * matterAcceptedCommands; +@property (atomic, assign, nullable, readonly) chip::CommandId * matterAcceptedCommands; /** * The list of generated commands IDs in the format the Matter stack needs. */ -@property (nonatomic, assign, nullable, readonly) chip::CommandId * matterGeneratedCommands; +@property (atomic, assign, nullable, readonly) chip::CommandId * matterGeneratedCommands; @end diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm index c58b1ac4fa7744..c7553230f3748e 100644 --- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm +++ b/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.unsignedLongLongValue)]) { + return NO; + } [_serverClusters addObject:serverCluster]; - serverCluster.parentEndpoint = static_cast(_endpointID.unsignedLongLongValue); + return YES; } @@ -401,7 +399,7 @@ - (void)invalidate NSMutableArray * grants = [[_matterAccessGrants allObjects] mutableCopy]; for (MTRServerCluster * cluster in _serverClusters) { if ([cluster.clusterID isEqual:clusterID]) { - [grants addObjectsFromArray:[cluster.matterAccessGrants allObjects]]; + [grants addObjectsFromArray:cluster.matterAccessGrants]; } }