Skip to content

Commit

Permalink
Fix threadsafety issue in MTRServerAttribute. (#32084)
Browse files Browse the repository at this point in the history
An attempt to get the description could race with updates of _parentCluster.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Apr 23, 2024
1 parent db0c02a commit 28000af
Showing 1 changed file with 13 additions and 7 deletions.
20 changes: 13 additions & 7 deletions src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,8 @@

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.
// _lock always protects access to _deviceController, _value,
// _serializedValue, and _parentCluster.
os_unfair_lock _lock;
MTRDeviceController * __weak _deviceController;
NSDictionary<NSString *, id> * _value;
Expand Down Expand Up @@ -137,7 +135,7 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate

_value = [value copy];

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

MTRDeviceController * deviceController = _deviceController;
if (deviceController == nil) {
Expand All @@ -150,6 +148,7 @@ - (BOOL)setValueInternal:(NSDictionary<NSString *, id> *)value logIfNotAssociate
_serializedValue = serializedValue;
} else {
[deviceController asyncDispatchToMatterQueue:^{
std::lock_guard lock(self->_lock);
auto changed = ![self->_serializedValue isEqual:serializedValue];
self->_serializedValue = serializedValue;
if (changed) {
Expand Down Expand Up @@ -185,7 +184,7 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller

_deviceController = controller;

MTR_LOG_DEFAULT("Associated %@ with controller", self);
MTR_LOG_DEFAULT("Associated %@ with controller", [self _descriptionWhileLocked]);

return YES;
}
Expand Down Expand Up @@ -216,14 +215,21 @@ - (void)updateParentCluster:(const app::ConcreteClusterPath &)cluster
_parentCluster = cluster;
}

- (const chip::app::ConcreteClusterPath &)parentCluster
- (const app::ConcreteClusterPath &)parentCluster
{
std::lock_guard lock(_lock);
return _parentCluster;
}

- (NSString *)description
{
std::lock_guard lock(_lock);
return [self _descriptionWhileLocked];
}

- (NSString *)_descriptionWhileLocked
{
os_unfair_lock_assert_owner(&_lock);
return [NSString stringWithFormat:@"<MTRServerAttribute endpoint %u, cluster " ChipLogFormatMEI ", id " ChipLogFormatMEI ", value '%@'>", static_cast<EndpointId>(_parentCluster.mEndpointId), ChipLogValueMEI(_parentCluster.mClusterId), ChipLogValueMEI(_attributeID.unsignedLongLongValue), _value];
}

Expand Down

0 comments on commit 28000af

Please sign in to comment.