Skip to content

Commit aeb3fe3

Browse files
bzbarsky-appleraul-marquez-csa
authored andcommittedFeb 16, 2024
Fix threadsafety issue in MTRServerAttribute. (project-chip#32084)
An attempt to get the description could race with updates of _parentCluster.
1 parent 4038f17 commit aeb3fe3

File tree

1 file changed

+13
-7
lines changed

1 file changed

+13
-7
lines changed
 

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

+13-7
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,8 @@
3434

3535
MTR_DIRECT_MEMBERS
3636
@implementation MTRServerAttribute {
37-
// _lock always protects access to _deviceController, _value, and
38-
// _parentCluster. _serializedValue is protected when we are modifying it
39-
// directly while we have no _deviceController. Once we have one,
40-
// _serializedValue is only modified on the Matter thread.
37+
// _lock always protects access to _deviceController, _value,
38+
// _serializedValue, and _parentCluster.
4139
os_unfair_lock _lock;
4240
MTRDeviceController * __weak _deviceController;
4341
NSDictionary<NSString *, id> * _value;
@@ -137,7 +135,7 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate
137135

138136
_value = [value copy];
139137

140-
MTR_LOG_DEFAULT("Attribute value updated: %@", self); // Logs new as part of our description.
138+
MTR_LOG_DEFAULT("Attribute value updated: %@", [self _descriptionWhileLocked]); // Logs new value as part of our description.
141139

142140
MTRDeviceController * deviceController = _deviceController;
143141
if (deviceController == nil) {
@@ -150,6 +148,7 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate
150148
_serializedValue = serializedValue;
151149
} else {
152150
[deviceController asyncDispatchToMatterQueue:^{
151+
std::lock_guard lock(self->_lock);
153152
auto changed = ![self->_serializedValue isEqual:serializedValue];
154153
self->_serializedValue = serializedValue;
155154
if (changed) {
@@ -185,7 +184,7 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
185184

186185
_deviceController = controller;
187186

188-
MTR_LOG_DEFAULT("Associated %@ with controller", self);
187+
MTR_LOG_DEFAULT("Associated %@ with controller", [self _descriptionWhileLocked]);
189188

190189
return YES;
191190
}
@@ -216,14 +215,21 @@ - (void)updateParentCluster:(const app::ConcreteClusterPath &)cluster
216215
_parentCluster = cluster;
217216
}
218217

219-
- (const chip::app::ConcreteClusterPath &)parentCluster
218+
- (const app::ConcreteClusterPath &)parentCluster
220219
{
221220
std::lock_guard lock(_lock);
222221
return _parentCluster;
223222
}
224223

225224
- (NSString *)description
226225
{
226+
std::lock_guard lock(_lock);
227+
return [self _descriptionWhileLocked];
228+
}
229+
230+
- (NSString *)_descriptionWhileLocked
231+
{
232+
os_unfair_lock_assert_owner(&_lock);
227233
return [NSString stringWithFormat:@"<MTRServerAttribute endpoint %u, cluster " ChipLogFormatMEI ", id " ChipLogFormatMEI ", value '%@'>", static_cast<EndpointId>(_parentCluster.mEndpointId), ChipLogValueMEI(_parentCluster.mClusterId), ChipLogValueMEI(_attributeID.unsignedLongLongValue), _value];
228234
}
229235

0 commit comments

Comments
 (0)