From f6b3594f83bef3d850a140e8e25cf967b0dc1418 Mon Sep 17 00:00:00 2001 From: fesseha-eve <88329315+fessehaeve@users.noreply.github.com> Date: Thu, 2 May 2024 12:29:30 +0200 Subject: [PATCH 01/11] Timesync improvements - part 1 (#32848) * change cluster revision in light-switch-app * added delegate functions for better usability * - made AttemptToGetTimeFromTrustedNode() public API - free readclient memory after read is complete * added default implementation for some delegate functions * added documentation for delegate functions --- .../light-switch-app.matter | 2 +- .../light-switch-common/light-switch-app.zap | 2 +- .../DefaultTimeSyncDelegate.cpp | 22 ----------------- .../DefaultTimeSyncDelegate.h | 5 ---- .../time-synchronization-delegate.h | 24 +++++++++++++++---- .../time-synchronization-server.cpp | 3 +++ .../time-synchronization-server.h | 3 ++- 7 files changed, 26 insertions(+), 35 deletions(-) diff --git a/examples/light-switch-app/light-switch-common/light-switch-app.matter b/examples/light-switch-app/light-switch-common/light-switch-app.matter index e51a772add14e2..65e7c5198a4052 100644 --- a/examples/light-switch-app/light-switch-common/light-switch-app.matter +++ b/examples/light-switch-app/light-switch-common/light-switch-app.matter @@ -2792,7 +2792,7 @@ endpoint 0 { callback attribute acceptedCommandList; callback attribute attributeList; ram attribute featureMap default = 0x0B; - ram attribute clusterRevision default = 1; + ram attribute clusterRevision default = 2; handle command SetUTCTime; handle command SetTrustedTimeSource; diff --git a/examples/light-switch-app/light-switch-common/light-switch-app.zap b/examples/light-switch-app/light-switch-common/light-switch-app.zap index ba56db35a2669b..dcfa64d7c5e88f 100644 --- a/examples/light-switch-app/light-switch-common/light-switch-app.zap +++ b/examples/light-switch-app/light-switch-common/light-switch-app.zap @@ -3826,7 +3826,7 @@ "storageOption": "RAM", "singleton": 0, "bounded": 0, - "defaultValue": "1", + "defaultValue": "2", "reportable": 1, "minInterval": 1, "maxInterval": 65534, diff --git a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp index e436d19f6d6136..59fb8b0bbee24d 100644 --- a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp +++ b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp @@ -24,17 +24,6 @@ using chip::TimeSyncDataProvider; using namespace chip::app::Clusters::TimeSynchronization; -void DefaultTimeSyncDelegate::TimeZoneListChanged(const Span timeZoneList) -{ - // placeholder implementation -} - -bool DefaultTimeSyncDelegate::HandleUpdateDSTOffset(chip::CharSpan name) -{ - // placeholder implementation - return false; -} - bool DefaultTimeSyncDelegate::IsNTPAddressValid(chip::CharSpan ntp) { // placeholder implementation @@ -67,14 +56,3 @@ CHIP_ERROR DefaultTimeSyncDelegate::UpdateTimeFromPlatformSource(chip::Callback: } return CHIP_ERROR_NOT_IMPLEMENTED; } - -CHIP_ERROR DefaultTimeSyncDelegate::UpdateTimeUsingNTPFallback(const CharSpan & fallbackNTP, - chip::Callback::Callback * callback) -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - -void DefaultTimeSyncDelegate::UTCTimeAvailabilityChanged(uint64_t time) -{ - // placeholder implementation -} diff --git a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.h b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.h index 01f156e4ea2ccb..0d78ef2f0fe6e9 100644 --- a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.h +++ b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.h @@ -29,14 +29,9 @@ class DefaultTimeSyncDelegate : public Delegate public: DefaultTimeSyncDelegate() : Delegate(){}; - void TimeZoneListChanged(const Span timeZoneList) override; - bool HandleUpdateDSTOffset(CharSpan name) override; bool IsNTPAddressValid(CharSpan ntp) override; bool IsNTPAddressDomain(CharSpan ntp) override; CHIP_ERROR UpdateTimeFromPlatformSource(chip::Callback::Callback * callback) override; - CHIP_ERROR UpdateTimeUsingNTPFallback(const CharSpan & fallbackNTP, - chip::Callback::Callback * callback) override; - void UTCTimeAvailabilityChanged(uint64_t time) override; }; } // namespace TimeSynchronization diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h b/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h index 7d5bad65ff0d2e..f38153528118fd 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h @@ -56,7 +56,7 @@ class Delegate * * @param timeZoneList new time zone list */ - virtual void TimeZoneListChanged(const Span timeZoneList) = 0; + virtual void TimeZoneListChanged(const Span timeZoneList) {} /** * @brief Give the delegate the chance to call SetDSTOffset on the TimeSynchronizationServer with a list of * DST offsets based on the provided time zone name. If the delegate does so, it should return true. @@ -64,7 +64,7 @@ class Delegate * * @param name name of active time zone */ - virtual bool HandleUpdateDSTOffset(const CharSpan name) = 0; + virtual bool HandleUpdateDSTOffset(const CharSpan name) { return false; } /** * @brief Returns true if the provided string is a valid NTP address (either domain name or IPv6 address). * @@ -104,12 +104,26 @@ class Delegate * a CHIP_ERROR. */ virtual CHIP_ERROR UpdateTimeUsingNTPFallback(const CharSpan & fallbackNTP, - chip::Callback::Callback * callback) = 0; + chip::Callback::Callback * callback) + { + return CHIP_ERROR_NOT_IMPLEMENTED; + } /** - * @brief Signals application that UTCTime has changed through the timesync cluster. + * @brief Signals application that UTCTime has changed through the timesync cluster. This gets called when + * time is available for the first time or is updated. Therefore, @param time will always have a valid value. + * The negative case of time being unavailable is handled by NotifyTimeFailure(). + */ + virtual void UTCTimeAvailabilityChanged(uint64_t time) {} + /** + * @brief Signals application that a new trusted time source is available. The application can then decide + * if it wants to attempt to query for time from this source. + */ + virtual void TrustedTimeSourceAvailabilityChanged(bool available, GranularityEnum granularity) {} + /** + * @brief Signals application that fetching time has failed. The reason is not relevant. */ - virtual void UTCTimeAvailabilityChanged(uint64_t time) = 0; + virtual void NotifyTimeFailure() {} virtual ~Delegate() = default; diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index d03d7842911d51..fda3bb50d7fda0 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -215,6 +215,7 @@ static bool emitTimeFailureEvent(EndpointId ep) // TODO: re-schedule event for after min 1hr if no time is still available // https://github.com/project-chip/connectedhomeip/issues/27200 ChipLogProgress(Zcl, "Emit TimeFailure event [ep=%d]", ep); + GetDelegate()->NotifyTimeFailure(); return true; } @@ -356,6 +357,7 @@ void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) SetUTCTime(kRootEndpointId, mTimeReadInfo->utcTime.Value(), ourGranularity, TimeSourceEnum::kNodeTimeCluster); if (err == CHIP_NO_ERROR) { + mTimeReadInfo = nullptr; return; } } @@ -504,6 +506,7 @@ CHIP_ERROR TimeSynchronizationServer::SetTrustedTimeSource(const DataModel::Null { AttemptToGetTime(); } + GetDelegate()->TrustedTimeSourceAvailabilityChanged(!mTrustedTimeSource.IsNull(), mGranularity); return err; } diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.h b/src/app/clusters/time-synchronization-server/time-synchronization-server.h index ef40eaacc790c4..2581c9712e7862 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.h @@ -123,6 +123,8 @@ class TimeSynchronizationServer : public FabricTable::Delegate // ReadClient::Callback functions void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override; void OnDone(ReadClient * apReadClient) override; + + CHIP_ERROR AttemptToGetTimeFromTrustedNode(); #endif // Platform event handler functions @@ -166,7 +168,6 @@ class TimeSynchronizationServer : public FabricTable::Delegate // Called when the platform is set up - attempts to get time using the recommended source list in the spec. void AttemptToGetTime(); - CHIP_ERROR AttemptToGetTimeFromTrustedNode(); // Attempts to get fallback NTP from the delegate (last available source) // If successful, the function will set mGranulatiry and the time source // If unsuccessful, it will emit a TimeFailure event. From de796eb65a004435fa9eefc6f19b7155f5be1fda Mon Sep 17 00:00:00 2001 From: Thirupathi S <108743108+Thirsrin@users.noreply.github.com> Date: Thu, 2 May 2024 19:20:30 +0530 Subject: [PATCH 02/11] [Linux]DGSW_2_3 fails CurrentHeapUsed is greater than CurrentHeapHighWatermark (#33252) * changes to store max heap size * Restyled by whitespace * Restyled by clang-format * typecast changes --------- Co-authored-by: Restyled.io --- .../Linux/DiagnosticDataProviderImpl.cpp | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/platform/Linux/DiagnosticDataProviderImpl.cpp b/src/platform/Linux/DiagnosticDataProviderImpl.cpp index 74f7003676c051..cc57da65e2d036 100644 --- a/src/platform/Linux/DiagnosticDataProviderImpl.cpp +++ b/src/platform/Linux/DiagnosticDataProviderImpl.cpp @@ -74,6 +74,9 @@ enum class WiFiStatsCountType kWiFiOverrunCount }; +// Static variable to store the maximum heap size +static size_t maxHeapHighWatermark = 0; + CHIP_ERROR GetEthernetStatsCount(EthernetStatsCountType type, uint64_t & count) { CHIP_ERROR err = CHIP_ERROR_READ_FAILED; @@ -244,6 +247,11 @@ CHIP_ERROR DiagnosticDataProviderImpl::GetCurrentHeapUsed(uint64_t & currentHeap // the current running program. currentHeapUsed = mallocInfo.uordblks; + // Update the maximum heap high watermark if the current heap usage exceeds it. + if (currentHeapUsed > maxHeapHighWatermark) + { + maxHeapHighWatermark = currentHeapUsed; + } return CHIP_NO_ERROR; #endif } @@ -260,9 +268,15 @@ CHIP_ERROR DiagnosticDataProviderImpl::GetCurrentHeapHighWatermark(uint64_t & cu // has been used by the Node. // On Linux, since it uses virtual memory, whereby a page of memory could be copied to // the hard disk, called swap space, and free up that page of memory. So it is impossible - // to know accurately peak physical memory it use. We just return the current heap memory - // being used by the current running program. - currentHeapHighWatermark = mallocInfo.uordblks; + // to know accurately peak physical memory it use. + // Update the maximum heap high watermark if the current heap usage exceeds it. + if (mallocInfo.uordblks > static_cast(maxHeapHighWatermark)) + { + maxHeapHighWatermark = mallocInfo.uordblks; + } + + // Set the current heap high watermark. + currentHeapHighWatermark = maxHeapHighWatermark; return CHIP_NO_ERROR; #endif @@ -275,6 +289,8 @@ CHIP_ERROR DiagnosticDataProviderImpl::ResetWatermarks() // On Linux, the write operation is non-op since we always rely on the mallinfo system // function to get the current heap memory. + struct mallinfo mallocInfo = mallinfo(); + maxHeapHighWatermark = mallocInfo.uordblks; return CHIP_NO_ERROR; } From 16b8076551471b21852611cc087a87832e3581f9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 2 May 2024 11:37:26 -0400 Subject: [PATCH 03/11] Fix reference cycle between controller and data store in Matter.framework. (#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. --- .github/workflows/darwin.yaml | 3 + .../CHIP/MTRDeviceControllerDataStore.mm | 66 ++++++++++++++----- .../CHIPTests/MTRPerControllerStorageTests.m | 10 +-- .../CHIPTests/TestHelpers/MTRTestCase.h | 28 ++++++++ .../CHIPTests/TestHelpers/MTRTestCase.mm | 44 +++++++++++++ .../Matter.xcodeproj/project.pbxproj | 6 ++ 6 files changed, 137 insertions(+), 20 deletions(-) create mode 100644 src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h create mode 100644 src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml index dcc025b6e2b32d..62a8d08f09ec4a 100644 --- 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/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 38f0bbccf0fa18..1f515f25565ca5 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -22,6 +22,7 @@ #include #include +#include #include // 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 _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 * _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 * 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,24 +184,27 @@ - (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 @@ -202,7 +212,7 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo // 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,7 +375,10 @@ - (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key - (BOOL)_bulkStoreAttributeCacheValues:(NSDictionary> *)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]; @@ -355,7 +386,10 @@ - (BOOL)_bulkStoreAttributeCacheValues:(NSDictionary -// system dependencies -#import - #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 new file mode 100644 index 00000000000000..d12ea0a1f69f84 --- /dev/null +++ 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 + +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 new file mode 100644 index 00000000000000..b4cf92b1ee3322 --- /dev/null +++ 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 +#include + +#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 index 03b0b105f30feb..be1712972895fb 100644 --- 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 = ""; }; 5124311C2BA0C09A000BC136 /* SetMRPParametersCommand.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = SetMRPParametersCommand.mm; sourceTree = ""; }; 5129BCFC26A9EE3300122DDF /* MTRError.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MTRError.h; sourceTree = ""; }; + 5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRTestCase.mm; sourceTree = ""; }; + 5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRTestCase.h; sourceTree = ""; }; 51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MTRCertificateValidityTests.m; sourceTree = ""; }; 5136660F28067D540025EDAE /* MTRDeviceController_Internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceController_Internal.h; sourceTree = ""; }; 5136661028067D540025EDAE /* MTRDeviceControllerFactory.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerFactory.mm; sourceTree = ""; }; @@ -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 */, From 3fa70cff7b016c24b7875102bc94ae5798c94cef Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 2 May 2024 11:47:47 -0400 Subject: [PATCH 04/11] Replace `DispatchSessionEvent` with `NotifySessionHang` on sessions. (#33259) * Replace `Dispatch event` with `NotifySessionHang` on sessions. Only a single event was ever dispatched and the pattern of passing in pointers to member methods was not used anywhere else and resulted in fairly unique code. Spelling out the `NotifySessionHang` explicitly seems to simplify maintainability. * Update src/transport/Session.h Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> --------- Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> --- src/messaging/ExchangeContext.cpp | 2 +- src/messaging/ReliableMessageMgr.cpp | 2 +- src/transport/Session.h | 10 +++++----- src/transport/SessionDelegate.h | 2 -- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index bca044d91f230d..554e5fbce9482d 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -485,7 +485,7 @@ void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded) { mSession->AsSecureSession()->MarkAsDefunct(); } - mSession->DispatchSessionEvent(&SessionDelegate::OnSessionHang); + mSession->NotifySessionHang(); } } diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 3827fd096d5072..17049069b71430 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -158,7 +158,7 @@ void ReliableMessageMgr::ExecuteActions() { session->AsSecureSession()->MarkAsDefunct(); } - session->DispatchSessionEvent(&SessionDelegate::OnSessionHang); + session->NotifySessionHang(); } // Do not StartTimer, we will schedule the timer at the end of the timer handler. diff --git a/src/transport/Session.h b/src/transport/Session.h index d9840ec3ee33f1..87937eef521110 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -114,8 +114,8 @@ class SessionHolder : public IntrusiveListNodeBase<> Transport::Session * operator->() const { return &mSession.Value().Get(); } - // There is not delegate, nothing to do here - virtual void DispatchSessionEvent(SessionDelegate::Event event) {} + // There is no delegate, nothing to do here + virtual void OnSessionHang() {} protected: // Helper for use by the Grab methods. @@ -147,7 +147,7 @@ class SessionHolderWithDelegate : public SessionHolder SessionHolder::ShiftToSession(session); } - void DispatchSessionEvent(SessionDelegate::Event event) override { (mDelegate.*event)(); } + void OnSessionHang() override { mDelegate.OnSessionHang(); } private: SessionDelegate & mDelegate; @@ -237,7 +237,7 @@ class Session void SetTCPConnection(ActiveTCPConnectionState * conn) { mTCPConnection = conn; } #endif // INET_CONFIG_ENABLE_TCP_ENDPOINT - void DispatchSessionEvent(SessionDelegate::Event event) + void NotifySessionHang() { // Holders might remove themselves when notified. auto holder = mHolders.begin(); @@ -245,7 +245,7 @@ class Session { auto cur = holder; ++holder; - cur->DispatchSessionEvent(event); + cur->OnSessionHang(); } } diff --git a/src/transport/SessionDelegate.h b/src/transport/SessionDelegate.h index 503aaa2b0c4f5c..8de535a265796c 100644 --- a/src/transport/SessionDelegate.h +++ b/src/transport/SessionDelegate.h @@ -51,8 +51,6 @@ class DLL_EXPORT SessionDelegate */ virtual NewSessionHandlingPolicy GetNewSessionHandlingPolicy() { return NewSessionHandlingPolicy::kShiftToNewSession; } - using Event = void (SessionDelegate::*)(); - /** * @brief * Called when a session is releasing. Callees SHALL NOT make synchronous calls into SessionManager to allocate a new session. From 74c95db031b0bbd44ff12de6d844dc6d3621d128 Mon Sep 17 00:00:00 2001 From: Noah Pendleton <2538614+noahp@users.noreply.github.com> Date: Thu, 2 May 2024 14:47:39 -0400 Subject: [PATCH 05/11] Support building chip-tool with python3.12 (#33242) This updates 2 python packages to versions compatible with python3.12, fixing the following errors when running `./scripts/examples/gn_build_example.sh examples/chip-tool BUILDFOLDER`: 1. `pyyaml` 6.0 build error: `AttributeError: cython_sources`, fixed [here](https://github.com/yaml/pyyaml/pull/702) 2. `construct` import error: ``` File "/tmp/pip-install-ifgk4tul/construct_6d60304f85e249759f6c184ea89f1317/construct/core.py", line 3, in import struct, io, binascii, itertools, collections, pickle, sys, os, tempfile, hashlib, importlib, imp ModuleNotFoundError: No module named 'imp' ``` this was fixed [here](https://github.com/construct/construct/commit/91cc0c65f5f8ceace2ee59da5068da92519a99c9), in version `v2.10.57` of `construct`. I'm just updating to latest though. Now running `./scripts/examples/gn_build_example.sh examples/chip-tool BUILDFOLDER` succeeds when the host python is python3.12. --- scripts/setup/constraints.txt | 5 ++--- scripts/setup/requirements.esp32.txt | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/scripts/setup/constraints.txt b/scripts/setup/constraints.txt index 5d0ca805416433..ab4bea6e965a9c 100644 --- 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 index c9043a3f0418c1..b2584bbac08803 100644 --- 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 From e0f9ede83a282e2f8f02c81124b8f4ae800492c4 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 2 May 2024 15:06:07 -0400 Subject: [PATCH 06/11] Fix off-by-one in look checks for QName iterators. (#33273) Unit test sizes for the string `test` were off by one which masked a off-by-one comparison in QName handling. Update unit test and comparisons. This will disallow backward references to "self" for qnames. Co-authored-by: Andrei Litvin --- src/lib/dnssd/minimal_mdns/core/QName.cpp | 2 +- src/lib/dnssd/minimal_mdns/core/tests/TestQName.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib/dnssd/minimal_mdns/core/QName.cpp b/src/lib/dnssd/minimal_mdns/core/QName.cpp index 2cc6488cfb1503..880c9ead298ca3 100644 --- a/src/lib/dnssd/minimal_mdns/core/QName.cpp +++ b/src/lib/dnssd/minimal_mdns/core/QName.cpp @@ -61,7 +61,7 @@ bool SerializedQNameIterator::Next(bool followIndirectPointers) } size_t offset = static_cast(((*mCurrentPosition & 0x3F) << 8) | *(mCurrentPosition + 1)); - if (offset > mLookBehindMax) + if (offset >= mLookBehindMax) { // Potential infinite recursion. mIsValid = false; diff --git a/src/lib/dnssd/minimal_mdns/core/tests/TestQName.cpp b/src/lib/dnssd/minimal_mdns/core/tests/TestQName.cpp index 5f430bd3c76a7a..46c0b517162133 100644 --- a/src/lib/dnssd/minimal_mdns/core/tests/TestQName.cpp +++ b/src/lib/dnssd/minimal_mdns/core/tests/TestQName.cpp @@ -135,7 +135,7 @@ TEST(TestQName, InvalidReferencing) { // Infinite recursion - static const uint8_t kData[] = "\03test\xc0\x00"; + static const uint8_t kData[] = "\04test\xc0\x00"; SerializedQNameIterator it = AsSerializedQName(kData); EXPECT_TRUE(it.Next()); @@ -145,7 +145,7 @@ TEST(TestQName, InvalidReferencing) { // Infinite recursion by referencing own element (inside the stream) - static const uint8_t kData[] = "\03test\xc0\x05"; + static const uint8_t kData[] = "\04test\xc0\x05"; SerializedQNameIterator it = AsSerializedQName(kData); EXPECT_TRUE(it.Next()); @@ -164,7 +164,7 @@ TEST(TestQName, InvalidReferencing) { // Reference that goes forwad instead of backward - static const uint8_t kData[] = "\03test\xc0\x07"; + static const uint8_t kData[] = "\04test\xc0\x07"; SerializedQNameIterator it = AsSerializedQName(kData); EXPECT_TRUE(it.Next()); From baf136e3014bc9c4fad7f411372adb37c3a7f9ad Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 2 May 2024 16:30:52 -0400 Subject: [PATCH 07/11] Fix leaks of keys in Matter.framework unit tests. (#33276) * Fix leaks of keys in Matter.framework unit tests. We need to CFRelease the return from SecKeyCreateWithData, and we were not doing that. * Address review comment. --- src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m | 2 ++ src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m b/src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m index 7a66d299e19e71..65313e92789990 100644 --- a/src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m +++ b/src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m @@ -163,6 +163,8 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo __auto_type * operationalCertificate = [self issueOperationalCertificateForNode:@(kDeviceId) operationalPublicKey:operationalPublicKey]; + // Release no-longer-needed key before we do anything else. + CFRelease(operationalPublicKey); XCTAssertNotNil(operationalCertificate); __auto_type * certChain = [[MTROperationalCertificateChain alloc] initWithOperationalCertificate:operationalCertificate diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 3c6ebfd3b5dc2f..19f114f7877d23 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -158,6 +158,8 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo nodeID:self.nextNodeID caseAuthenticatedTags:nil error:&error]; + // Release no-longer-needed key before we do anything else. + CFRelease(publicKey); XCTAssertNil(error); XCTAssertNotNil(operationalCert); From e29eef4356cad0de4161439fbcc69fdfd8440d0a Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Thu, 2 May 2024 13:33:24 -0700 Subject: [PATCH 08/11] =?UTF-8?q?Remove=20the=20kDNSServiceFlagsTimeout=20?= =?UTF-8?q?flag=20when=20calling=20DNSServiceGetAdd=E2=80=A6=20(#33266)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove the kDNSServiceFlagsTimeout flag when calling DNSServiceGetAddrInfo - We should not be terminating the queries if we do not get anything back quickly The queries should be running until we get something back * Remove the constants for the DNSServiceFlags - Either use the DNSServiceFlags value directly in the DNS SD API calls or define a local scoped variable * Make the registerFlags const * Restyled by clang-format * Address review comment * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/platform/Darwin/DnssdImpl.cpp | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index b80e5958310e68..37406009cf0247 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -41,12 +41,6 @@ constexpr char kSRPDot[] = "default.service.arpa."; // The extra time in milliseconds that we will wait for the resolution on the SRP domain to complete. constexpr uint16_t kSRPTimeoutInMsec = 250; -constexpr DNSServiceFlags kRegisterFlags = kDNSServiceFlagsNoAutoRename; -constexpr DNSServiceFlags kBrowseFlags = kDNSServiceFlagsShareConnection; -constexpr DNSServiceFlags kGetAddrInfoFlags = kDNSServiceFlagsTimeout | kDNSServiceFlagsShareConnection; -constexpr DNSServiceFlags kResolveFlags = kDNSServiceFlagsShareConnection; -constexpr DNSServiceFlags kReconfirmRecordFlags = 0; - bool IsSupportedProtocol(DnssdServiceProtocol protocol) { return (protocol == DnssdServiceProtocol::kDnssdProtocolUdp) || (protocol == DnssdServiceProtocol::kDnssdProtocolTcp); @@ -179,10 +173,11 @@ CHIP_ERROR Register(void * context, DnssdPublishCallback callback, uint32_t inte ChipLogProgress(Discovery, "Registering service %s on host %s with port %u and type: %s on interface id: %" PRIu32, StringOrNullMarker(name), StringOrNullMarker(hostname), port, StringOrNullMarker(type), interfaceId); - RegisterContext * sdCtx = nullptr; + constexpr DNSServiceFlags registerFlags = kDNSServiceFlagsNoAutoRename; + RegisterContext * sdCtx = nullptr; if (CHIP_NO_ERROR == MdnsContexts::GetInstance().GetRegisterContextOfTypeAndName(type, name, &sdCtx)) { - auto err = DNSServiceUpdateRecord(sdCtx->serviceRef, nullptr, kRegisterFlags, record.size(), record.data(), 0 /* ttl */); + auto err = DNSServiceUpdateRecord(sdCtx->serviceRef, nullptr, registerFlags, record.size(), record.data(), 0 /* ttl */); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); return CHIP_NO_ERROR; } @@ -194,7 +189,7 @@ CHIP_ERROR Register(void * context, DnssdPublishCallback callback, uint32_t inte VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); DNSServiceRef sdRef; - err = DNSServiceRegister(&sdRef, kRegisterFlags, interfaceId, name, type, kLocalDot, hostname, htons(port), record.size(), + err = DNSServiceRegister(&sdRef, registerFlags, interfaceId, name, type, kLocalDot, hostname, htons(port), record.size(), record.data(), OnRegister, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); @@ -213,7 +208,7 @@ CHIP_ERROR BrowseOnDomain(BrowseHandler * sdCtx, uint32_t interfaceId, const cha { auto sdRef = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection - auto err = DNSServiceBrowse(&sdRef, kBrowseFlags, interfaceId, type, domain, OnBrowse, sdCtx); + auto err = DNSServiceBrowse(&sdRef, kDNSServiceFlagsShareConnection, interfaceId, type, domain, OnBrowse, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); return CHIP_NO_ERROR; } @@ -334,8 +329,8 @@ static void GetAddrInfo(ResolveContext * sdCtx) ResolveContextWithType * contextWithType = (interface.first.isSRPResult) ? &sdCtx->resolveContextWithSRPType : &sdCtx->resolveContextWithNonSRPType; - auto err = - DNSServiceGetAddrInfo(&sdRefCopy, kGetAddrInfoFlags, interfaceId, protocol, hostname, OnGetAddrInfo, contextWithType); + auto err = DNSServiceGetAddrInfo(&sdRefCopy, kDNSServiceFlagsShareConnection, interfaceId, protocol, hostname, + OnGetAddrInfo, contextWithType); VerifyOrReturn(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); interface.second.isDNSLookUpRequested = true; } @@ -372,7 +367,8 @@ static CHIP_ERROR ResolveWithContext(ResolveContext * sdCtx, uint32_t interfaceI { auto sdRef = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection - auto err = DNSServiceResolve(&sdRef, kResolveFlags, interfaceId, name, type, domain, OnResolve, contextWithType); + auto err = + DNSServiceResolve(&sdRef, kDNSServiceFlagsShareConnection, interfaceId, name, type, domain, OnResolve, contextWithType); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); return CHIP_NO_ERROR; } @@ -655,7 +651,7 @@ CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress return CHIP_ERROR_INVALID_ARGUMENT; } - auto error = DNSServiceReconfirmRecord(kReconfirmRecordFlags, interfaceId, fullname.c_str(), rrtype, rrclass, rdlen, rdata); + auto error = DNSServiceReconfirmRecord(0 /* DNSServiceFlags */, interfaceId, fullname.c_str(), rrtype, rrclass, rdlen, rdata); LogOnFailure(__func__, error); return Error::ToChipError(error); From 997ccd0b389aa7b1f00b66d85d5bd1fcf89f35d4 Mon Sep 17 00:00:00 2001 From: pankore <86098180+pankore@users.noreply.github.com> Date: Fri, 3 May 2024 04:38:19 +0800 Subject: [PATCH 09/11] [Ameba] Add Ameba crypto implementation (#33208) * [Ameba] Add Ameba crypto implementation * Add Ameba cryto implementation for security purposes * Inject only customized operational key storage * Remove the use of chip_crypto = platform, instead keep it as mbedtls, and only inject customized operaional key storage --- .../ameba/main/chipinterface.cpp | 9 + .../ameba/main/chipinterface.cpp | 10 + .../ameba/main/chipinterface.cpp | 9 + .../lighting-app/ameba/main/chipinterface.cpp | 12 +- src/platform/Ameba/BUILD.gn | 2 + src/platform/Ameba/FactoryDataDecoder.cpp | 11 + src/platform/Ameba/FactoryDataDecoder.h | 4 + src/platform/Ameba/FactoryDataProvider.cpp | 23 +- ...baPersistentStorageOperationalKeystore.cpp | 446 ++++++++++++++++++ ...mebaPersistentStorageOperationalKeystore.h | 146 ++++++ 10 files changed, 667 insertions(+), 5 deletions(-) mode change 100644 => 100755 src/platform/Ameba/FactoryDataDecoder.cpp mode change 100644 => 100755 src/platform/Ameba/FactoryDataDecoder.h create mode 100644 src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.cpp create mode 100644 src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.h diff --git a/examples/air-purifier-app/ameba/main/chipinterface.cpp b/examples/air-purifier-app/ameba/main/chipinterface.cpp index fd1c568afbf44f..0829bb041e4882 100644 --- a/examples/air-purifier-app/ameba/main/chipinterface.cpp +++ b/examples/air-purifier-app/ameba/main/chipinterface.cpp @@ -37,6 +37,9 @@ #include #include #include +#if CONFIG_ENABLE_AMEBA_CRYPTO +#include +#endif #include @@ -130,6 +133,12 @@ static void InitServer(intptr_t context) // Init ZCL Data Model and CHIP App Server static chip::CommonCaseDeviceServerInitParams initParams; (void) initParams.InitializeStaticResourcesBeforeServerInit(); +#if CONFIG_ENABLE_AMEBA_CRYPTO + ChipLogProgress(DeviceLayer, "platform crypto enabled!"); + static chip::AmebaPersistentStorageOperationalKeystore sAmebaPersistentStorageOpKeystore; + VerifyOrDie((sAmebaPersistentStorageOpKeystore.Init(initParams.persistentStorageDelegate)) == CHIP_NO_ERROR); + initParams.operationalKeystore = &sAmebaPersistentStorageOpKeystore; +#endif chip::Server::GetInstance().Init(initParams); gExampleDeviceInfoProvider.SetStorageDelegate(&Server::GetInstance().GetPersistentStorage()); chip::DeviceLayer::SetDeviceInfoProvider(&gExampleDeviceInfoProvider); diff --git a/examples/all-clusters-app/ameba/main/chipinterface.cpp b/examples/all-clusters-app/ameba/main/chipinterface.cpp index aa7cc4afe75047..2a35d86eadc3a2 100644 --- a/examples/all-clusters-app/ameba/main/chipinterface.cpp +++ b/examples/all-clusters-app/ameba/main/chipinterface.cpp @@ -41,6 +41,9 @@ #include #include #include +#if CONFIG_ENABLE_AMEBA_CRYPTO +#include +#endif #include #include #include @@ -153,6 +156,13 @@ static void InitServer(intptr_t context) initParams.InitializeStaticResourcesBeforeServerInit(); +#if CONFIG_ENABLE_AMEBA_CRYPTO + ChipLogProgress(DeviceLayer, "platform crypto enabled!"); + static chip::AmebaPersistentStorageOperationalKeystore sAmebaPersistentStorageOpKeystore; + VerifyOrDie((sAmebaPersistentStorageOpKeystore.Init(initParams.persistentStorageDelegate)) == CHIP_NO_ERROR); + initParams.operationalKeystore = &sAmebaPersistentStorageOpKeystore; +#endif + chip::Server::GetInstance().Init(initParams); gExampleDeviceInfoProvider.SetStorageDelegate(&Server::GetInstance().GetPersistentStorage()); // TODO: Use our own DeviceInfoProvider diff --git a/examples/light-switch-app/ameba/main/chipinterface.cpp b/examples/light-switch-app/ameba/main/chipinterface.cpp index c141769ba7d483..35d11da6342ba5 100644 --- a/examples/light-switch-app/ameba/main/chipinterface.cpp +++ b/examples/light-switch-app/ameba/main/chipinterface.cpp @@ -34,6 +34,9 @@ #include #include #include +#if CONFIG_ENABLE_AMEBA_CRYPTO +#include +#endif #include #include #include @@ -100,6 +103,12 @@ static void InitServer(intptr_t context) // Init ZCL Data Model and CHIP App Server static chip::CommonCaseDeviceServerInitParams initParams; initParams.InitializeStaticResourcesBeforeServerInit(); +#if CONFIG_ENABLE_AMEBA_CRYPTO + ChipLogProgress(DeviceLayer, "platform crypto enabled!"); + static chip::AmebaPersistentStorageOperationalKeystore sAmebaPersistentStorageOpKeystore; + VerifyOrDie((sAmebaPersistentStorageOpKeystore.Init(initParams.persistentStorageDelegate)) == CHIP_NO_ERROR); + initParams.operationalKeystore = &sAmebaPersistentStorageOpKeystore; +#endif chip::Server::GetInstance().Init(initParams); gExampleDeviceInfoProvider.SetStorageDelegate(&Server::GetInstance().GetPersistentStorage()); chip::DeviceLayer::SetDeviceInfoProvider(&gExampleDeviceInfoProvider); diff --git a/examples/lighting-app/ameba/main/chipinterface.cpp b/examples/lighting-app/ameba/main/chipinterface.cpp index 461b8bea6143f6..76459b728a545b 100644 --- a/examples/lighting-app/ameba/main/chipinterface.cpp +++ b/examples/lighting-app/ameba/main/chipinterface.cpp @@ -35,12 +35,14 @@ #include #include #include +#if CONFIG_ENABLE_AMEBA_CRYPTO +#include +#endif +#include #include #include #include -#include - #if CONFIG_ENABLE_PW_RPC #include "Rpc.h" #endif @@ -121,6 +123,12 @@ static void InitServer(intptr_t context) // Init ZCL Data Model and CHIP App Server static chip::CommonCaseDeviceServerInitParams initParams; (void) initParams.InitializeStaticResourcesBeforeServerInit(); +#if CONFIG_ENABLE_AMEBA_CRYPTO + ChipLogProgress(DeviceLayer, "platform crypto enabled!"); + static chip::AmebaPersistentStorageOperationalKeystore sAmebaPersistentStorageOpKeystore; + VerifyOrDie((sAmebaPersistentStorageOpKeystore.Init(initParams.persistentStorageDelegate)) == CHIP_NO_ERROR); + initParams.operationalKeystore = &sAmebaPersistentStorageOpKeystore; +#endif chip::Server::GetInstance().Init(initParams); gExampleDeviceInfoProvider.SetStorageDelegate(&Server::GetInstance().GetPersistentStorage()); chip::DeviceLayer::SetDeviceInfoProvider(&gExampleDeviceInfoProvider); diff --git a/src/platform/Ameba/BUILD.gn b/src/platform/Ameba/BUILD.gn index ff05b1fdeb165a..9e06191ae667ca 100755 --- a/src/platform/Ameba/BUILD.gn +++ b/src/platform/Ameba/BUILD.gn @@ -50,6 +50,8 @@ static_library("Ameba") { "SoftwareUpdateManagerImpl.h", "SystemTimeSupport.cpp", "SystemTimeSupport.h", + "crypto/AmebaPersistentStorageOperationalKeystore.cpp", + "crypto/AmebaPersistentStorageOperationalKeystore.h", ] deps = [ diff --git a/src/platform/Ameba/FactoryDataDecoder.cpp b/src/platform/Ameba/FactoryDataDecoder.cpp old mode 100644 new mode 100755 index 4e3536e3de1455..250487342bf0d9 --- a/src/platform/Ameba/FactoryDataDecoder.cpp +++ b/src/platform/Ameba/FactoryDataDecoder.cpp @@ -41,5 +41,16 @@ CHIP_ERROR FactoryDataDecoder::DecodeFactoryData(uint8_t * buffer, FactoryData * return err; } +#if CONFIG_ENABLE_AMEBA_CRYPTO +CHIP_ERROR FactoryDataDecoder::GetSign(uint8_t * PublicKeyData, size_t PublicKeySize, const unsigned char * MessageData, + size_t MessageSize, unsigned char * Signature) +{ + int32_t error = matter_get_signature(PublicKeyData, PublicKeySize, MessageData, MessageSize, Signature); + CHIP_ERROR err = CHIP_NO_ERROR; + + return err; +} +#endif + } // namespace DeviceLayer } // namespace chip diff --git a/src/platform/Ameba/FactoryDataDecoder.h b/src/platform/Ameba/FactoryDataDecoder.h old mode 100644 new mode 100755 index 623eb8e102d093..1aa295d6a4c5e8 --- a/src/platform/Ameba/FactoryDataDecoder.h +++ b/src/platform/Ameba/FactoryDataDecoder.h @@ -27,6 +27,10 @@ class FactoryDataDecoder public: CHIP_ERROR ReadFactoryData(uint8_t * buffer, uint16_t * pfactorydata_len); CHIP_ERROR DecodeFactoryData(uint8_t * buffer, FactoryData * fdata, uint16_t factorydata_len); +#if CONFIG_ENABLE_AMEBA_CRYPTO + CHIP_ERROR GetSign(uint8_t * PublicKeyData, size_t PublicKeySize, const unsigned char * MessageData, size_t MessageSize, + unsigned char * Signature); +#endif static FactoryDataDecoder & GetInstance() { static FactoryDataDecoder instance; diff --git a/src/platform/Ameba/FactoryDataProvider.cpp b/src/platform/Ameba/FactoryDataProvider.cpp index 71243a1b7cfe84..d41f85669014b8 100644 --- a/src/platform/Ameba/FactoryDataProvider.cpp +++ b/src/platform/Ameba/FactoryDataProvider.cpp @@ -16,7 +16,6 @@ */ #include "FactoryDataProvider.h" - #include "FactoryDataDecoder.h" #include #include @@ -254,6 +253,19 @@ CHIP_ERROR FactoryDataProvider::SignWithDeviceAttestationKey(const ByteSpan & me if (kReadFromFlash) { +#if CONFIG_ENABLE_AMEBA_CRYPTO + ReturnErrorCodeIf(!mFactoryData.dac.dac_cert.value, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + // Extract public key from DAC cert. + ByteSpan dacCertSpan{ reinterpret_cast(mFactoryData.dac.dac_cert.value), mFactoryData.dac.dac_cert.len }; + chip::Crypto::P256PublicKey dacPublicKey; + + ReturnErrorOnFailure(chip::Crypto::ExtractPubkeyFromX509Cert(dacCertSpan, dacPublicKey)); + + CHIP_ERROR err = CHIP_NO_ERROR; + FactoryDataDecoder decoder = FactoryDataDecoder::GetInstance(); + err = decoder.GetSign(dacPublicKey.Bytes(), dacPublicKey.Length(), messageToSign.data(), messageToSign.size(), + signature.Bytes()); +#else ReturnErrorCodeIf(!mFactoryData.dac.dac_cert.value, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); ReturnErrorCodeIf(!mFactoryData.dac.dac_key.value, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); // Extract public key from DAC cert. @@ -261,18 +273,23 @@ CHIP_ERROR FactoryDataProvider::SignWithDeviceAttestationKey(const ByteSpan & me chip::Crypto::P256PublicKey dacPublicKey; ReturnErrorOnFailure(chip::Crypto::ExtractPubkeyFromX509Cert(dacCertSpan, dacPublicKey)); + ReturnErrorOnFailure( LoadKeypairFromRaw(ByteSpan(reinterpret_cast(mFactoryData.dac.dac_key.value), mFactoryData.dac.dac_key.len), ByteSpan(dacPublicKey.Bytes(), dacPublicKey.Length()), keypair)); +#endif } else { ReturnErrorOnFailure(LoadKeypairFromRaw(ByteSpan(kDacPrivateKey), ByteSpan(kDacPublicKey), keypair)); } - +#if CONFIG_ENABLE_AMEBA_CRYPTO + VerifyOrReturnError(signature.SetLength(chip::Crypto::kP256_ECDSA_Signature_Length_Raw) == CHIP_NO_ERROR, CHIP_ERROR_INTERNAL); + return CopySpanToMutableSpan(ByteSpan{ signature.ConstBytes(), signature.Length() }, outSignBuffer); +#else ReturnErrorOnFailure(keypair.ECDSA_sign_msg(messageToSign.data(), messageToSign.size(), signature)); - return CopySpanToMutableSpan(ByteSpan{ signature.ConstBytes(), signature.Length() }, outSignBuffer); +#endif } CHIP_ERROR FactoryDataProvider::GetSetupDiscriminator(uint16_t & setupDiscriminator) diff --git a/src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.cpp b/src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.cpp new file mode 100644 index 00000000000000..73358f77ea4af0 --- /dev/null +++ b/src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.cpp @@ -0,0 +1,446 @@ +/* + * Copyright (c) 2022 Project CHIP Authors + * All rights reserved. + * + * 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. + */ + +#if CONFIG_ENABLE_AMEBA_CRYPTO +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include +#include + +#include +#include + +namespace chip { + +using namespace chip::Crypto; + +static inline mbedtls_ecp_keypair * to_keypair(P256KeypairContext * context) +{ + return SafePointerCast(context); +} + +static int CryptoRNG(void * ctxt, uint8_t * out_buffer, size_t out_length) +{ + return (chip::Crypto::DRBG_get_bytes(out_buffer, out_length) == CHIP_NO_ERROR) ? 0 : 1; +} + +CHIP_ERROR AmebaP256Keypair::Initialize(Crypto::ECPKeyTarget key_target) +{ + CHIP_ERROR error = CHIP_NO_ERROR; + int result = 0; + + Clear(); + + mbedtls_ecp_keypair * keypair = to_keypair(&mKeypair); + + VerifyOrExit(matter_get_publickey(Uint8::to_uchar(mPublicKey), mPublicKey.Length()) != 0, + error = CHIP_ERROR_INVALID_PUBLIC_KEY); + + keypair = nullptr; + mInitialized = true; + +exit: + if (keypair != nullptr) + { + keypair = nullptr; + } + + _log_mbedTLS_error(result); + return error; +} + +void AmebaP256Keypair::Clear() +{ + if (mInitialized) + { + mbedtls_ecp_keypair * keypair = to_keypair(&mKeypair); + mbedtls_ecp_keypair_free(keypair); + mInitialized = false; + } +} + +AmebaP256Keypair::~AmebaP256Keypair() +{ + Clear(); +} + +namespace { + +// Tags for our operational keypair storage. +constexpr TLV::Tag kOpKeyVersionTag = TLV::ContextTag(0); +constexpr TLV::Tag kOpKeyDataTag = TLV::ContextTag(1); + +// If this version grows beyond UINT16_MAX, adjust OpKeypairTLVMaxSize +// accordingly. +constexpr uint16_t kOpKeyVersion = 1; + +constexpr size_t OpKeyTLVMaxSize() +{ + // Version and serialized key + return TLV::EstimateStructOverhead(sizeof(uint16_t), Crypto::P256SerializedKeypair::Capacity()); +} + +/** WARNING: This can leave the operational key on the stack somewhere, since many of the platform + * APIs use stack buffers and do not sanitize! This implementation is for example purposes + * only of the API and it is recommended to avoid directly accessing raw private key bits + * in storage. + */ +CHIP_ERROR StoreOperationalKey(FabricIndex fabricIndex, PersistentStorageDelegate * storage, P256Keypair * keypair) +{ + VerifyOrReturnError(IsValidFabricIndex(fabricIndex) && (storage != nullptr) && (keypair != nullptr), + CHIP_ERROR_INVALID_ARGUMENT); + + // Use a SensitiveDataBuffer to get RAII secret data clearing on scope exit. + Crypto::SensitiveDataBuffer buf; + TLV::TLVWriter writer; + + writer.Init(buf.Bytes(), buf.Capacity()); + + TLV::TLVType outerType; + ReturnErrorOnFailure(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, outerType)); + + ReturnErrorOnFailure(writer.Put(kOpKeyVersionTag, kOpKeyVersion)); + + { + // P256SerializedKeypair has RAII secret clearing + Crypto::P256SerializedKeypair serializedOpKey; + size_t len = serializedOpKey.Length() == 0 ? serializedOpKey.Capacity() : serializedOpKey.Length(); + + int result = matter_serialize(serializedOpKey.Bytes(), len); + if (result != 0) + { + return CHIP_ERROR_INTERNAL; + } + ReturnErrorOnFailure(writer.Put(kOpKeyDataTag, ByteSpan(serializedOpKey.Bytes(), len))); + } + + ReturnErrorOnFailure(writer.EndContainer(outerType)); + + const auto opKeyLength = writer.GetLengthWritten(); + VerifyOrReturnError(CanCastTo(opKeyLength), CHIP_ERROR_BUFFER_TOO_SMALL); + ReturnErrorOnFailure(storage->SyncSetKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName(), buf.ConstBytes(), + static_cast(opKeyLength))); + + return CHIP_NO_ERROR; +} + +CHIP_ERROR ExportStoredOpKey(FabricIndex fabricIndex, PersistentStorageDelegate * storage, + Crypto::P256SerializedKeypair & serializedOpKey) +{ + VerifyOrReturnError(storage != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); + + // Use a SensitiveDataBuffer to get RAII secret data clearing on scope exit. + Crypto::SensitiveDataBuffer buf; + + // Load up the operational key structure from storage + uint16_t size = static_cast(buf.Capacity()); + ReturnErrorOnFailure( + storage->SyncGetKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName(), buf.Bytes(), size)); + + buf.SetLength(static_cast(size)); + + // Read-out the operational key TLV entry. + TLV::ContiguousBufferTLVReader reader; + reader.Init(buf.Bytes(), buf.Length()); + + ReturnErrorOnFailure(reader.Next(TLV::kTLVType_Structure, TLV::AnonymousTag())); + TLV::TLVType containerType; + ReturnErrorOnFailure(reader.EnterContainer(containerType)); + + ReturnErrorOnFailure(reader.Next(kOpKeyVersionTag)); + uint16_t opKeyVersion; + ReturnErrorOnFailure(reader.Get(opKeyVersion)); + VerifyOrReturnError(opKeyVersion == kOpKeyVersion, CHIP_ERROR_VERSION_MISMATCH); + + ReturnErrorOnFailure(reader.Next(kOpKeyDataTag)); + { + ByteSpan keyData; + ReturnErrorOnFailure(reader.GetByteView(keyData)); + + // Unfortunately, we have to copy the data into a P256SerializedKeypair. + VerifyOrReturnError(keyData.size() <= serializedOpKey.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL); + + ReturnErrorOnFailure(reader.ExitContainer(containerType)); + + memcpy(serializedOpKey.Bytes(), keyData.data(), keyData.size()); + serializedOpKey.SetLength(keyData.size()); + } + + return CHIP_NO_ERROR; +} + +/** WARNING: This can leave the operational key on the stack somewhere, since many of the platform + * APIs use stack buffers and do not sanitize! This implementation is for example purposes + * only of the API and it is recommended to avoid directly accessing raw private key bits + * in storage. + */ +CHIP_ERROR SignWithStoredOpKey(FabricIndex fabricIndex, PersistentStorageDelegate * storage, const ByteSpan & message, + P256ECDSASignature & outSignature) +{ + VerifyOrReturnError(IsValidFabricIndex(fabricIndex) && (storage != nullptr), CHIP_ERROR_INVALID_ARGUMENT); + + // Use RAII scoping for the transient keypair, to make sure it doesn't get leaked on any error paths. + // Key is put in heap since signature is a costly stack operation and P256Keypair is + // a costly class depending on the backend. + auto transientOperationalKeypair = Platform::MakeUnique(); + if (!transientOperationalKeypair) + { + return CHIP_ERROR_NO_MEMORY; + } + + // Scope 1: Load up the keypair data from storage + P256SerializedKeypair serializedOpKey; + CHIP_ERROR err = ExportStoredOpKey(fabricIndex, storage, serializedOpKey); + if (CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND == err) + { + return CHIP_ERROR_INVALID_FABRIC_INDEX; + } + + // Load-up key material + // WARNING: This makes use of the raw key bits + int result = matter_deserialize(serializedOpKey.Bytes(), serializedOpKey.Length()); + if (result != 0) + { + return CHIP_ERROR_INTERNAL; + } + + // Scope 2: Sign message with the keypair + result = matter_ecdsa_sign_msg(message.data(), message.size(), outSignature.Bytes()); + if (result != 0) + { + return CHIP_ERROR_INTERNAL; + } + + VerifyOrReturnError(outSignature.SetLength(kP256_ECDSA_Signature_Length_Raw) == CHIP_NO_ERROR, err = CHIP_ERROR_INTERNAL); + return err; +} + +} // namespace + +bool AmebaPersistentStorageOperationalKeystore::HasOpKeypairForFabric(FabricIndex fabricIndex) const +{ + VerifyOrReturnError(mStorage != nullptr, false); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex), false); + + // If there was a pending keypair, then there's really a usable key + if (mIsPendingKeypairActive && (fabricIndex == mPendingFabricIndex) && (mPendingKeypair != nullptr)) + { + return true; + } + + // TODO(#16958): need to actually read the key to know if it's there due to platforms not + // properly enforcing CHIP_ERROR_BUFFER_TOO_SMALL behavior needed by + // PersistentStorageDelegate. Very unfortunate, needs fixing ASAP. + + // Use a SensitiveDataBuffer to get RAII secret data clearing on scope exit. + Crypto::SensitiveDataBuffer buf; + + uint16_t keySize = static_cast(buf.Capacity()); + CHIP_ERROR err = + mStorage->SyncGetKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName(), buf.Bytes(), keySize); + + return (err == CHIP_NO_ERROR); +} + +CHIP_ERROR AmebaPersistentStorageOperationalKeystore::NewOpKeypairForFabric(FabricIndex fabricIndex, + MutableByteSpan & outCertificateSigningRequest) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); + // If a key is pending, we cannot generate for a different fabric index until we commit or revert. + if ((mPendingFabricIndex != kUndefinedFabricIndex) && (fabricIndex != mPendingFabricIndex)) + { + return CHIP_ERROR_INVALID_FABRIC_INDEX; + } + VerifyOrReturnError(outCertificateSigningRequest.size() >= Crypto::kMIN_CSR_Buffer_Size, CHIP_ERROR_BUFFER_TOO_SMALL); + + // Replace previous pending keypair, if any was previously allocated + ResetPendingKey(); + + mPendingKeypair = Platform::New(); + VerifyOrReturnError(mPendingKeypair != nullptr, CHIP_ERROR_NO_MEMORY); + + size_t TempLength = outCertificateSigningRequest.size(); + size_t csrLength = matter_gen_new_csr(outCertificateSigningRequest.data(), TempLength); + + if (csrLength <= 0) + { + ResetPendingKey(); + return CHIP_ERROR_INTERNAL; + } + + outCertificateSigningRequest.reduce_size(csrLength); + mPendingFabricIndex = fabricIndex; + + return CHIP_NO_ERROR; +} + +CHIP_ERROR AmebaPersistentStorageOperationalKeystore::ActivateOpKeypairForFabric(FabricIndex fabricIndex, + const Crypto::P256PublicKey & nocPublicKey) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mPendingKeypair != nullptr, CHIP_ERROR_INVALID_FABRIC_INDEX); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex) && (fabricIndex == mPendingFabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); + + // Validate public key being activated matches last generated pending keypair + mPendingKeypair = Platform::New(); + VerifyOrReturnError(mPendingKeypair != nullptr, CHIP_ERROR_NO_MEMORY); + mPendingKeypair->Initialize(Crypto::ECPKeyTarget::ECDSA); + VerifyOrReturnError(mPendingKeypair->Pubkey().Matches(nocPublicKey), CHIP_ERROR_INVALID_PUBLIC_KEY); + + mIsPendingKeypairActive = true; + + return CHIP_NO_ERROR; +} + +CHIP_ERROR AmebaPersistentStorageOperationalKeystore::CommitOpKeypairForFabric(FabricIndex fabricIndex) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mPendingKeypair != nullptr, CHIP_ERROR_INVALID_FABRIC_INDEX); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex) && (fabricIndex == mPendingFabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); + VerifyOrReturnError(mIsPendingKeypairActive == true, CHIP_ERROR_INCORRECT_STATE); + + // Try to store persistent key. On failure, leave everything pending as-is + CHIP_ERROR err = StoreOperationalKey(fabricIndex, mStorage, mPendingKeypair); + ReturnErrorOnFailure(err); + + // If we got here, we succeeded and can reset the pending key: next `SignWithOpKeypair` will use the stored key. + ResetPendingKey(); + return CHIP_NO_ERROR; +} + +CHIP_ERROR AmebaPersistentStorageOperationalKeystore::ExportOpKeypairForFabric(FabricIndex fabricIndex, + Crypto::P256SerializedKeypair & outKeypair) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + return ExportStoredOpKey(fabricIndex, mStorage, outKeypair); +} + +CHIP_ERROR AmebaPersistentStorageOperationalKeystore::RemoveOpKeypairForFabric(FabricIndex fabricIndex) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); + + // Remove pending state if matching + if ((mPendingKeypair != nullptr) && (fabricIndex == mPendingFabricIndex)) + { + RevertPendingKeypair(); + } + + CHIP_ERROR err = mStorage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName()); + if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND) + { + err = CHIP_ERROR_INVALID_FABRIC_INDEX; + } + + return err; +} + +void AmebaPersistentStorageOperationalKeystore::RevertPendingKeypair() +{ + VerifyOrReturn(mStorage != nullptr); + + // Just reset the pending key, we never stored anything + ResetPendingKey(); +} + +CHIP_ERROR AmebaPersistentStorageOperationalKeystore::SignWithOpKeypair(FabricIndex fabricIndex, const ByteSpan & message, + Crypto::P256ECDSASignature & outSignature) const +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); + + if (mIsPendingKeypairActive && (fabricIndex == mPendingFabricIndex)) + { + VerifyOrReturnError(mPendingKeypair != nullptr, CHIP_ERROR_INTERNAL); + // We have an override key: sign with it! + CHIP_ERROR err = CHIP_NO_ERROR; + if (matter_ecdsa_sign_msg(message.data(), message.size(), outSignature.Bytes()) != 0) + { + return CHIP_ERROR_INTERNAL; + } + VerifyOrReturnError(outSignature.SetLength(kP256_ECDSA_Signature_Length_Raw) == CHIP_NO_ERROR, err = CHIP_ERROR_INTERNAL); + return err; + } + + return SignWithStoredOpKey(fabricIndex, mStorage, message, outSignature); +} + +Crypto::P256Keypair * AmebaPersistentStorageOperationalKeystore::AllocateEphemeralKeypairForCASE() +{ + // DO NOT CUT AND PASTE without considering the ReleaseEphemeralKeypair(). + // If allocating a derived class, then `ReleaseEphemeralKeypair` MUST + // de-allocate the derived class after up-casting the base class pointer. + return Platform::New(); +} + +void AmebaPersistentStorageOperationalKeystore::ReleaseEphemeralKeypair(Crypto::P256Keypair * keypair) +{ + // DO NOT CUT AND PASTE without considering the AllocateEphemeralKeypairForCASE(). + // This must delete the same concrete class as allocated in `AllocateEphemeralKeypairForCASE` + Platform::Delete(keypair); +} + +CHIP_ERROR AmebaPersistentStorageOperationalKeystore::MigrateOpKeypairForFabric(FabricIndex fabricIndex, + OperationalKeystore & operationalKeystore) const +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); + + P256SerializedKeypair serializedKeypair; + + // Do not allow overwriting the existing key and just remove it from the previous Operational Keystore if needed. + if (!HasOpKeypairForFabric(fabricIndex)) + { + ReturnErrorOnFailure(operationalKeystore.ExportOpKeypairForFabric(fabricIndex, serializedKeypair)); + + auto operationalKeypair = Platform::MakeUnique(); + if (!operationalKeypair) + { + return CHIP_ERROR_NO_MEMORY; + } + + ReturnErrorOnFailure(operationalKeypair->Deserialize(serializedKeypair)); + ReturnErrorOnFailure(StoreOperationalKey(fabricIndex, mStorage, operationalKeypair.get())); + + ReturnErrorOnFailure(operationalKeystore.RemoveOpKeypairForFabric(fabricIndex)); + } + else if (operationalKeystore.HasOpKeypairForFabric(fabricIndex)) + { + ReturnErrorOnFailure(operationalKeystore.RemoveOpKeypairForFabric(fabricIndex)); + } + + return CHIP_NO_ERROR; +} + +} // namespace chip + +#endif /* CONFIG_ENABLE_AMEBA_CRYPTO */ diff --git a/src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.h b/src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.h new file mode 100644 index 00000000000000..4969e0677764c9 --- /dev/null +++ b/src/platform/Ameba/crypto/AmebaPersistentStorageOperationalKeystore.h @@ -0,0 +1,146 @@ +/* + * Copyright (c) 2022 Project CHIP Authors + * All rights reserved. + * + * 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. + */ + +#pragma once + +#if CONFIG_ENABLE_AMEBA_CRYPTO +#include +#include +#include +#include +#include +#include +#include + +namespace chip { + +class AmebaP256Keypair : public Crypto::P256Keypair +{ +public: + AmebaP256Keypair() {} + ~AmebaP256Keypair() override; + + /** + * @brief Initialize the keypair. + * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise + **/ + CHIP_ERROR Initialize(Crypto::ECPKeyTarget key_target) override; + + /** @brief Return public key for the keypair. + **/ + const Crypto::P256PublicKey & Pubkey() const override { return mPublicKey; } + + /** Release resources associated with this key pair */ + void Clear(); + +protected: + Crypto::P256PublicKey mPublicKey; + mutable Crypto::P256KeypairContext mKeypair; + bool mInitialized = false; +}; + +/** + * @brief OperationalKeystore implementation making use of PersistentStorageDelegate + * to load/store keypairs. This is the legacy behavior of `FabricTable` prior + * to refactors to use `OperationalKeystore` and exists as a baseline example + * of how to use the interface. + * + * WARNING: Ensure that any implementation that uses this one as a starting point + * DOES NOT have the raw key material (in usable form) passed up/down to + * direct storage APIs that may make copies on heap/stack without sanitization. + */ +class AmebaPersistentStorageOperationalKeystore : public Crypto::OperationalKeystore +{ +public: + AmebaPersistentStorageOperationalKeystore() = default; + virtual ~AmebaPersistentStorageOperationalKeystore() { Finish(); } + + // Non-copyable + AmebaPersistentStorageOperationalKeystore(AmebaPersistentStorageOperationalKeystore const &) = delete; + void operator=(AmebaPersistentStorageOperationalKeystore const &) = delete; + + /** + * @brief Initialize the Operational Keystore to map to a given storage delegate. + * + * @param storage Pointer to persistent storage delegate to use. Must outlive this instance. + * @retval CHIP_NO_ERROR on success + * @retval CHIP_ERROR_INCORRECT_STATE if already initialized + */ + CHIP_ERROR Init(PersistentStorageDelegate * storage) + { + VerifyOrReturnError(mStorage == nullptr, CHIP_ERROR_INCORRECT_STATE); + mPendingFabricIndex = kUndefinedFabricIndex; + mIsExternallyOwnedKeypair = false; + mStorage = storage; + mPendingKeypair = nullptr; + mIsPendingKeypairActive = false; + return CHIP_NO_ERROR; + } + + /** + * @brief Finalize the keystore, so that subsequent operations fail + */ + void Finish() + { + VerifyOrReturn(mStorage != nullptr); + + ResetPendingKey(); + mStorage = nullptr; + } + + bool HasPendingOpKeypair() const override { return (mPendingKeypair != nullptr); } + + bool HasOpKeypairForFabric(FabricIndex fabricIndex) const override; + CHIP_ERROR NewOpKeypairForFabric(FabricIndex fabricIndex, MutableByteSpan & outCertificateSigningRequest) override; + CHIP_ERROR ActivateOpKeypairForFabric(FabricIndex fabricIndex, const Crypto::P256PublicKey & nocPublicKey) override; + CHIP_ERROR CommitOpKeypairForFabric(FabricIndex fabricIndex) override; + CHIP_ERROR ExportOpKeypairForFabric(FabricIndex fabricIndex, Crypto::P256SerializedKeypair & outKeypair) override; + CHIP_ERROR RemoveOpKeypairForFabric(FabricIndex fabricIndex) override; + void RevertPendingKeypair() override; + CHIP_ERROR SignWithOpKeypair(FabricIndex fabricIndex, const ByteSpan & message, + Crypto::P256ECDSASignature & outSignature) const override; + Crypto::P256Keypair * AllocateEphemeralKeypairForCASE() override; + void ReleaseEphemeralKeypair(Crypto::P256Keypair * keypair) override; + CHIP_ERROR MigrateOpKeypairForFabric(FabricIndex fabricIndex, OperationalKeystore & operationalKeystore) const override; + +protected: + void ResetPendingKey() + { + if (!mIsExternallyOwnedKeypair && (mPendingKeypair != nullptr)) + { + Platform::Delete(mPendingKeypair); + } + mPendingKeypair = nullptr; + mIsExternallyOwnedKeypair = false; + mIsPendingKeypairActive = false; + mPendingFabricIndex = kUndefinedFabricIndex; + } + + PersistentStorageDelegate * mStorage = nullptr; + + // This pending fabric index is `kUndefinedFabricIndex` if there isn't a pending keypair override for a given fabric. + FabricIndex mPendingFabricIndex = kUndefinedFabricIndex; + Crypto::P256Keypair * mPendingKeypair = nullptr; + bool mIsPendingKeypairActive = false; + + // If overridding NewOpKeypairForFabric method in a subclass, set this to true in + // `NewOpKeypairForFabric` if the mPendingKeypair should not be deleted when no longer in use. + bool mIsExternallyOwnedKeypair = false; +}; + +} // namespace chip +#endif /* CONFIG_ENABLE_AMEBA_CRYPTO */ From ddc06d45295eacc0ee43dcae49b21bd94b7f11d3 Mon Sep 17 00:00:00 2001 From: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com> Date: Thu, 2 May 2024 18:02:23 -0400 Subject: [PATCH 10/11] [Silabs]Migration memMonitoring to cmsisos with some cleanup due to api changes (#33270) * Migration memMonitoring to cmsisos with some cleanup due to api changes change namespace and method names * fixup * Apply suggestion to use new/delete instead of longer MemoryMalloc/MemoryFree api --- examples/platform/silabs/MatterConfig.cpp | 2 +- examples/platform/silabs/MemMonitoring.cpp | 110 ++++++++++----------- examples/platform/silabs/MemMonitoring.h | 13 ++- 3 files changed, 64 insertions(+), 61 deletions(-) diff --git a/examples/platform/silabs/MatterConfig.cpp b/examples/platform/silabs/MatterConfig.cpp index 9cc030557ab0f3..0789c12f37aaac 100644 --- a/examples/platform/silabs/MatterConfig.cpp +++ b/examples/platform/silabs/MatterConfig.cpp @@ -240,7 +240,7 @@ CHIP_ERROR SilabsMatterConfig::InitMatter(const char * appName) #endif #ifdef HEAP_MONITORING - MemMonitoring::startHeapMonitoring(); + MemMonitoring::StartMonitor(); #endif //============================================== diff --git a/examples/platform/silabs/MemMonitoring.cpp b/examples/platform/silabs/MemMonitoring.cpp index 9da075e4e79d42..7ea61e4c8fe265 100644 --- a/examples/platform/silabs/MemMonitoring.cpp +++ b/examples/platform/silabs/MemMonitoring.cpp @@ -19,93 +19,91 @@ #include "MemMonitoring.h" #include "AppConfig.h" -#include "FreeRTOS.h" +#include #include +#include -#define BLE_STACK_TASK_NAME "Bluetooth stack" -#define BLE_LINK_TASK_NAME "Bluetooth linklayer" +namespace chip { +namespace DeviceLayer { +namespace Silabs { -static StackType_t monitoringStack[MONITORING_STACK_SIZE_byte / sizeof(StackType_t)]; -static StaticTask_t monitoringTaskStruct; +static osThreadId_t sMonitorThreadHandle; +constexpr uint32_t kMonitorTaskSize = 1024; +static uint8_t monitorStack[kMonitorTaskSize]; +static osThread_t sMonitorTaskControlBlock; +constexpr osThreadAttr_t kMonitorTaskAttr = { .name = "MemMonitor", + .attr_bits = osThreadDetached, + .cb_mem = &sMonitorTaskControlBlock, + .cb_size = osThreadCbSize, + .stack_mem = monitorStack, + .stack_size = kMonitorTaskSize, + .priority = osPriorityLow }; size_t nbAllocSuccess = 0; size_t nbFreeSuccess = 0; size_t largestBlockAllocated = 0; -void MemMonitoring::startHeapMonitoring() +void MemMonitoring::StartMonitor() { - xTaskCreateStatic(HeapMonitoring, "Monitoring", MONITORING_STACK_SIZE_byte / sizeof(StackType_t), NULL, 1, monitoringStack, - &monitoringTaskStruct); + sMonitorThreadHandle = osThreadNew(MonitorTask, nullptr, &kMonitorTaskAttr); } -void MemMonitoring::HeapMonitoring(void * pvParameter) +void MemMonitoring::MonitorTask(void * pvParameter) { + uint32_t threadCount = osThreadGetCount(); - UBaseType_t appTaskValue; - UBaseType_t bleEventTaskValue; - UBaseType_t bleTaskValue; - UBaseType_t linkLayerTaskValue; - UBaseType_t openThreadTaskValue; - UBaseType_t eventLoopTaskValue; - - TaskHandle_t eventLoopHandleStruct = xTaskGetHandle(CHIP_DEVICE_CONFIG_CHIP_TASK_NAME); - TaskHandle_t otTaskHandle = xTaskGetHandle(CHIP_DEVICE_CONFIG_THREAD_TASK_NAME); - TaskHandle_t appTaskHandle = xTaskGetHandle(APP_TASK_NAME); - TaskHandle_t bleStackTaskHandle = xTaskGetHandle(BLE_STACK_TASK_NAME); - TaskHandle_t bleLinkTaskHandle = xTaskGetHandle(BLE_LINK_TASK_NAME); - TaskHandle_t bleEventTaskHandle = xTaskGetHandle(CHIP_DEVICE_CONFIG_BLE_APP_TASK_NAME); - -#if CHIP_SYSTEM_CONFIG_USE_LWIP - UBaseType_t lwipTaskValue; - TaskHandle_t lwipHandle = xTaskGetHandle(TCPIP_THREAD_NAME); -#endif // CHIP_SYSTEM_CONFIG_USE_LWIP + osThreadId_t * threadIdTable = new osThreadId_t[threadCount]; + // Forms a table of the active thread ids + osThreadEnumerate(threadIdTable, threadCount); while (true) { - appTaskValue = uxTaskGetStackHighWaterMark(appTaskHandle); - bleEventTaskValue = uxTaskGetStackHighWaterMark(bleEventTaskHandle); - bleTaskValue = uxTaskGetStackHighWaterMark(bleStackTaskHandle); - linkLayerTaskValue = uxTaskGetStackHighWaterMark(bleLinkTaskHandle); - openThreadTaskValue = uxTaskGetStackHighWaterMark(otTaskHandle); - eventLoopTaskValue = uxTaskGetStackHighWaterMark(eventLoopHandleStruct); -#if CHIP_SYSTEM_CONFIG_USE_LWIP - lwipTaskValue = uxTaskGetStackHighWaterMark(lwipHandle); -#endif // CHIP_SYSTEM_CONFIG_USE_LWIP SILABS_LOG("============================="); - SILABS_LOG(" "); - SILABS_LOG("Largest Block allocated 0x%x", largestBlockAllocated); - SILABS_LOG("Number Of Successful Alloc 0x%x", nbAllocSuccess); - SILABS_LOG("Number Of Successful Frees 0x%x", nbFreeSuccess); - SILABS_LOG(" "); - SILABS_LOG("App Task most bytes ever Free 0x%x", (appTaskValue * 4)); - SILABS_LOG("BLE Event most bytes ever Free 0x%x", (bleEventTaskValue * 4)); - SILABS_LOG("BLE Stack most bytes ever Free 0x%x", (bleTaskValue * 4)); - SILABS_LOG("Link Layer Task most bytes ever Free 0x%x", (linkLayerTaskValue * 4)); - SILABS_LOG("OpenThread Task most bytes ever Free 0x%x", (openThreadTaskValue * 4)); - SILABS_LOG("Event Loop Task most bytes ever Free 0x%x", (eventLoopTaskValue * 4)); -#if CHIP_SYSTEM_CONFIG_USE_LWIP - SILABS_LOG("LWIP Task most bytes ever Free 0x%x", (lwipTaskValue * 4)); -#endif // CHIP_SYSTEM_CONFIG_USE_LWIP - SILABS_LOG(" "); + SILABS_LOG(" "); + SILABS_LOG("Largest Block allocated %lu B", largestBlockAllocated); + SILABS_LOG("Number Of Successful Alloc %lu", nbAllocSuccess); + SILABS_LOG("Number Of Successful Frees %lu", nbFreeSuccess); + SILABS_LOG(" "); + + SILABS_LOG("Thread stack highwatermark "); + for (uint8_t tIdIndex = 0; tIdIndex < threadCount; tIdIndex++) + { + osThreadId_t tId = threadIdTable[tIdIndex]; + if (tId != sMonitorThreadHandle) // don't print stats for this current debug thread. + { + // The smallest amount of free stack space there has been since the thread creation + SILABS_LOG("\t%-10s : %6lu B", osThreadGetName(tId), osThreadGetStackSpace(tId)); + } + } + + SILABS_LOG(" "); SILABS_LOG("============================="); - vTaskDelay(pdMS_TO_TICKS(5000)); + // run loop every 5 seconds + osDelay(osKernelGetTickFreq() * 5); } + + // will never get here. Still, free allocated memory before exiting + delete threadIdTable; } +} // namespace Silabs +} // namespace DeviceLayer +} // namespace chip + extern "C" void memMonitoringTrackAlloc(void * ptr, size_t size) { if (ptr != NULL) { - nbAllocSuccess++; - if (largestBlockAllocated < size) + chip::DeviceLayer::Silabs::nbAllocSuccess++; + if (chip::DeviceLayer::Silabs::largestBlockAllocated < size) { - largestBlockAllocated = size; + chip::DeviceLayer::Silabs::largestBlockAllocated = size; } } } extern "C" void memMonitoringTrackFree(void * ptr, size_t size) { - nbFreeSuccess++; + chip::DeviceLayer::Silabs::nbFreeSuccess++; } diff --git a/examples/platform/silabs/MemMonitoring.h b/examples/platform/silabs/MemMonitoring.h index ffc430ccaf3193..c54aa2acf421ee 100644 --- a/examples/platform/silabs/MemMonitoring.h +++ b/examples/platform/silabs/MemMonitoring.h @@ -19,17 +19,22 @@ #pragma once #ifdef HEAP_MONITORING -#include "FreeRTOS.h" -#define MONITORING_STACK_SIZE_byte 1024 +namespace chip { +namespace DeviceLayer { +namespace Silabs { class MemMonitoring { public: - static void startHeapMonitoring(); + static void StartMonitor(); private: - static void HeapMonitoring(void * pvParameter); + static void MonitorTask(void * pvParameter); }; +} // namespace Silabs +} // namespace DeviceLayer +} // namespace chip + #endif From fcfc9bcc85afa4054cf84cbc1606b64eb305b94d Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Fri, 3 May 2024 12:13:52 +1200 Subject: [PATCH 11/11] Fix handling of short discriminator in QRCodeSetupPayloadGenerator (#33250) * Fix handling of short discriminator in QRCodeSetupPayloadGenerator If a SetupPayload contains a short discriminator then - isValidQRCodePayload() should return false - trying to generate a QR Code should return INVALID_ARGUMENT - generating with SetAllowInvalidPayload(true) should work (not die) * SetupPayload tweaks Make IsCommonTag, IsVendorTag, and getOptionalVendorData(tag, &info) public. The first two are just encoding spec rules that are useful for clients, and the latter allows clients to read vendor data by tag instead of having to read the whole list. Also use default values instead of explicit constructors for OptionalQRCodeInfo and fix up some doc comments to correctly reference the vendor tag range. * Address review comments Elaborate on the use case for AllowInvalidPayload in the comments, and encode a missing long discriminator as 0, to avoid a client encoding invalid payloads from relying on round-tripping a short discriminator through a QR code. * Handle rendezvousInformation and discriminator the same way * Address review comment: use ValueOr --- .../QRCodeSetupPayloadGenerator.cpp | 12 +++-- src/setup_payload/SetupPayload.cpp | 18 ++----- src/setup_payload/SetupPayload.h | 49 ++++++++----------- src/setup_payload/tests/TestQRCode.cpp | 38 ++++++++++++++ 4 files changed, 72 insertions(+), 45 deletions(-) diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp index 6ffa2319dadb35..962a0393736683 100644 --- a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp +++ b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp @@ -162,6 +162,11 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi size_t totalPayloadSizeInBits = kTotalPayloadDataSizeInBits + (tlvDataLengthInBytes * 8); VerifyOrReturnError(bits.size() * 8 >= totalPayloadSizeInBits, CHIP_ERROR_BUFFER_TOO_SMALL); + // isValidQRCodePayload() has already performed all relevant checks (including that we have a + // long discriminator and rendevouz information). But if AllowInvalidPayload is set these + // requirements might be violated; in that case simply encode 0 for the relevant fields. + // Encoding an invalid (or partially valid) payload is useful for clients that need to be able + // to serialize and deserialize partially populated or invalid payloads. ReturnErrorOnFailure( populateBits(bits.data(), offset, payload.version, kVersionFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure( @@ -170,10 +175,11 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi populateBits(bits.data(), offset, payload.productID, kProductIDFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure(populateBits(bits.data(), offset, static_cast(payload.commissioningFlow), kCommissioningFlowFieldLengthInBits, kTotalPayloadDataSizeInBits)); - VerifyOrReturnError(payload.rendezvousInformation.HasValue(), CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.rendezvousInformation.Value().Raw(), + ReturnErrorOnFailure(populateBits(bits.data(), offset, + payload.rendezvousInformation.ValueOr(RendezvousInformationFlag::kNone).Raw(), kRendezvousInfoFieldLengthInBits, kTotalPayloadDataSizeInBits)); - ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.discriminator.GetLongValue(), + auto const & pd = payload.discriminator; + ReturnErrorOnFailure(populateBits(bits.data(), offset, (!pd.IsShortDiscriminator() ? pd.GetLongValue() : 0), kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure( populateBits(bits.data(), offset, payload.setUpPINCode, kSetupPINCodeFieldLengthInBits, kTotalPayloadDataSizeInBits)); diff --git a/src/setup_payload/SetupPayload.cpp b/src/setup_payload/SetupPayload.cpp index 3d312cd5846e8d..063456327f6a62 100644 --- a/src/setup_payload/SetupPayload.cpp +++ b/src/setup_payload/SetupPayload.cpp @@ -33,18 +33,6 @@ namespace chip { -// Spec 5.1.4.2 CHIPCommon tag numbers are in the range [0x00, 0x7F] -bool SetupPayload::IsCommonTag(uint8_t tag) -{ - return tag < 0x80; -} - -// Spec 5.1.4.1 Manufacture-specific tag numbers are in the range [0x80, 0xFF] -bool SetupPayload::IsVendorTag(uint8_t tag) -{ - return !IsCommonTag(tag); -} - // Check the Setup Payload for validity // // `vendor_id` and `product_id` are allowed all of uint16_t @@ -79,7 +67,11 @@ bool PayloadContents::isValidQRCodePayload() const return false; } - // Discriminator validity is enforced by the SetupDiscriminator class. + // General discriminator validity is enforced by the SetupDiscriminator class, but it can't be short for QR a code. + if (discriminator.IsShortDiscriminator()) + { + return false; + } if (setUpPINCode >= 1 << kSetupPINCodeFieldLengthInBits) { diff --git a/src/setup_payload/SetupPayload.h b/src/setup_payload/SetupPayload.h index 9b18574480ba6e..6e81e0e0e48ddb 100644 --- a/src/setup_payload/SetupPayload.h +++ b/src/setup_payload/SetupPayload.h @@ -153,29 +153,19 @@ enum optionalQRCodeInfoType */ struct OptionalQRCodeInfo { - OptionalQRCodeInfo() { int32 = 0; } - /*@{*/ uint8_t tag; /**< the tag number of the optional info */ enum optionalQRCodeInfoType type; /**< the type (String or Int) of the optional info */ std::string data; /**< the string value if type is optionalQRCodeInfoTypeString, otherwise should not be set */ - int32_t int32; /**< the integer value if type is optionalQRCodeInfoTypeInt, otherwise should not be set */ + int32_t int32 = 0; /**< the integer value if type is optionalQRCodeInfoTypeInt32, otherwise should not be set */ /*@}*/ }; struct OptionalQRCodeInfoExtension : OptionalQRCodeInfo { - OptionalQRCodeInfoExtension() - { - int32 = 0; - int64 = 0; - uint32 = 0; - uint64 = 0; - } - - int64_t int64; - uint64_t uint32; - uint64_t uint64; + int64_t int64 = 0; /**< the integer value if type is optionalQRCodeInfoTypeInt64, otherwise should not be set */ + uint64_t uint32 = 0; /**< the integer value if type is optionalQRCodeInfoTypeUInt32, otherwise should not be set */ + uint64_t uint64 = 0; /**< the integer value if type is optionalQRCodeInfoTypeUInt64, otherwise should not be set */ }; class SetupPayload : public PayloadContents @@ -193,17 +183,25 @@ class SetupPayload : public PayloadContents CHIP_ERROR addOptionalVendorData(uint8_t tag, std::string data); /** @brief A function to add an optional vendor data - * @param tag 7 bit [0-127] tag number + * @param tag tag number in the [0x80-0xFF] range * @param data Integer representation of data to add * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise **/ CHIP_ERROR addOptionalVendorData(uint8_t tag, int32_t data); /** @brief A function to remove an optional vendor data - * @param tag 7 bit [0-127] tag number + * @param tag tag number in the [0x80-0xFF] range * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise **/ CHIP_ERROR removeOptionalVendorData(uint8_t tag); + + /** @brief A function to retrieve an optional QR Code info vendor object + * @param tag tag number in the [0x80-0xFF] range + * @param info retrieved OptionalQRCodeInfo object + * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise + **/ + CHIP_ERROR getOptionalVendorData(uint8_t tag, OptionalQRCodeInfo & info) const; + /** * @brief A function to retrieve the vector of OptionalQRCodeInfo infos * @return Returns a vector of optionalQRCodeInfos @@ -235,21 +233,21 @@ class SetupPayload : public PayloadContents bool operator==(const SetupPayload & input) const; -private: - std::map optionalVendorData; - std::map optionalExtensionData; - /** @brief Checks if the tag is CHIP Common type * @param tag Tag to be checked * @return Returns True if the tag is of Common type **/ - static bool IsCommonTag(uint8_t tag); + static bool IsCommonTag(uint8_t tag) { return tag < 0x80; } /** @brief Checks if the tag is vendor-specific * @param tag Tag to be checked * @return Returns True if the tag is Vendor-specific **/ - static bool IsVendorTag(uint8_t tag); + static bool IsVendorTag(uint8_t tag) { return !IsCommonTag(tag); } + +private: + std::map optionalVendorData; + std::map optionalExtensionData; /** @brief A function to add an optional QR Code info vendor object * @param info Optional QR code info object to add @@ -269,13 +267,6 @@ class SetupPayload : public PayloadContents **/ std::vector getAllOptionalExtensionData() const; - /** @brief A function to retrieve an optional QR Code info vendor object - * @param tag 7 bit [0-127] tag number - * @param info retrieved OptionalQRCodeInfo object - * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise - **/ - CHIP_ERROR getOptionalVendorData(uint8_t tag, OptionalQRCodeInfo & info) const; - /** @brief A function to retrieve an optional QR Code info extended object * @param tag 8 bit [128-255] tag number * @param info retrieved OptionalQRCodeInfoExtension object diff --git a/src/setup_payload/tests/TestQRCode.cpp b/src/setup_payload/tests/TestQRCode.cpp index 1ec508578dec30..6ca349c2884341 100644 --- a/src/setup_payload/tests/TestQRCode.cpp +++ b/src/setup_payload/tests/TestQRCode.cpp @@ -381,6 +381,44 @@ TEST(TestQRCode, TestQRCodeToPayloadGeneration) EXPECT_EQ(result, true); } +TEST(TestQRCode, TestGenerateWithShortDiscriminatorInvalid) +{ + SetupPayload payload = GetDefaultPayload(); + EXPECT_TRUE(payload.isValidQRCodePayload()); + + // A short discriminator isn't valid for a QR Code + payload.discriminator.SetShortValue(1); + EXPECT_FALSE(payload.isValidQRCodePayload()); + + // QRCodeSetupPayloadGenerator should therefore return an error + string base38Rep; + QRCodeSetupPayloadGenerator generator(payload); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_ERROR_INVALID_ARGUMENT); + + // If we allow invalid payloads we should be able to encode + generator.SetAllowInvalidPayload(true); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_NO_ERROR); +} + +TEST(TestQRCode, TestGenerateWithoutRendezvousInformation) +{ + SetupPayload payload = GetDefaultPayload(); + EXPECT_TRUE(payload.isValidQRCodePayload()); + + // Rendezvouz Information is required for a QR code + payload.rendezvousInformation.ClearValue(); + EXPECT_FALSE(payload.isValidQRCodePayload()); + + // QRCodeSetupPayloadGenerator should therefore return an error + string base38Rep; + QRCodeSetupPayloadGenerator generator(payload); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_ERROR_INVALID_ARGUMENT); + + // If we allow invalid payloads we should be able to encode + generator.SetAllowInvalidPayload(true); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_NO_ERROR); +} + TEST(TestQRCode, TestExtractPayload) { EXPECT_EQ(QRCodeSetupPayloadParser::ExtractPayload(string("MT:ABC")), string("ABC"));