From c98a785e57e23f62c79b4e6b595559ce4ea70207 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 24 Sep 2024 18:09:12 -0400 Subject: [PATCH 1/6] Add autorelease pools to the Darwin KVS implementation. (#35752) KVS can be used from outside the Matter queue in our example apps, and we were leaking things that never got released due to the lack of an autorelease pool around these KVS operations. Many thanks to Vivien Nicolas for catching this. --- .../Darwin/KeyValueStoreManagerImpl.mm | 268 +++++++++--------- 1 file changed, 138 insertions(+), 130 deletions(-) diff --git a/src/platform/Darwin/KeyValueStoreManagerImpl.mm b/src/platform/Darwin/KeyValueStoreManagerImpl.mm index b0ea2591c371b4..ffb28435f642af 100644 --- a/src/platform/Darwin/KeyValueStoreManagerImpl.mm +++ b/src/platform/Darwin/KeyValueStoreManagerImpl.mm @@ -147,179 +147,187 @@ - (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context key:(n CHIP_ERROR KeyValueStoreManagerImpl::Init(const char * fileName) { - if (mInitialized) { - return CHIP_NO_ERROR; - } + @autoreleasepool { + if (mInitialized) { + return CHIP_NO_ERROR; + } - ReturnErrorCodeIf(gContext != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorCodeIf(fileName == nullptr, CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorCodeIf(fileName[0] == '\0', CHIP_ERROR_INVALID_ARGUMENT); - - NSURL * url = nullptr; - NSString * filepath = [NSString stringWithUTF8String:fileName]; - ReturnErrorCodeIf(filepath == nil, CHIP_ERROR_INVALID_ARGUMENT); - - // relative paths are relative to Documents folder - if (![filepath hasPrefix:@"/"]) { - NSURL * documentsDirectory = [NSFileManager.defaultManager URLForDirectory:NSDocumentDirectory - inDomain:NSUserDomainMask - appropriateForURL:nil - create:YES - error:nil]; - if (documentsDirectory == nullptr) { - ChipLogError(DeviceLayer, "Failed to get documents directory."); - return CHIP_ERROR_INTERNAL; + ReturnErrorCodeIf(gContext != nullptr, CHIP_ERROR_INCORRECT_STATE); + ReturnErrorCodeIf(fileName == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(fileName[0] == '\0', CHIP_ERROR_INVALID_ARGUMENT); + + NSURL * url = nullptr; + NSString * filepath = [NSString stringWithUTF8String:fileName]; + ReturnErrorCodeIf(filepath == nil, CHIP_ERROR_INVALID_ARGUMENT); + + // relative paths are relative to Documents folder + if (![filepath hasPrefix:@"/"]) { + NSURL * documentsDirectory = [NSFileManager.defaultManager URLForDirectory:NSDocumentDirectory + inDomain:NSUserDomainMask + appropriateForURL:nil + create:YES + error:nil]; + if (documentsDirectory == nullptr) { + ChipLogError(DeviceLayer, "Failed to get documents directory."); + return CHIP_ERROR_INTERNAL; + } + ChipLogProgress( + DeviceLayer, "Found user documents directory: %s", [[documentsDirectory absoluteString] UTF8String]); + + url = [NSURL URLWithString:filepath relativeToURL:documentsDirectory]; + } else { + url = [NSURL fileURLWithPath:filepath]; } - ChipLogProgress( - DeviceLayer, "Found user documents directory: %s", [[documentsDirectory absoluteString] UTF8String]); + ReturnErrorCodeIf(url == nullptr, CHIP_ERROR_NO_MEMORY); - url = [NSURL URLWithString:filepath relativeToURL:documentsDirectory]; - } else { - url = [NSURL fileURLWithPath:filepath]; - } - ReturnErrorCodeIf(url == nullptr, CHIP_ERROR_NO_MEMORY); + ChipLogProgress(DeviceLayer, "KVS will be written to: %s", [[url absoluteString] UTF8String]); - ChipLogProgress(DeviceLayer, "KVS will be written to: %s", [[url absoluteString] UTF8String]); + NSManagedObjectModel * model = CreateManagedObjectModel(); + ReturnErrorCodeIf(model == nullptr, CHIP_ERROR_NO_MEMORY); - NSManagedObjectModel * model = CreateManagedObjectModel(); - ReturnErrorCodeIf(model == nullptr, CHIP_ERROR_NO_MEMORY); + // setup persistent store coordinator - // setup persistent store coordinator + NSPersistentStoreCoordinator * coordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:model]; - NSPersistentStoreCoordinator * coordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:model]; + NSError * error = nil; + if (![coordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:url options:nil error:&error]) { + ChipLogError(DeviceLayer, "Invalid store. Attempting to clear: %s", error.localizedDescription.UTF8String); + if (![[NSFileManager defaultManager] removeItemAtURL:url error:&error]) { + ChipLogError(DeviceLayer, "Failed to delete item: %s", error.localizedDescription.UTF8String); + } - NSError * error = nil; - if (![coordinator addPersistentStoreWithType:NSSQLiteStoreType configuration:nil URL:url options:nil error:&error]) { - ChipLogError(DeviceLayer, "Invalid store. Attempting to clear: %s", error.localizedDescription.UTF8String); - if (![[NSFileManager defaultManager] removeItemAtURL:url error:&error]) { - ChipLogError(DeviceLayer, "Failed to delete item: %s", error.localizedDescription.UTF8String); + if (![coordinator addPersistentStoreWithType:NSSQLiteStoreType + configuration:nil + URL:url + options:nil + error:&error]) { + ChipLogError(DeviceLayer, "Failed to initialize clear KVS storage: %s", error.localizedDescription.UTF8String); + chipDie(); + } } - if (![coordinator addPersistentStoreWithType:NSSQLiteStoreType - configuration:nil - URL:url - options:nil - error:&error]) { - ChipLogError(DeviceLayer, "Failed to initialize clear KVS storage: %s", error.localizedDescription.UTF8String); - chipDie(); - } - } + // create Managed Object context + gContext = [[NSManagedObjectContext alloc] initWithConcurrencyType:NSPrivateQueueConcurrencyType]; + [gContext setMergePolicy:NSMergeByPropertyObjectTrumpMergePolicy]; + [gContext setPersistentStoreCoordinator:coordinator]; - // create Managed Object context - gContext = [[NSManagedObjectContext alloc] initWithConcurrencyType:NSPrivateQueueConcurrencyType]; - [gContext setMergePolicy:NSMergeByPropertyObjectTrumpMergePolicy]; - [gContext setPersistentStoreCoordinator:coordinator]; - - mInitialized = true; - return CHIP_NO_ERROR; + mInitialized = true; + return CHIP_NO_ERROR; + } // @autoreleasepool } CHIP_ERROR KeyValueStoreManagerImpl::_Get( const char * key, void * value, size_t value_size, size_t * read_bytes_size, size_t offset) { - ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorCodeIf(offset != 0, CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_UNINITIALIZED); - - KeyValueItem * item = FindItemForKey([[NSString alloc] initWithUTF8String:key], nil, true); - if (!item) { - return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; - } + @autoreleasepool { + ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(offset != 0, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_UNINITIALIZED); + + KeyValueItem * item = FindItemForKey([[NSString alloc] initWithUTF8String:key], nil, true); + if (!item) { + return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; + } - __block NSData * itemValue = nil; - // can only access this object on the managed queue - [gContext performBlockAndWait:^{ - itemValue = item.value; - }]; + __block NSData * itemValue = nil; + // can only access this object on the managed queue + [gContext performBlockAndWait:^{ + itemValue = item.value; + }]; - if (read_bytes_size != nullptr) { - *read_bytes_size = itemValue.length; - } + if (read_bytes_size != nullptr) { + *read_bytes_size = itemValue.length; + } - if (value != nullptr) { - memcpy(value, itemValue.bytes, std::min((itemValue.length), value_size)); + if (value != nullptr) { + memcpy(value, itemValue.bytes, std::min((itemValue.length), value_size)); #if CHIP_CONFIG_DARWIN_STORAGE_VERBOSE_LOGGING - fprintf(stderr, "GETTING VALUE FOR: '%s': ", key); - for (size_t i = 0; i < std::min((itemValue.length), value_size); ++i) { - fprintf(stderr, "%02x ", static_cast(value)[i]); - } - fprintf(stderr, "\n"); + fprintf(stderr, "GETTING VALUE FOR: '%s': ", key); + for (size_t i = 0; i < std::min((itemValue.length), value_size); ++i) { + fprintf(stderr, "%02x ", static_cast(value)[i]); + } + fprintf(stderr, "\n"); #endif - } + } - if (itemValue.length > value_size) { - return CHIP_ERROR_BUFFER_TOO_SMALL; - } + if (itemValue.length > value_size) { + return CHIP_ERROR_BUFFER_TOO_SMALL; + } - return CHIP_NO_ERROR; + return CHIP_NO_ERROR; + } // @autoreleasepool } CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key) { - ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_UNINITIALIZED); + @autoreleasepool { + ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_UNINITIALIZED); - KeyValueItem * item = FindItemForKey([[NSString alloc] initWithUTF8String:key], nil); - if (!item) { - return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; - } + KeyValueItem * item = FindItemForKey([[NSString alloc] initWithUTF8String:key], nil); + if (!item) { + return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; + } - __block BOOL success = NO; - __block NSError * error = nil; - [gContext performBlockAndWait:^{ - [gContext deleteObject:item]; - success = [gContext save:&error]; - }]; + __block BOOL success = NO; + __block NSError * error = nil; + [gContext performBlockAndWait:^{ + [gContext deleteObject:item]; + success = [gContext save:&error]; + }]; - if (!success) { - ChipLogError(DeviceLayer, "Error saving context: %s", error.localizedDescription.UTF8String); - return CHIP_ERROR_PERSISTED_STORAGE_FAILED; - } + if (!success) { + ChipLogError(DeviceLayer, "Error saving context: %s", error.localizedDescription.UTF8String); + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + } - return CHIP_NO_ERROR; + return CHIP_NO_ERROR; + } // @autoreleasepool } CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, size_t value_size) { - ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_UNINITIALIZED); - - NSData * data = [[NSData alloc] initWithBytes:value length:value_size]; - - NSString * itemKey = [[NSString alloc] initWithUTF8String:key]; - ReturnErrorCodeIf(itemKey == nil, CHIP_ERROR_INVALID_ARGUMENT); + @autoreleasepool { + ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_UNINITIALIZED); + + NSData * data = [[NSData alloc] initWithBytes:value length:value_size]; + + NSString * itemKey = [[NSString alloc] initWithUTF8String:key]; + ReturnErrorCodeIf(itemKey == nil, CHIP_ERROR_INVALID_ARGUMENT); + + KeyValueItem * item = FindItemForKey(itemKey, nil); + if (!item) { + [gContext performBlockAndWait:^{ + [gContext insertObject:[[KeyValueItem alloc] initWithContext:gContext key:itemKey value:data]]; + }]; + } else { + [gContext performBlockAndWait:^{ + item.value = data; + }]; + } - KeyValueItem * item = FindItemForKey(itemKey, nil); - if (!item) { + __block BOOL success = NO; + __block NSError * error = nil; [gContext performBlockAndWait:^{ - [gContext insertObject:[[KeyValueItem alloc] initWithContext:gContext key:itemKey value:data]]; + success = [gContext save:&error]; }]; - } else { - [gContext performBlockAndWait:^{ - item.value = data; - }]; - } - __block BOOL success = NO; - __block NSError * error = nil; - [gContext performBlockAndWait:^{ - success = [gContext save:&error]; - }]; - - if (!success) { - ChipLogError(DeviceLayer, "Error saving context: %s", error.localizedDescription.UTF8String); - return CHIP_ERROR_PERSISTED_STORAGE_FAILED; - } + if (!success) { + ChipLogError(DeviceLayer, "Error saving context: %s", error.localizedDescription.UTF8String); + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + } #if CHIP_CONFIG_DARWIN_STORAGE_VERBOSE_LOGGING - fprintf(stderr, "PUT VALUE FOR: '%s': ", key); - for (size_t i = 0; i < value_size; ++i) { - fprintf(stderr, "%02x ", static_cast(value)[i]); - } - fprintf(stderr, "\n"); + fprintf(stderr, "PUT VALUE FOR: '%s': ", key); + for (size_t i = 0; i < value_size; ++i) { + fprintf(stderr, "%02x ", static_cast(value)[i]); + } + fprintf(stderr, "\n"); #endif - return CHIP_NO_ERROR; + return CHIP_NO_ERROR; + } // @autoreleasepool } } // namespace PersistedStorage From b0f527575d4b0e808f0cb06fd192707d38e6de7a Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 24 Sep 2024 18:14:25 -0400 Subject: [PATCH 2/6] MTRDevice should remember new DataVersion values even if the attribute value did not change. (#35756) We used to ignore "same value but new DataVersion" because some devices would spam reports that looked like that and we were constantly hitting storage for the DataVersion updates. But now that we have backoffs on the storage of our cluster data, we can go ahead and just note the new DataVersion and rely on that backoff to prevent hitting storage too much. The test update verifies that: 1) Reports with same value but new DataVersion do get persisted but are subject to the persistence backoff settings. 2) Reports with the same value and same DataVersion do not lead to any storage traffic. --- .../Framework/CHIP/MTRDevice_Concrete.mm | 6 +- .../Framework/CHIPTests/MTRDeviceTests.m | 123 +++++++++++------- 2 files changed, 81 insertions(+), 48 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index 4617a78b161a2a..41962667a8a2fb 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -3533,6 +3533,8 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray *> *)_testAttributeReportWithValue:(unsigned int)testValue +{ + return [self _testAttributeReportWithValue:testValue dataVersion:testValue]; +} + +- (NSArray *> *)_testAttributeReportWithValue:(unsigned int)testValue dataVersion:(unsigned int)dataVersion { return @[ @{ MTRAttributePathKey : [MTRAttributePath attributePathWithEndpointID:@(0) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(MTRAttributeIDTypeClusterLevelControlAttributeCurrentLevelID)], MTRDataKey : @ { - MTRDataVersionKey : @(testValue), + MTRDataVersionKey : @(dataVersion), MTRTypeKey : MTRUnsignedIntegerValueType, MTRValueKey : @(testValue), } @@ -3809,7 +3814,7 @@ - (void)test036_TestStorageBehaviorConfiguration [device setDelegate:delegate queue:queue]; // Use a counter that will be incremented for each report as the value. - unsigned int currentTestValue = 1; + __block unsigned int currentTestValue = 1; // Initial setup: Inject report and see that the attribute persisted. No delay is // expected for the first (priming) report. @@ -3928,54 +3933,84 @@ - (void)test036_TestStorageBehaviorConfiguration XCTAssertLessThan(reportToPersistenceDelay, baseTestDelayTime * 2 * 5 * 1.3); // Test 4: test reporting excessively, and see that persistence does not happen until - // reporting frequency goes back above the threshold - reportEndTime = nil; - dataPersistedTime = nil; - XCTestExpectation * dataPersisted4 = [self expectationWithDescription:@"data persisted 4"]; - delegate.onClusterDataPersisted = ^{ - os_unfair_lock_lock(&lock); - if (!dataPersistedTime) { - dataPersistedTime = [NSDate now]; - } - os_unfair_lock_unlock(&lock); - [dataPersisted4 fulfill]; - }; - - // Set report times with short delay and check that the multiplier is engaged - [device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[ - [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 4)], - [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 3)], - [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 2)], - [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1)], - ]]]; - - // Inject report that makes MTRDevice detect the device is reporting excessively - [device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES]; + // reporting frequency goes back below the threshold + __auto_type excessiveReportTest = ^(unsigned int testId, NSArray *> * (^reportGenerator)(void), bool expectPersistence) { + reportEndTime = nil; + dataPersistedTime = nil; + XCTestExpectation * dataPersisted = [self expectationWithDescription:[NSString stringWithFormat:@"data persisted %u", testId]]; + dataPersisted.inverted = !expectPersistence; + delegate.onClusterDataPersisted = ^{ + os_unfair_lock_lock(&lock); + if (!dataPersistedTime) { + dataPersistedTime = [NSDate now]; + } + os_unfair_lock_unlock(&lock); + [dataPersisted fulfill]; + }; - // Now keep reporting excessively for base delay time max times max multiplier, plus a bit more - NSDate * excessiveStartTime = [NSDate now]; - for (;;) { - usleep((useconds_t) (baseTestDelayTime * 0.1 * USEC_PER_SEC)); - [device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES]; - NSTimeInterval elapsed = -[excessiveStartTime timeIntervalSinceNow]; - if (elapsed > (baseTestDelayTime * 2 * 5 * 1.2)) { - break; + // Set report times with short delay and check that the multiplier is engaged + [device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[ + [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 4)], + [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 3)], + [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 2)], + [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1)], + ]]]; + + // Inject report that makes MTRDevice detect the device is reporting excessively + [device unitTestInjectAttributeReport:reportGenerator() fromSubscription:YES]; + + // Now keep reporting excessively for base delay time max times max multiplier, plus a bit more + NSDate * excessiveStartTime = [NSDate now]; + for (;;) { + usleep((useconds_t) (baseTestDelayTime * 0.1 * USEC_PER_SEC)); + [device unitTestInjectAttributeReport:reportGenerator() fromSubscription:YES]; + NSTimeInterval elapsed = -[excessiveStartTime timeIntervalSinceNow]; + if (elapsed > (baseTestDelayTime * 2 * 5 * 1.2)) { + break; + } } - } - // Check that persistence has not happened because it's now turned off - XCTAssertNil(dataPersistedTime); + // Check that persistence has not happened because it's now turned off + XCTAssertNil(dataPersistedTime); - // Now force report times to large number, to simulate time passage - [device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[ - [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 10)], - ]]]; + // Now force report times to large number, to simulate time passage + [device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[ + [NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 10)], + ]]]; - // And inject a report to trigger MTRDevice to recalculate that this device is no longer - // reporting excessively - [device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES]; + // And inject a report to trigger MTRDevice to recalculate that this device is no longer + // reporting excessively + [device unitTestInjectAttributeReport:reportGenerator() fromSubscription:YES]; + + [self waitForExpectations:@[ dataPersisted ] timeout:60]; + }; - [self waitForExpectations:@[ dataPersisted4 ] timeout:60]; + excessiveReportTest( + 4, ^{ + return [self _testAttributeReportWithValue:currentTestValue++]; + }, true); + + // Test 5: test reporting excessively with the same value and different data + // versions, and see that persistence does not happen until reporting + // frequency goes back below the threshold. + __block __auto_type dataVersion = currentTestValue; + // We incremented currentTestValue after injecting the last report. Make sure all the new + // reports use that last-reported value. + __auto_type lastReportedValue = currentTestValue - 1; + excessiveReportTest( + 5, ^{ + return [self _testAttributeReportWithValue:lastReportedValue dataVersion:dataVersion++]; + }, true); + + // Test 6: test reporting excessively with the same value and same data + // version, and see that persistence does not happen at all. + // We incremented dataVersion after injecting the last report. Make sure all the new + // reports use that last-reported value. + __block __auto_type lastReportedDataVersion = dataVersion - 1; + excessiveReportTest( + 6, ^{ + return [self _testAttributeReportWithValue:lastReportedValue dataVersion:lastReportedDataVersion]; + }, false); delegate.onReportEnd = nil; delegate.onClusterDataPersisted = nil; From ed5c819d624a919f40b8ec333b61fd1ae258e8df Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Tue, 24 Sep 2024 18:06:11 -0700 Subject: [PATCH 3/6] bad comparison (#35763) --- src/darwin/Framework/CHIP/MTRCertificates.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRCertificates.mm b/src/darwin/Framework/CHIP/MTRCertificates.mm index 5837a50a3fe21a..b153cf4442b373 100644 --- a/src/darwin/Framework/CHIP/MTRCertificates.mm +++ b/src/darwin/Framework/CHIP/MTRCertificates.mm @@ -186,7 +186,7 @@ + (BOOL)isCertificate:(MTRCertificateDERBytes)certificate1 equalTo:(MTRCertifica MTR_LOG_ERROR("Can't extract public key from second certificate: %s", ErrorStr(err)); return NO; } - P256PublicKeySpan keySpan2(pubKey1.ConstBytes()); + P256PublicKeySpan keySpan2(pubKey2.ConstBytes()); if (!keySpan1.data_equal(keySpan2)) { return NO; From 34c0fe17542519715e6d8ce7f3b51100be484065 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 24 Sep 2024 23:06:14 -0400 Subject: [PATCH 4/6] Work around random "leaks" failures in CI. (#35762) It seems that "leaks" randomly fails on the new (ARM) Darwin runners. For now, just ignore failures and only fail the test suite if "leaks" ran to completion but detected leaks. --- .../CHIPTests/TestHelpers/MTRTestCase.mm | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm index a418393a0fca79..ee036574734b2b 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm @@ -65,15 +65,26 @@ - (void)setUp - (void)tearDown { #if defined(ENABLE_LEAK_DETECTION) && ENABLE_LEAK_DETECTION + /** + * Unfortunately, doing this in "+ (void)tearDown" (the global suite teardown) + * does not trigger a test failure even if the XCTAssertEqual below fails. + */ if (_detectLeaks) { int pid = getpid(); __auto_type * cmd = [NSString stringWithFormat:@"leaks %d", pid]; int ret = system(cmd.UTF8String); - /** - * Unfortunately, doing this in "+ (void)tearDown" (the global suite teardown) - * does not trigger a test failure even if the XCTAssertEqual fails. - */ - XCTAssertEqual(ret, 0, "LEAKS DETECTED"); + if (WIFEXITED(ret)) { + // leaks ran to completion. + XCTAssertEqual(WEXITSTATUS(ret), 0, "LEAKS DETECTED"); + } else { + // leaks failed to actually run to completion (e.g. crashed or ran + // into some other sort of failure trying to do its work). Ideally + // we would fail our tests in that case, but this seems to be + // happening a fair amount, and randomly, on the ARM GitHub runners. + // Just log and ignore for now. + XCTAssertFalse(WIFSTOPPED(ret), "Not expecting a stopped leaks"); + NSLog(@"Stopped by signal %d", WTERMSIG(ret)); + } } #endif From a643be65a9e7803aa7d7238a7ea589463658f1ff Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 25 Sep 2024 01:46:53 -0400 Subject: [PATCH 5/6] Make the diagnostic log downloader logs clearly say what they are. (#35760) Just saying "BDX" can make people think this is OTA. --- src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm b/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm index c461737cf504a8..fdd8a389f71309 100644 --- a/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm +++ b/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm @@ -476,7 +476,7 @@ - (void)handleBDXTransferSessionBeginForFileDesignator:(NSString *)fileDesignato abortHandler:(AbortHandler)abortHandler; { assertChipStackLockedByCurrentThread(); - MTR_LOG("BDX Transfer Session Begin: %@", fileDesignator); + MTR_LOG("BDX Transfer Session Begin for log download: %@", fileDesignator); auto * download = [_downloads get:fileDesignator fabricIndex:fabricIndex nodeID:nodeID]; VerifyOrReturn(nil != download, completion([MTRError errorForCHIPErrorCode:CHIP_ERROR_NOT_FOUND])); @@ -492,7 +492,7 @@ - (void)handleBDXTransferSessionDataForFileDesignator:(NSString *)fileDesignator completion:(MTRStatusCompletion)completion { assertChipStackLockedByCurrentThread(); - MTR_LOG("BDX Transfer Session Data: %@: %@", fileDesignator, data); + MTR_LOG("BDX Transfer Session Data for log download: %@: %@", fileDesignator, data); auto * download = [_downloads get:fileDesignator fabricIndex:fabricIndex nodeID:nodeID]; VerifyOrReturn(nil != download, completion([MTRError errorForCHIPErrorCode:CHIP_ERROR_NOT_FOUND])); @@ -510,7 +510,7 @@ - (void)handleBDXTransferSessionEndForFileDesignator:(NSString *)fileDesignator error:(NSError * _Nullable)error { assertChipStackLockedByCurrentThread(); - MTR_LOG("BDX Transfer Session End: %@: %@", fileDesignator, error); + MTR_LOG("BDX Transfer Session End for log download: %@: %@", fileDesignator, error); auto * download = [_downloads get:fileDesignator fabricIndex:fabricIndex nodeID:nodeID]; VerifyOrReturn(nil != download); From e9d7b2fffc59f191c2d71f5da4653801bc9e17ca Mon Sep 17 00:00:00 2001 From: Hasty Granbery Date: Wed, 25 Sep 2024 00:59:44 -0700 Subject: [PATCH 6/6] Update TC_TSTAT_4_2 to work with thermostats that are already on a fabric or don't have min/max (#35754) * Don't hard-code the fabric index when removing the secondary fabric * Fall back on Absolute setpoints, and then defaults, when user-configured setpoints are unimplemented --- src/python_testing/TC_TSTAT_4_2.py | 46 ++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/src/python_testing/TC_TSTAT_4_2.py b/src/python_testing/TC_TSTAT_4_2.py index b2916c5914e38a..8ebd2ece70521f 100644 --- a/src/python_testing/TC_TSTAT_4_2.py +++ b/src/python_testing/TC_TSTAT_4_2.py @@ -261,6 +261,8 @@ async def test_TC_TSTAT_4_2(self): nodeId=self.dut_node_id, setupPinCode=params.setupPinCode, filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=1234) + secondary_fabric_index = await self.read_single_attribute_check_success(dev_ctrl=secondary_controller, endpoint=0, cluster=Clusters.Objects.OperationalCredentials, attribute=Clusters.OperationalCredentials.Attributes.CurrentFabricIndex) + current_presets = [] presetTypes = [] presetScenarioCounts = {} @@ -272,26 +274,54 @@ async def test_TC_TSTAT_4_2(self): supportsHeat = self.check_pics("TSTAT.S.F00") supportsCool = self.check_pics("TSTAT.S.F01") - supportsOccupancy = self.check_pics("TSTAT.S.F02") - - occupied = True if supportsHeat: - minHeatSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MinHeatSetpointLimit) - maxHeatSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MaxHeatSetpointLimit) + # If the server supports MinHeatSetpointLimit & MaxHeatSetpointLimit, use those + if self.check_pics("TSTAT.S.A0015") and self.check_pics("TSTAT.S.A0016"): + minHeatSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MinHeatSetpointLimit) + maxHeatSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MaxHeatSetpointLimit) + elif self.check_pics("TSTAT.S.A0003") and self.check_pics("TSTAT.S.A0004"): + # Otherwise, if the server supports AbsMinHeatSetpointLimit & AbsMaxHeatSetpointLimit, use those + minHeatSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.AbsMinHeatSetpointLimit) + maxHeatSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.AbsMaxHeatSetpointLimit) + asserts.assert_true(minHeatSetpointLimit < maxHeatSetpointLimit, "Heat setpoint range invalid") if supportsCool: - minCoolSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MinCoolSetpointLimit) - maxCoolSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MaxCoolSetpointLimit) + # If the server supports MinCoolSetpointLimit & MaxCoolSetpointLimit, use those + if self.check_pics("TSTAT.S.A0017") and self.check_pics("TSTAT.S.A0018"): + minCoolSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MinCoolSetpointLimit) + maxCoolSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MaxCoolSetpointLimit) + elif self.check_pics("TSTAT.S.A0005") and self.check_pics("TSTAT.S.A0006"): + # Otherwise, if the server supports AbsMinCoolSetpointLimit & AbsMaxCoolSetpointLimit, use those + minCoolSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.AbsMinCoolSetpointLimit) + maxCoolSetpointLimit = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.AbsMaxCoolSetpointLimit) + asserts.assert_true(minCoolSetpointLimit < maxCoolSetpointLimit, "Cool setpoint range invalid") + # Servers that do not support occupancy are always "occupied" + occupied = True + + supportsOccupancy = self.check_pics("TSTAT.S.F02") if supportsOccupancy: occupied = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.Occupancy) & 1 + # Target setpoints heatSetpoint = minHeatSetpointLimit + ((maxHeatSetpointLimit - minHeatSetpointLimit) / 2) coolSetpoint = minCoolSetpointLimit + ((maxCoolSetpointLimit - minCoolSetpointLimit) / 2) + # Set the heating and cooling setpoints to something other than the target setpoints + if occupied: + if supportsHeat: + await self.write_single_attribute(attribute_value=cluster.Attributes.OccupiedHeatingSetpoint(heatSetpoint-1), endpoint_id=endpoint) + if supportsCool: + await self.write_single_attribute(attribute_value=cluster.Attributes.OccupiedCoolingSetpoint(coolSetpoint-1), endpoint_id=endpoint) + else: + if supportsHeat: + await self.write_single_attribute(attribute_value=cluster.Attributes.UnoccupiedHeatingSetpoint(heatSetpoint-1), endpoint_id=endpoint) + if supportsCool: + await self.write_single_attribute(attribute_value=cluster.Attributes.UnoccupiedCoolingSetpoint(coolSetpoint-1), endpoint_id=endpoint) + self.step("2") if self.pics_guard(self.check_pics("TSTAT.S.F08") and self.check_pics("TSTAT.S.A0050")): @@ -643,7 +673,7 @@ async def test_TC_TSTAT_4_2(self): await self.send_atomic_request_begin_command(dev_ctrl=secondary_controller) # Primary controller removes the second fabric - await self.send_single_cmd(Clusters.OperationalCredentials.Commands.RemoveFabric(fabricIndex=2), endpoint=0) + await self.send_single_cmd(Clusters.OperationalCredentials.Commands.RemoveFabric(fabricIndex=secondary_fabric_index), endpoint=0) # Send the AtomicRequest begin command from primary controller, which should succeed, as the secondary controller's atomic write state has been cleared status = await self.send_atomic_request_begin_command()