Skip to content

Commit 69d3aed

Browse files
bzbarsky-applebzbarsky-apple
committed
Fix reference cycle between controller and data store in Matter.framework.
Also adds some leak detection to unit tests in CI that would have caught this.
1 parent 335ae19 commit 69d3aed

File tree

6 files changed

+164
-20
lines changed

6 files changed

+164
-20
lines changed

‎.github/workflows/darwin.yaml

+3
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ jobs:
8383
-enableUndefinedBehaviorSanitizer YES
8484
- flavor: tsan
8585
arguments: -enableThreadSanitizer YES
86+
# "leaks" does not seem to be very compatible with asan or tsan
87+
- flavor: leaks
88+
defines: ENABLE_LEAK_DETECTION=1
8689
steps:
8790
- name: Checkout
8891
uses: actions/checkout@v4

‎src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm

+79-16
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,8 @@ static bool IsValidCATNumber(id _Nullable value)
106106
@implementation MTRDeviceControllerDataStore {
107107
id<MTRDeviceControllerStorageDelegate> _storageDelegate;
108108
dispatch_queue_t _storageDelegateQueue;
109-
MTRDeviceController * _controller;
109+
// Controller owns us, so we have to make sure to not keep it alive.
110+
__weak MTRDeviceController * _controller;
110111
// Array of nodes with resumption info, oldest-stored first.
111112
NSMutableArray<NSNumber *> * _nodesWithResumptionInfo;
112113
}
@@ -126,7 +127,9 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller
126127
__block id resumptionNodeList;
127128
dispatch_sync(_storageDelegateQueue, ^{
128129
@autoreleasepool {
129-
resumptionNodeList = [_storageDelegate controller:_controller
130+
// NOTE: controller, not our weak ref, since we know it's still
131+
// valid under this sync dispatch.
132+
resumptionNodeList = [_storageDelegate controller:controller
130133
valueForKey:sResumptionNodeListKey
131134
securityLevel:MTRStorageSecurityLevelSecure
132135
sharingType:MTRStorageSharingTypeNotShared];
@@ -154,9 +157,15 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller
154157
- (void)fetchAttributeDataForAllDevices:(MTRDeviceControllerDataStoreClusterDataHandler)clusterDataHandler
155158
{
156159
__block NSDictionary<NSString *, id> * dataStoreSecureLocalValues = nil;
160+
MTRDeviceController * controller = _controller;
161+
if (controller == nil) {
162+
// Not expected; no way to call delegate without controller.
163+
return;
164+
}
165+
157166
dispatch_sync(_storageDelegateQueue, ^{
158167
if ([self->_storageDelegate respondsToSelector:@selector(valuesForController:securityLevel:sharingType:)]) {
159-
dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:self->_controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
168+
dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
160169
}
161170
});
162171

@@ -177,32 +186,38 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD
177186

178187
- (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo
179188
{
189+
MTRDeviceController * controller = _controller;
190+
if (controller == nil) {
191+
// Not expected; no way to call delegate without controller.
192+
return;
193+
}
194+
180195
auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID];
181196
dispatch_sync(_storageDelegateQueue, ^{
182197
if (oldInfo != nil) {
183198
// Remove old resumption id key. No need to do that for the
184199
// node id, because we are about to overwrite it.
185-
[_storageDelegate controller:_controller
200+
[_storageDelegate controller:controller
186201
removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
187202
securityLevel:MTRStorageSecurityLevelSecure
188203
sharingType:MTRStorageSharingTypeNotShared];
189204
[_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID];
190205
}
191206

192-
[_storageDelegate controller:_controller
207+
[_storageDelegate controller:controller
193208
storeValue:resumptionInfo
194209
forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID)
195210
securityLevel:MTRStorageSecurityLevelSecure
196211
sharingType:MTRStorageSharingTypeNotShared];
197-
[_storageDelegate controller:_controller
212+
[_storageDelegate controller:controller
198213
storeValue:resumptionInfo
199214
forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID)
200215
securityLevel:MTRStorageSecurityLevelSecure
201216
sharingType:MTRStorageSharingTypeNotShared];
202217

203218
// Update our resumption info node list.
204219
[_nodesWithResumptionInfo addObject:resumptionInfo.nodeID];
205-
[_storageDelegate controller:_controller
220+
[_storageDelegate controller:controller
206221
storeValue:[_nodesWithResumptionInfo copy]
207222
forKey:sResumptionNodeListKey
208223
securityLevel:MTRStorageSecurityLevelSecure
@@ -212,17 +227,23 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo
212227

213228
- (void)clearAllResumptionInfo
214229
{
230+
MTRDeviceController * controller = _controller;
231+
if (controller == nil) {
232+
// Not expected; no way to call delegate without controller.
233+
return;
234+
}
235+
215236
// Can we do less dispatch? We would need to have a version of
216237
// _findResumptionInfoWithKey that assumes we are already on the right queue.
217238
for (NSNumber * nodeID in _nodesWithResumptionInfo) {
218239
auto * oldInfo = [self findResumptionInfoByNodeID:nodeID];
219240
if (oldInfo != nil) {
220241
dispatch_sync(_storageDelegateQueue, ^{
221-
[_storageDelegate controller:_controller
242+
[_storageDelegate controller:controller
222243
removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
223244
securityLevel:MTRStorageSecurityLevelSecure
224245
sharingType:MTRStorageSharingTypeNotShared];
225-
[_storageDelegate controller:_controller
246+
[_storageDelegate controller:controller
226247
removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID)
227248
securityLevel:MTRStorageSecurityLevelSecure
228249
sharingType:MTRStorageSharingTypeNotShared];
@@ -235,9 +256,15 @@ - (void)clearAllResumptionInfo
235256

236257
- (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc
237258
{
259+
MTRDeviceController * controller = _controller;
260+
if (controller == nil) {
261+
// Not expected; no way to call delegate without controller.
262+
return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
263+
}
264+
238265
__block BOOL ok;
239266
dispatch_sync(_storageDelegateQueue, ^{
240-
ok = [_storageDelegate controller:_controller
267+
ok = [_storageDelegate controller:controller
241268
storeValue:noc
242269
forKey:sLastLocallyUsedNOCKey
243270
securityLevel:MTRStorageSecurityLevelSecure
@@ -248,10 +275,16 @@ - (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc
248275

249276
- (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC
250277
{
278+
MTRDeviceController * controller = _controller;
279+
if (controller == nil) {
280+
// Not expected; no way to call delegate without controller.
281+
return nil;
282+
}
283+
251284
__block id data;
252285
dispatch_sync(_storageDelegateQueue, ^{
253286
@autoreleasepool {
254-
data = [_storageDelegate controller:_controller
287+
data = [_storageDelegate controller:controller
255288
valueForKey:sLastLocallyUsedNOCKey
256289
securityLevel:MTRStorageSecurityLevelSecure
257290
sharingType:MTRStorageSharingTypeNotShared];
@@ -271,6 +304,12 @@ - (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC
271304

272305
- (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable NSString *)key
273306
{
307+
MTRDeviceController * controller = _controller;
308+
if (controller == nil) {
309+
// Not expected; no way to call delegate without controller.
310+
return nil;
311+
}
312+
274313
// key could be nil if [NSString stringWithFormat] returns nil for some reason.
275314
if (key == nil) {
276315
return nil;
@@ -279,7 +318,7 @@ - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable
279318
__block id resumptionInfo;
280319
dispatch_sync(_storageDelegateQueue, ^{
281320
@autoreleasepool {
282-
resumptionInfo = [_storageDelegate controller:_controller
321+
resumptionInfo = [_storageDelegate controller:controller
283322
valueForKey:key
284323
securityLevel:MTRStorageSecurityLevelSecure
285324
sharingType:MTRStorageSharingTypeNotShared];
@@ -318,9 +357,15 @@ - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable
318357

319358
- (id)_fetchAttributeCacheValueForKey:(NSString *)key expectedClass:(Class)expectedClass;
320359
{
360+
MTRDeviceController * controller = _controller;
361+
if (controller == nil) {
362+
// Not expected; no way to call delegate without controller.
363+
return nil;
364+
}
365+
321366
id data;
322367
@autoreleasepool {
323-
data = [_storageDelegate controller:_controller
368+
data = [_storageDelegate controller:controller
324369
valueForKey:key
325370
securityLevel:MTRStorageSecurityLevelSecure
326371
sharingType:MTRStorageSharingTypeNotShared];
@@ -338,7 +383,13 @@ - (id)_fetchAttributeCacheValueForKey:(NSString *)key expectedClass:(Class)expec
338383

339384
- (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key
340385
{
341-
return [_storageDelegate controller:_controller
386+
MTRDeviceController * controller = _controller;
387+
if (controller == nil) {
388+
// Not expected; no way to call delegate without controller.
389+
return NO;
390+
}
391+
392+
return [_storageDelegate controller:controller
342393
storeValue:value
343394
forKey:key
344395
securityLevel:MTRStorageSecurityLevelSecure
@@ -347,15 +398,27 @@ - (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key
347398

348399
- (BOOL)_bulkStoreAttributeCacheValues:(NSDictionary<NSString *, id<NSSecureCoding>> *)values
349400
{
350-
return [_storageDelegate controller:_controller
401+
MTRDeviceController * controller = _controller;
402+
if (controller == nil) {
403+
// Not expected; no way to call delegate without controller.
404+
return NO;
405+
}
406+
407+
return [_storageDelegate controller:controller
351408
storeValues:values
352409
securityLevel:MTRStorageSecurityLevelSecure
353410
sharingType:MTRStorageSharingTypeNotShared];
354411
}
355412

356413
- (BOOL)_removeAttributeCacheValueForKey:(NSString *)key
357414
{
358-
return [_storageDelegate controller:_controller
415+
MTRDeviceController * controller = _controller;
416+
if (controller == nil) {
417+
// Not expected; no way to call delegate without controller.
418+
return NO;
419+
}
420+
421+
return [_storageDelegate controller:controller
359422
removeValueForKey:key
360423
securityLevel:MTRStorageSecurityLevelSecure
361424
sharingType:MTRStorageSharingTypeNotShared];

‎src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m

+6-4
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,12 @@
1616

1717
#import <Matter/Matter.h>
1818

19-
// system dependencies
20-
#import <XCTest/XCTest.h>
21-
2219
#import "MTRDeviceControllerLocalTestStorage.h"
2320
#import "MTRDeviceTestDelegate.h"
2421
#import "MTRDevice_Internal.h"
2522
#import "MTRErrorTestUtils.h"
2623
#import "MTRFabricInfoChecker.h"
24+
#import "MTRTestCase.h"
2725
#import "MTRTestDeclarations.h"
2826
#import "MTRTestKeys.h"
2927
#import "MTRTestPerControllerStorage.h"
@@ -177,7 +175,7 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo
177175

178176
@end
179177

180-
@interface MTRPerControllerStorageTests : XCTestCase
178+
@interface MTRPerControllerStorageTests : MTRTestCase
181179
@end
182180

183181
@implementation MTRPerControllerStorageTests {
@@ -353,6 +351,8 @@ - (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)roo
353351

354352
- (void)test001_BasicControllerStartup
355353
{
354+
self.detectLeaks = YES;
355+
356356
__auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
357357
XCTAssertNotNil(factory);
358358

@@ -401,6 +401,8 @@ - (void)test001_BasicControllerStartup
401401

402402
- (void)test002_TryStartingTwoControllersWithSameNodeID
403403
{
404+
self.detectLeaks = YES;
405+
404406
__auto_type * rootKeys = [[MTRTestKeys alloc] init];
405407
XCTAssertNotNil(rootKeys);
406408

‎src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
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+
#import <XCTest/XCTest.h>
18+
19+
NS_ASSUME_NONNULL_BEGIN
20+
21+
@interface MTRTestCase : XCTestCase
22+
// It would be nice to do the leak-detection automatically, but running "leaks"
23+
// on every single sub-test is slow, and some of our tests seem to have leaks
24+
// outside Matter.framework. So have it be opt-in for now, and improve later.
25+
@property (nonatomic) BOOL detectLeaks;
26+
@end
27+
28+
NS_ASSUME_NONNULL_END

‎src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm

+42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
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+
#include <stdlib.h>
18+
#include <unistd.h>
19+
20+
#import "MTRTestCase.h"
21+
22+
@implementation MTRTestCase
23+
24+
/**
25+
* Unfortunately, doing this in "+ (void)tearDown" (the global suite teardown)
26+
* does not trigger a test failure even if the XCTAssertEqual fails.
27+
*/
28+
- (void)tearDown
29+
{
30+
#if defined(ENABLE_LEAK_DETECTION) && ENABLE_LEAK_DETECTION
31+
if (_detectLeaks) {
32+
int pid = getpid();
33+
__auto_type * cmd = [NSString stringWithFormat:@"leaks %d", pid];
34+
int ret = system(cmd.UTF8String);
35+
XCTAssertEqual(ret, 0, "LEAKS DETECTED");
36+
}
37+
#endif
38+
39+
[super tearDown];
40+
}
41+
42+
@end

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

+6
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@
139139
512431282BA0C8BF000BC136 /* SetMRPParametersCommand.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5124311C2BA0C09A000BC136 /* SetMRPParametersCommand.mm */; };
140140
512431292BA0C8BF000BC136 /* ResetMRPParametersCommand.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5124311A2BA0C09A000BC136 /* ResetMRPParametersCommand.mm */; };
141141
5129BCFD26A9EE3300122DDF /* MTRError.h in Headers */ = {isa = PBXBuildFile; fileRef = 5129BCFC26A9EE3300122DDF /* MTRError.h */; settings = {ATTRIBUTES = (Public, ); }; };
142+
5131BF662BE2E1B000D5D6BC /* MTRTestCase.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */; };
142143
51339B1F2A0DA64D00C798C1 /* MTRCertificateValidityTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */; };
143144
5136661328067D550025EDAE /* MTRDeviceController_Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 5136660F28067D540025EDAE /* MTRDeviceController_Internal.h */; };
144145
5136661428067D550025EDAE /* MTRDeviceControllerFactory.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5136661028067D540025EDAE /* MTRDeviceControllerFactory.mm */; };
@@ -549,6 +550,8 @@
549550
5124311B2BA0C09A000BC136 /* SetMRPParametersCommand.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SetMRPParametersCommand.h; sourceTree = "<group>"; };
550551
5124311C2BA0C09A000BC136 /* SetMRPParametersCommand.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = SetMRPParametersCommand.mm; sourceTree = "<group>"; };
551552
5129BCFC26A9EE3300122DDF /* MTRError.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MTRError.h; sourceTree = "<group>"; };
553+
5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRTestCase.mm; sourceTree = "<group>"; };
554+
5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRTestCase.h; sourceTree = "<group>"; };
552555
51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MTRCertificateValidityTests.m; sourceTree = "<group>"; };
553556
5136660F28067D540025EDAE /* MTRDeviceController_Internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceController_Internal.h; sourceTree = "<group>"; };
554557
5136661028067D540025EDAE /* MTRDeviceControllerFactory.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerFactory.mm; sourceTree = "<group>"; };
@@ -1119,6 +1122,8 @@
11191122
51189FC72A33ACE900184508 /* TestHelpers */ = {
11201123
isa = PBXGroup;
11211124
children = (
1125+
5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */,
1126+
5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */,
11221127
D437613F285BDC0D0051FEA2 /* MTRTestKeys.h */,
11231128
51C8E3F72825CDB600D47D00 /* MTRTestKeys.m */,
11241129
D4376140285BDC0D0051FEA2 /* MTRTestStorage.h */,
@@ -1974,6 +1979,7 @@
19741979
buildActionMask = 2147483647;
19751980
files = (
19761981
51742B4E29CB6B88009974FE /* MTRPairingTests.m in Sources */,
1982+
5131BF662BE2E1B000D5D6BC /* MTRTestCase.mm in Sources */,
19771983
51669AF02913204400F4AA36 /* MTRBackwardsCompatTests.m in Sources */,
19781984
51D10D2E2808E2CA00E8CA3D /* MTRTestStorage.m in Sources */,
19791985
51D0B12A2B61766F006E3511 /* MTRServerEndpointTests.m in Sources */,

0 commit comments

Comments
 (0)