Merging from master

rbultman · rbultman · commit 6ae664fedc2b · 2024-05-02T15:03:51.000-04:00
diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml
@@ -83,6 +83,9 @@ jobs:
                           -enableUndefinedBehaviorSanitizer YES
                     - flavor: tsan
                       arguments: -enableThreadSanitizer YES
+                    # "leaks" does not seem to be very compatible with asan or tsan
+                    - flavor: leaks
+                      defines: ENABLE_LEAK_DETECTION=1
         steps:
             - name: Checkout
               uses: actions/checkout@v4
diff --git a/scripts/setup/constraints.txt b/scripts/setup/constraints.txt
@@ -49,7 +49,7 @@ colorama==0.4.6
     #   west
 coloredlogs==15.0.1
     # via -r requirements.all.txt
-construct==2.10.54
+construct==2.10.70
     # via
     #   -r requirements.esp32.txt
     #   esp-coredump
@@ -208,7 +208,7 @@ python-socketio==4.6.1
     # via -r requirements.esp32.txt
 pytz==2022.7.1
     # via pandas
-pyyaml==6.0
+pyyaml==6.0.1
     # via
     #   esptool
     #   idf-component-manager
@@ -296,4 +296,3 @@ setuptools==68.0.0
 # Higher versions depend on proto-plus, which break
 # nanopb code generation (due to name conflict of the 'proto' module)
 google-api-core==2.17.0
-
diff --git a/scripts/setup/requirements.esp32.txt b/scripts/setup/requirements.esp32.txt
@@ -9,7 +9,7 @@ reedsolo>=1.5.3,<=1.5.4
 bitarray==2.6.0
 bitstring>=3.1.6,<4
 ecdsa>=0.16.0
-construct==2.10.54
+construct>=2.10.70
 python-socketio<5
 itsdangerous<2.1 ; python_version < "3.11"
 esp_idf_monitor==1.1.1
diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
@@ -22,6 +22,7 @@
 
 #include <lib/core/CASEAuthTag.h>
 #include <lib/core/NodeId.h>
+#include <lib/support/CodeUtils.h>
 #include <lib/support/SafeInt.h>
 
 // FIXME: Are these good key strings? https://github.com/project-chip/connectedhomeip/issues/28973
@@ -106,7 +107,8 @@ static bool IsValidCATNumber(id _Nullable value)
 @implementation MTRDeviceControllerDataStore {
     id<MTRDeviceControllerStorageDelegate> _storageDelegate;
     dispatch_queue_t _storageDelegateQueue;
-    MTRDeviceController * _controller;
+    // Controller owns us, so we have to make sure to not keep it alive.
+    __weak MTRDeviceController * _controller;
     // Array of nodes with resumption info, oldest-stored first.
     NSMutableArray<NSNumber *> * _nodesWithResumptionInfo;
 }
@@ -126,7 +128,9 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller
     __block id resumptionNodeList;
     dispatch_sync(_storageDelegateQueue, ^{
         @autoreleasepool {
-            resumptionNodeList = [_storageDelegate controller:_controller
+            // NOTE: controller, not our weak ref, since we know it's still
+            // valid under this sync dispatch.
+            resumptionNodeList = [_storageDelegate controller:controller
                                                   valueForKey:sResumptionNodeListKey
                                                 securityLevel:MTRStorageSecurityLevelSecure
                                                   sharingType:MTRStorageSharingTypeNotShared];
@@ -154,9 +158,12 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller
 - (void)fetchAttributeDataForAllDevices:(MTRDeviceControllerDataStoreClusterDataHandler)clusterDataHandler
 {
     __block NSDictionary<NSString *, id> * dataStoreSecureLocalValues = nil;
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturn(controller != nil); // No way to call delegate without controller.
+
     dispatch_sync(_storageDelegateQueue, ^{
         if ([self->_storageDelegate respondsToSelector:@selector(valuesForController:securityLevel:sharingType:)]) {
-            dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:self->_controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
+            dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
         }
     });
 
@@ -177,32 +184,35 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD
 
 - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo
 {
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturn(controller != nil); // No way to call delegate without controller.
+
     auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID];
     dispatch_sync(_storageDelegateQueue, ^{
         if (oldInfo != nil) {
             // Remove old resumption id key.  No need to do that for the
             // node id, because we are about to overwrite it.
-            [_storageDelegate controller:_controller
+            [_storageDelegate controller:controller
                        removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
                            securityLevel:MTRStorageSecurityLevelSecure
                              sharingType:MTRStorageSharingTypeNotShared];
             [_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID];
         }
 
-        [_storageDelegate controller:_controller
+        [_storageDelegate controller:controller
                           storeValue:resumptionInfo
                               forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID)
                        securityLevel:MTRStorageSecurityLevelSecure
                          sharingType:MTRStorageSharingTypeNotShared];
-        [_storageDelegate controller:_controller
+        [_storageDelegate controller:controller
                           storeValue:resumptionInfo
                               forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID)
                        securityLevel:MTRStorageSecurityLevelSecure
                          sharingType:MTRStorageSharingTypeNotShared];
 
         // Update our resumption info node list.
         [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID];
-        [_storageDelegate controller:_controller
+        [_storageDelegate controller:controller
                           storeValue:[_nodesWithResumptionInfo copy]
                               forKey:sResumptionNodeListKey
                        securityLevel:MTRStorageSecurityLevelSecure
@@ -212,17 +222,20 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo
 
 - (void)clearAllResumptionInfo
 {
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturn(controller != nil); // No way to call delegate without controller.
+
     // Can we do less dispatch?  We would need to have a version of
     // _findResumptionInfoWithKey that assumes we are already on the right queue.
     for (NSNumber * nodeID in _nodesWithResumptionInfo) {
         auto * oldInfo = [self findResumptionInfoByNodeID:nodeID];
         if (oldInfo != nil) {
             dispatch_sync(_storageDelegateQueue, ^{
-                [_storageDelegate controller:_controller
+                [_storageDelegate controller:controller
                            removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
                                securityLevel:MTRStorageSecurityLevelSecure
                                  sharingType:MTRStorageSharingTypeNotShared];
-                [_storageDelegate controller:_controller
+                [_storageDelegate controller:controller
                            removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID)
                                securityLevel:MTRStorageSecurityLevelSecure
                                  sharingType:MTRStorageSharingTypeNotShared];
@@ -235,9 +248,12 @@ - (void)clearAllResumptionInfo
 
 - (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc
 {
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturnError(controller != nil, CHIP_ERROR_PERSISTED_STORAGE_FAILED); // No way to call delegate without controller.
+
     __block BOOL ok;
     dispatch_sync(_storageDelegateQueue, ^{
-        ok = [_storageDelegate controller:_controller
+        ok = [_storageDelegate controller:controller
                                storeValue:noc
                                    forKey:sLastLocallyUsedNOCKey
                             securityLevel:MTRStorageSecurityLevelSecure
@@ -248,10 +264,13 @@ - (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc
 
 - (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC
 {
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturnValue(controller != nil, nil); // No way to call delegate without controller.
+
     __block id data;
     dispatch_sync(_storageDelegateQueue, ^{
         @autoreleasepool {
-            data = [_storageDelegate controller:_controller
+            data = [_storageDelegate controller:controller
                                     valueForKey:sLastLocallyUsedNOCKey
                                   securityLevel:MTRStorageSecurityLevelSecure
                                     sharingType:MTRStorageSharingTypeNotShared];
@@ -271,6 +290,9 @@ - (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC
 
 - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable NSString *)key
 {
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturnValue(controller != nil, nil); // No way to call delegate without controller.
+
     // key could be nil if [NSString stringWithFormat] returns nil for some reason.
     if (key == nil) {
         return nil;
@@ -279,7 +301,7 @@ - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable
     __block id resumptionInfo;
     dispatch_sync(_storageDelegateQueue, ^{
         @autoreleasepool {
-            resumptionInfo = [_storageDelegate controller:_controller
+            resumptionInfo = [_storageDelegate controller:controller
                                               valueForKey:key
                                             securityLevel:MTRStorageSecurityLevelSecure
                                               sharingType:MTRStorageSharingTypeNotShared];
@@ -318,9 +340,12 @@ - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable
 
 - (id)_fetchAttributeCacheValueForKey:(NSString *)key expectedClass:(Class)expectedClass;
 {
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturnValue(controller != nil, nil); // No way to call delegate without controller.
+
     id data;
     @autoreleasepool {
-        data = [_storageDelegate controller:_controller
+        data = [_storageDelegate controller:controller
                                 valueForKey:key
                               securityLevel:MTRStorageSecurityLevelSecure
                                 sharingType:MTRStorageSharingTypeNotShared];
@@ -338,7 +363,10 @@ - (id)_fetchAttributeCacheValueForKey:(NSString *)key expectedClass:(Class)expec
 
 - (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key
 {
-    return [_storageDelegate controller:_controller
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturnValue(controller != nil, NO); // No way to call delegate without controller.
+
+    return [_storageDelegate controller:controller
                              storeValue:value
                                  forKey:key
                           securityLevel:MTRStorageSecurityLevelSecure
@@ -347,15 +375,21 @@ - (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key
 
 - (BOOL)_bulkStoreAttributeCacheValues:(NSDictionary<NSString *, id<NSSecureCoding>> *)values
 {
-    return [_storageDelegate controller:_controller
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturnValue(controller != nil, NO); // No way to call delegate without controller.
+
+    return [_storageDelegate controller:controller
                             storeValues:values
                           securityLevel:MTRStorageSecurityLevelSecure
                             sharingType:MTRStorageSharingTypeNotShared];
 }
 
 - (BOOL)_removeAttributeCacheValueForKey:(NSString *)key
 {
-    return [_storageDelegate controller:_controller
+    MTRDeviceController * controller = _controller;
+    VerifyOrReturnValue(controller != nil, NO); // No way to call delegate without controller.
+
+    return [_storageDelegate controller:controller
                       removeValueForKey:key
                           securityLevel:MTRStorageSecurityLevelSecure
                             sharingType:MTRStorageSharingTypeNotShared];
diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
@@ -16,14 +16,12 @@
 
 #import <Matter/Matter.h>
 
-// system dependencies
-#import <XCTest/XCTest.h>
-
 #import "MTRDeviceControllerLocalTestStorage.h"
 #import "MTRDeviceTestDelegate.h"
 #import "MTRDevice_Internal.h"
 #import "MTRErrorTestUtils.h"
 #import "MTRFabricInfoChecker.h"
+#import "MTRTestCase.h"
 #import "MTRTestDeclarations.h"
 #import "MTRTestKeys.h"
 #import "MTRTestPerControllerStorage.h"
@@ -177,7 +175,7 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo
 
 @end
 
-@interface MTRPerControllerStorageTests : XCTestCase
+@interface MTRPerControllerStorageTests : MTRTestCase
 @end
 
 @implementation MTRPerControllerStorageTests {
@@ -353,6 +351,8 @@ - (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)roo
 
 - (void)test001_BasicControllerStartup
 {
+    self.detectLeaks = YES;
+
     __auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
     XCTAssertNotNil(factory);
 
@@ -401,6 +401,8 @@ - (void)test001_BasicControllerStartup
 
 - (void)test002_TryStartingTwoControllersWithSameNodeID
 {
+    self.detectLeaks = YES;
+
     __auto_type * rootKeys = [[MTRTestKeys alloc] init];
     XCTAssertNotNil(rootKeys);
 
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h
@@ -0,0 +1,28 @@
+/**
+ *    Copyright (c) 2024 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#import <XCTest/XCTest.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface MTRTestCase : XCTestCase
+// It would be nice to do the leak-detection automatically, but running "leaks"
+// on every single sub-test is slow, and some of our tests seem to have leaks
+// outside Matter.framework.  So have it be opt-in for now, and improve later.
+@property (nonatomic) BOOL detectLeaks;
+@end
+
+NS_ASSUME_NONNULL_END
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm
@@ -0,0 +1,44 @@
+/**
+ *    Copyright (c) 2024 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <stdlib.h>
+#include <unistd.h>
+
+#import "MTRTestCase.h"
+
+@implementation MTRTestCase
+
+/**
+ * Unfortunately, doing this in "+ (void)tearDown" (the global suite teardown)
+ * does not trigger a test failure even if the XCTAssertEqual fails.
+ */
+- (void)tearDown
+{
+#if 0
+#if defined(ENABLE_LEAK_DETECTION) && ENABLE_LEAK_DETECTION
+    if (_detectLeaks) {
+        int pid = getpid();
+        __auto_type * cmd = [NSString stringWithFormat:@"leaks %d", pid];
+        int ret = system(cmd.UTF8String);
+        XCTAssertEqual(ret, 0, "LEAKS DETECTED");
+    }
+#endif
+#endif
+
+    [super tearDown];
+}
+
+@end
diff --git a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj
@@ -139,6 +139,7 @@
 		512431282BA0C8BF000BC136 /* SetMRPParametersCommand.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5124311C2BA0C09A000BC136 /* SetMRPParametersCommand.mm */; };
 		512431292BA0C8BF000BC136 /* ResetMRPParametersCommand.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5124311A2BA0C09A000BC136 /* ResetMRPParametersCommand.mm */; };
 		5129BCFD26A9EE3300122DDF /* MTRError.h in Headers */ = {isa = PBXBuildFile; fileRef = 5129BCFC26A9EE3300122DDF /* MTRError.h */; settings = {ATTRIBUTES = (Public, ); }; };
+		5131BF662BE2E1B000D5D6BC /* MTRTestCase.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */; };
 		51339B1F2A0DA64D00C798C1 /* MTRCertificateValidityTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */; };
 		5136661328067D550025EDAE /* MTRDeviceController_Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 5136660F28067D540025EDAE /* MTRDeviceController_Internal.h */; };
 		5136661428067D550025EDAE /* MTRDeviceControllerFactory.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5136661028067D540025EDAE /* MTRDeviceControllerFactory.mm */; };
@@ -549,6 +550,8 @@
 		5124311B2BA0C09A000BC136 /* SetMRPParametersCommand.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SetMRPParametersCommand.h; sourceTree = "<group>"; };
 		5124311C2BA0C09A000BC136 /* SetMRPParametersCommand.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = SetMRPParametersCommand.mm; sourceTree = "<group>"; };
 		5129BCFC26A9EE3300122DDF /* MTRError.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MTRError.h; sourceTree = "<group>"; };
+		5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRTestCase.mm; sourceTree = "<group>"; };
+		5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRTestCase.h; sourceTree = "<group>"; };
 		51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MTRCertificateValidityTests.m; sourceTree = "<group>"; };
 		5136660F28067D540025EDAE /* MTRDeviceController_Internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceController_Internal.h; sourceTree = "<group>"; };
 		5136661028067D540025EDAE /* MTRDeviceControllerFactory.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerFactory.mm; sourceTree = "<group>"; };
@@ -1119,6 +1122,8 @@
 		51189FC72A33ACE900184508 /* TestHelpers */ = {
 			isa = PBXGroup;
 			children = (
+				5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */,
+				5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */,
 				D437613F285BDC0D0051FEA2 /* MTRTestKeys.h */,
 				51C8E3F72825CDB600D47D00 /* MTRTestKeys.m */,
 				D4376140285BDC0D0051FEA2 /* MTRTestStorage.h */,
@@ -1974,6 +1979,7 @@
 			buildActionMask = 2147483647;
 			files = (
 				51742B4E29CB6B88009974FE /* MTRPairingTests.m in Sources */,
+				5131BF662BE2E1B000D5D6BC /* MTRTestCase.mm in Sources */,
 				51669AF02913204400F4AA36 /* MTRBackwardsCompatTests.m in Sources */,
 				51D10D2E2808E2CA00E8CA3D /* MTRTestStorage.m in Sources */,
 				51D0B12A2B61766F006E3511 /* MTRServerEndpointTests.m in Sources */,
diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp
diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp
diff --git a/src/transport/Session.h b/src/transport/Session.h
diff --git a/src/transport/SessionDelegate.h b/src/transport/SessionDelegate.h
