From 24f7fed5c340b32a29e967f2d30a15c4f4a439a0 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Mon, 12 Feb 2024 17:37:25 -0500
Subject: [PATCH] Fix threadsafety issue in MTRServerAttribute. (#32084)

An attempt to get the description could race with updates of _parentCluster.
---
 .../CHIP/ServerEndpoint/MTRServerAttribute.mm | 20 ++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm
index b2f840839f7c46..638b6f5ea48e28 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm
+++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.mm
@@ -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;
@@ -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) {
@@ -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) {
@@ -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;
 }
@@ -216,7 +215,7 @@ - (void)updateParentCluster:(const app::ConcreteClusterPath &)cluster
     _parentCluster = cluster;
 }
 
-- (const chip::app::ConcreteClusterPath &)parentCluster
+- (const app::ConcreteClusterPath &)parentCluster
 {
     std::lock_guard lock(_lock);
     return _parentCluster;
@@ -224,6 +223,13 @@ - (void)updateParentCluster:(const app::ConcreteClusterPath &)cluster
 
 - (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];
 }