Skip to content

Commit

Permalink
Fix reference cycle between controller and data store in Matter.frame…
Browse files Browse the repository at this point in the history
…work. (#33263)

* 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.

* Address review comment.

* Disable the leak checks for now, because we are getting unrelated positives.
  • Loading branch information
bzbarsky-apple authored May 2, 2024
1 parent de796eb commit 16b8076
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 20 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/darwin.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
66 changes: 50 additions & 16 deletions src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand All @@ -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];
Expand Down Expand Up @@ -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];
}
});

Expand All @@ -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
Expand All @@ -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];
Expand All @@ -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
Expand All @@ -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];
Expand All @@ -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;
Expand All @@ -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];
Expand Down Expand Up @@ -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];
Expand All @@ -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
Expand All @@ -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];
Expand Down
10 changes: 6 additions & 4 deletions src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -177,7 +175,7 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo

@end

@interface MTRPerControllerStorageTests : XCTestCase
@interface MTRPerControllerStorageTests : MTRTestCase
@end

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

- (void)test001_BasicControllerStartup
{
self.detectLeaks = YES;

__auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
XCTAssertNotNil(factory);

Expand Down Expand Up @@ -401,6 +401,8 @@ - (void)test001_BasicControllerStartup

- (void)test002_TryStartingTwoControllersWithSameNodeID
{
self.detectLeaks = YES;

__auto_type * rootKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(rootKeys);

Expand Down
28 changes: 28 additions & 0 deletions src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h
Original file line number Diff line number Diff line change
@@ -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
44 changes: 44 additions & 0 deletions src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions src/darwin/Framework/Matter.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -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>"; };
Expand Down Expand Up @@ -1119,6 +1122,8 @@
51189FC72A33ACE900184508 /* TestHelpers */ = {
isa = PBXGroup;
children = (
5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */,
5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */,
D437613F285BDC0D0051FEA2 /* MTRTestKeys.h */,
51C8E3F72825CDB600D47D00 /* MTRTestKeys.m */,
D4376140285BDC0D0051FEA2 /* MTRTestStorage.h */,
Expand Down Expand Up @@ -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 */,
Expand Down

0 comments on commit 16b8076

Please sign in to comment.