Skip to content

Commit 19e9ec6

Browse files
bzbarsky-applebzbarsky-apple
committed
Make MTRServerAttribute threadsafe.
If two API clients are both touching the same instance of MTRServerAttribute on different threads, we should handle that correctly.
1 parent 4196fd0 commit 19e9ec6

File tree

5 files changed

+117
-9
lines changed

5 files changed

+117
-9
lines changed

‎src/darwin/Framework/CHIP/MTRUnfairLock.h

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/**
2+
* Copyright (c) 2024 Project CHIP Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
/**
18+
* RAII wrapper around os_unfair_lock.
19+
*/
20+
21+
#import <os/lock.h>
22+
23+
class MTRAutoUnfairLock;
24+
25+
class MTRUnfairLock
26+
{
27+
public:
28+
MTRUnfairLock() { mOSLock = OS_UNFAIR_LOCK_INIT; }
29+
30+
void AssertOwner() { os_unfair_lock_assert_owner(&mOSLock); }
31+
32+
private:
33+
friend class MTRAutoUnfairLock;
34+
os_unfair_lock mOSLock;
35+
};
36+
37+
class MTRAutoUnfairLock
38+
{
39+
public:
40+
MTRAutoUnfairLock(MTRUnfairLock & aLock) : mLock(aLock) { os_unfair_lock_lock(&mLock.mOSLock); }
41+
42+
~MTRAutoUnfairLock() { os_unfair_lock_unlock(&mLock.mOSLock); }
43+
44+
private:
45+
MTRUnfairLock & mLock;
46+
};

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

+48-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@
2020
#import "MTRLogging_Internal.h"
2121
#import "MTRServerAttribute_Internal.h"
2222
#import "MTRServerEndpoint_Internal.h"
23+
#import "MTRUnfairLock.h"
2324
#import "NSDataSpanConversion.h"
25+
2426
#import <Matter/MTRServerAttribute.h>
2527

2628
#include <app/reporting/reporting.h>
@@ -32,7 +34,14 @@
3234

3335
MTR_DIRECT_MEMBERS
3436
@implementation MTRServerAttribute {
37+
// _lock always protects access to _deviceController, _value, and _parentCluster.
38+
// _serializedValue is protected when we are modifying it directly while we
39+
// have no _deviceController. Once we have one, _serializedValue is only
40+
// modified on the Matter thread.
41+
MTRUnfairLock _lock;
3542
MTRDeviceController * __weak _deviceController;
43+
NSDictionary<NSString *, id> * _value;
44+
app::ConcreteClusterPath _parentCluster;
3645
}
3746

3847
- (nullable instancetype)initAttributeWithID:(NSNumber *)attributeID initialValue:(NSDictionary<NSString *, id> *)value requiredReadPrivilege:(MTRAccessControlEntryPrivilege)requiredReadPrivilege writable:(BOOL)writable
@@ -121,7 +130,10 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate
121130
}
122131
}
123132

124-
// We serialized properly, so should be good to go on the value.
133+
// We serialized properly, so should be good to go on the value. Lock
134+
// around our ivar accesses.
135+
MTRAutoUnfairLock lock(_lock);
136+
125137
_value = [value copy];
126138

127139
MTRDeviceController * deviceController = _deviceController;
@@ -147,8 +159,16 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate
147159
return YES;
148160
}
149161

162+
- (NSDictionary<NSString *, id> *)value
163+
{
164+
MTRAutoUnfairLock lock(_lock);
165+
return [_value copy];
166+
}
167+
150168
- (BOOL)associateWithController:(nullable MTRDeviceController *)controller
151169
{
170+
MTRAutoUnfairLock lock(_lock);
171+
152172
MTRDeviceController * existingController = _deviceController;
153173
if (existingController != nil) {
154174
#if MTR_PER_CONTROLLER_STORAGE_ENABLED
@@ -167,7 +187,34 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
167187

168188
- (void)invalidate
169189
{
190+
MTRAutoUnfairLock lock(_lock);
191+
170192
_deviceController = nil;
171193
}
172194

195+
- (BOOL)addToCluster:(const app::ConcreteClusterPath &)cluster
196+
{
197+
MTRAutoUnfairLock lock(_lock);
198+
199+
if (_parentCluster.mClusterId != kInvalidClusterId) {
200+
MTR_LOG_ERROR("Cannot add attribute to cluster " ChipLogFormatMEI "; already added to cluster " ChipLogFormatMEI, ChipLogValueMEI(cluster.mClusterId), ChipLogValueMEI(_parentCluster.mClusterId));
201+
return NO;
202+
}
203+
204+
_parentCluster = cluster;
205+
return YES;
206+
}
207+
208+
- (void)updateParentCluster:(const app::ConcreteClusterPath &)cluster
209+
{
210+
MTRAutoUnfairLock lock(_lock);
211+
_parentCluster = cluster;
212+
}
213+
214+
- (const chip::app::ConcreteClusterPath &)parentCluster
215+
{
216+
MTRAutoUnfairLock lock(_lock);
217+
return _parentCluster;
218+
}
219+
173220
@end

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

+14-1
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,19 @@ NS_ASSUME_NONNULL_BEGIN
3737
*/
3838
- (void)invalidate;
3939

40+
/**
41+
* Add the attribute to a cluster with the given cluster path. Will return NO
42+
* if the attribute is already added to a cluster.
43+
*/
44+
- (BOOL)addToCluster:(const chip::app::ConcreteClusterPath &)cluster;
45+
46+
/**
47+
* Update the parent cluster path of the attribute. Should only be done for
48+
* attributes that are already added to a cluster, when the endpoint id needs to
49+
* be updated.
50+
*/
51+
- (void)updateParentCluster:(const chip::app::ConcreteClusterPath &)cluster;
52+
4053
/**
4154
* serializedValue is either an NSData or an NSArray<NSData *>, depending on
4255
* whether the attribute is list-typed.
@@ -47,7 +60,7 @@ NS_ASSUME_NONNULL_BEGIN
4760
* parentCluster will have kInvalidClusterId for the cluster ID until the
4861
* attribute is added to a cluster.
4962
*/
50-
@property (nonatomic, assign) chip::app::ConcreteClusterPath parentCluster;
63+
@property (nonatomic, assign, readonly) const chip::app::ConcreteClusterPath & parentCluster;
5164

5265
@end
5366

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

+5-7
Original file line numberDiff line numberDiff line change
@@ -176,11 +176,6 @@ - (BOOL)addAttribute:(MTRServerAttribute *)attribute
176176
return NO;
177177
}
178178

179-
if (attribute.parentCluster.mClusterId != kInvalidClusterId) {
180-
MTR_LOG_ERROR("Cannot add attribute to cluster %llu; already added to cluster %" PRIu32, _clusterID.unsignedLongLongValue, attribute.parentCluster.mClusterId);
181-
return NO;
182-
}
183-
184179
auto attributeID = attribute.attributeID.unsignedLongLongValue;
185180
if (attributeID == MTRAttributeIDTypeGlobalAttributeAttributeListID || attributeID == MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID || attributeID == MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID || attributeID == MTRAttributeIDTypeGlobalAttributeClusterRevisionID) {
186181
MTR_LOG_ERROR("Cannot add global attribute %llx on cluster %llx", attributeID, _clusterID.unsignedLongLongValue);
@@ -201,8 +196,11 @@ - (BOOL)addAttribute:(MTRServerAttribute *)attribute
201196
}
202197
}
203198

199+
if (![attribute addToCluster:ConcreteClusterPath(_parentEndpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue))]) {
200+
return NO;
201+
}
202+
204203
[_attributes addObject:attribute];
205-
attribute.parentCluster = ConcreteClusterPath(_parentEndpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue));
206204
return YES;
207205
}
208206

@@ -374,7 +372,7 @@ - (void)setParentEndpoint:(EndpointId)endpoint
374372
// Update it on all the attributes, in case the attributes were added to us
375373
// before we were added to the endpoint.
376374
for (MTRServerAttribute * attr in _attributes) {
377-
attr.parentCluster = ConcreteClusterPath(endpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue));
375+
[attr updateParentCluster:ConcreteClusterPath(endpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue))];
378376
}
379377
}
380378

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

+4
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@
170170
51565CB22A7AD77600469F18 /* MTRDeviceControllerDataStore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51565CB02A7AD77600469F18 /* MTRDeviceControllerDataStore.mm */; };
171171
51565CB42A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h in Headers */ = {isa = PBXBuildFile; fileRef = 51565CB32A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h */; settings = {ATTRIBUTES = (Public, ); }; };
172172
51565CB62A7B0D6600469F18 /* MTRDeviceControllerParameters.h in Headers */ = {isa = PBXBuildFile; fileRef = 51565CB52A7B0D6600469F18 /* MTRDeviceControllerParameters.h */; settings = {ATTRIBUTES = (Public, ); }; };
173+
515BE4ED2B72C0C5000BC1FD /* MTRUnfairLock.h in Headers */ = {isa = PBXBuildFile; fileRef = 515BE4EC2B72C0C5000BC1FD /* MTRUnfairLock.h */; };
173174
515C1C6F284F9FFB00A48F0C /* MTRFramework.mm in Sources */ = {isa = PBXBuildFile; fileRef = 515C1C6D284F9FFB00A48F0C /* MTRFramework.mm */; };
174175
515C1C70284F9FFB00A48F0C /* MTRFramework.h in Headers */ = {isa = PBXBuildFile; fileRef = 515C1C6E284F9FFB00A48F0C /* MTRFramework.h */; };
175176
516411312B6BF70300E67C05 /* DataModelHandler.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 516415FE2B6B132200D5CE11 /* DataModelHandler.cpp */; };
@@ -566,6 +567,7 @@
566567
51565CB02A7AD77600469F18 /* MTRDeviceControllerDataStore.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerDataStore.mm; sourceTree = "<group>"; };
567568
51565CB32A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerStorageDelegate.h; sourceTree = "<group>"; };
568569
51565CB52A7B0D6600469F18 /* MTRDeviceControllerParameters.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerParameters.h; sourceTree = "<group>"; };
570+
515BE4EC2B72C0C5000BC1FD /* MTRUnfairLock.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRUnfairLock.h; sourceTree = "<group>"; };
569571
515C1C6D284F9FFB00A48F0C /* MTRFramework.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRFramework.mm; sourceTree = "<group>"; };
570572
515C1C6E284F9FFB00A48F0C /* MTRFramework.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRFramework.h; sourceTree = "<group>"; };
571573
516415FA2B6ACA8300D5CE11 /* MTRServerAccessControl.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRServerAccessControl.mm; sourceTree = "<group>"; };
@@ -1353,6 +1355,7 @@
13531355
5173A47229C0E2ED00F67F48 /* MTRFabricInfo_Internal.h */,
13541356
5173A47429C0E2ED00F67F48 /* MTRFabricInfo.h */,
13551357
5173A47329C0E2ED00F67F48 /* MTRFabricInfo.mm */,
1358+
515BE4EC2B72C0C5000BC1FD /* MTRUnfairLock.h */,
13561359
3D69867E29382E58007314E7 /* Resources */,
13571360
);
13581361
path = CHIP;
@@ -1617,6 +1620,7 @@
16171620
51E51FC0282AD37A00FC978D /* MTRDeviceControllerStartupParams_Internal.h in Headers */,
16181621
3DECCB702934AECD00585AEC /* MTRLogging.h in Headers */,
16191622
1E4D654E29C208DD00BC3478 /* MTRCommissionableBrowserResult.h in Headers */,
1623+
515BE4ED2B72C0C5000BC1FD /* MTRUnfairLock.h in Headers */,
16201624
998F286F26D55EC5001846C6 /* MTRP256KeypairBridge.h in Headers */,
16211625
2C222ADF255C811800E446B9 /* MTRBaseDevice_Internal.h in Headers */,
16221626
514C7A022B64223400DD6D7B /* MTRServerEndpoint_Internal.h in Headers */,

0 commit comments

Comments
 (0)