Skip to content

Commit 80cb929

Browse files
Darwin: MTRDeviceControllerFactory improvements (#32997)
* Darwin: MTRDeviceControllerFactory improvements Handle cleanup after failed factory start in the same dispatch_sync * Darwin: OOM handling at the framework level is unnecessary * Lint * Use copy instead of dataWithData
1 parent 035d302 commit 80cb929

7 files changed

+69
-173
lines changed

src/darwin/Framework/CHIP/MTRBaseDevice.mm

+1-5
Original file line numberDiff line numberDiff line change
@@ -1840,11 +1840,7 @@ static CHIP_ERROR OpenCommissioningWindow(Controller::DeviceController * control
18401840
CHIP_ERROR OpenCommissioningWindowHelper::OpenCommissioningWindow(Controller::DeviceController * controller, NodeId nodeID,
18411841
System::Clock::Seconds16 timeout, uint16_t discriminator, const Optional<uint32_t> & setupPIN, ResultCallback callback)
18421842
{
1843-
auto * self = new (std::nothrow) OpenCommissioningWindowHelper(controller, callback);
1844-
if (self == nullptr) {
1845-
return CHIP_ERROR_NO_MEMORY;
1846-
}
1847-
1843+
auto * self = new OpenCommissioningWindowHelper(controller, callback);
18481844
SetupPayload unused;
18491845
CHIP_ERROR err = self->mOpener.OpenCommissioningWindow(nodeID, timeout, Crypto::kSpake2p_Min_PBKDF_Iterations, discriminator,
18501846
setupPIN, NullOptional, &self->mOnOpenCommissioningWindowCallback, unused);

src/darwin/Framework/CHIP/MTRDeviceController.mm

-4
Original file line numberDiff line numberDiff line change
@@ -413,10 +413,6 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
413413
}
414414

415415
_cppCommissioner = new chip::Controller::DeviceCommissioner();
416-
if (_cppCommissioner == nullptr) {
417-
[self checkForStartError:CHIP_ERROR_NO_MEMORY logMsg:kErrorCommissionerInit];
418-
return;
419-
}
420416

421417
// nocBuffer might not be used, but if it is it needs to live
422418
// long enough (until after we are done using

src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm

+52-127
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,6 @@
6767
using namespace chip;
6868
using namespace chip::Controller;
6969

70-
static NSString * const kErrorPersistentStorageInit = @"Init failure while creating a persistent storage delegate";
71-
static NSString * const kErrorSessionResumptionStorageInit = @"Init failure while creating a session resumption storage delegate";
72-
static NSString * const kErrorControllerFactoryInit = @"Init failure while initializing controller factory";
73-
static NSString * const kErrorKeystoreInit = @"Init failure while initializing persistent storage keystore";
74-
static NSString * const kErrorCertStoreInit = @"Init failure while initializing persistent storage operational certificate store";
75-
7670
static bool sExitHandlerRegistered = false;
7771
static void ShutdownOnExit()
7872
{
@@ -228,21 +222,6 @@ - (void)_assertCurrentQueueIsNotMatterQueue
228222
VerifyOrDie(!DeviceLayer::PlatformMgrImpl().IsWorkQueueCurrentQueue());
229223
}
230224

231-
- (BOOL)checkIsRunning:(NSError * __autoreleasing *)error
232-
{
233-
[self _assertCurrentQueueIsNotMatterQueue];
234-
235-
if ([self isRunning]) {
236-
return YES;
237-
}
238-
239-
if (error != nil) {
240-
*error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE];
241-
}
242-
243-
return NO;
244-
}
245-
246225
- (void)cleanupStartupObjects
247226
{
248227
assertChipStackLockedByCurrentThread();
@@ -293,7 +272,7 @@ - (CHIP_ERROR)_initFabricTable:(FabricTable &)fabricTable
293272
{
294273
[self _assertCurrentQueueIsNotMatterQueue];
295274

296-
if (!self.isRunning) {
275+
if (!_running) { // Note: reading _running from outside of the Matter work queue
297276
return nil;
298277
}
299278

@@ -339,40 +318,25 @@ - (BOOL)_startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParam
339318
{
340319
[self _assertCurrentQueueIsNotMatterQueue];
341320

342-
if ([self isRunning]) {
343-
MTR_LOG_DEBUG("Ignoring duplicate call to startup, Matter controller factory already started...");
344-
return YES;
345-
}
346-
347-
__block CHIP_ERROR errorCode = CHIP_NO_ERROR;
321+
__block CHIP_ERROR err = CHIP_ERROR_INTERNAL;
348322
dispatch_sync(_chipWorkQueue, ^{
349-
if ([self isRunning]) {
350-
return;
323+
if (_running) {
324+
// TODO: When treating a duplicate call as success we should validate parameters match
325+
MTR_LOG_DEBUG("Ignoring duplicate call to startup, Matter controller factory already started...");
326+
ExitNow(err = CHIP_NO_ERROR);
351327
}
352328

353329
StartupMetricsCollection();
354330
InitializeServerAccessControl();
355331

356332
if (startupParams.hasStorage) {
357-
_persistentStorageDelegate = new (std::nothrow) MTRPersistentStorageDelegateBridge(startupParams.storage);
333+
_persistentStorageDelegate = new MTRPersistentStorageDelegateBridge(startupParams.storage);
358334
_sessionResumptionStorage = nullptr;
359335
_usingPerControllerStorage = NO;
360336
} else {
361-
_persistentStorageDelegate = new (std::nothrow) MTRDemuxingStorage(self);
362-
_sessionResumptionStorage = new (std::nothrow) MTRSessionResumptionStorageBridge(self);
337+
_persistentStorageDelegate = new MTRDemuxingStorage(self);
338+
_sessionResumptionStorage = new MTRSessionResumptionStorageBridge(self);
363339
_usingPerControllerStorage = YES;
364-
365-
if (_sessionResumptionStorage == nil) {
366-
MTR_LOG_ERROR("Error: %@", kErrorSessionResumptionStorageInit);
367-
errorCode = CHIP_ERROR_NO_MEMORY;
368-
return;
369-
}
370-
}
371-
372-
if (_persistentStorageDelegate == nil) {
373-
MTR_LOG_ERROR("Error: %@", kErrorPersistentStorageInit);
374-
errorCode = CHIP_ERROR_NO_MEMORY;
375-
return;
376340
}
377341

378342
_otaProviderDelegate = startupParams.otaProviderDelegate;
@@ -383,63 +347,42 @@ - (BOOL)_startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParam
383347

384348
// TODO: Allow passing a different keystore implementation via startupParams.
385349
_keystore = new PersistentStorageOperationalKeystore();
386-
if (_keystore == nullptr) {
387-
MTR_LOG_ERROR("Error: %@", kErrorKeystoreInit);
388-
errorCode = CHIP_ERROR_NO_MEMORY;
389-
return;
390-
}
391-
392-
errorCode = _keystore->Init(_persistentStorageDelegate);
393-
if (errorCode != CHIP_NO_ERROR) {
394-
MTR_LOG_ERROR("Error: %@", kErrorKeystoreInit);
395-
return;
396-
}
350+
SuccessOrExit(err = _keystore->Init(_persistentStorageDelegate));
397351

398352
// TODO Allow passing a different opcert store implementation via startupParams.
399353
_opCertStore = new Credentials::PersistentStorageOpCertStore();
400-
if (_opCertStore == nullptr) {
401-
MTR_LOG_ERROR("Error: %@", kErrorCertStoreInit);
402-
errorCode = CHIP_ERROR_NO_MEMORY;
403-
return;
404-
}
405-
406-
errorCode = _opCertStore->Init(_persistentStorageDelegate);
407-
if (errorCode != CHIP_NO_ERROR) {
408-
MTR_LOG_ERROR("Error: %@", kErrorCertStoreInit);
409-
return;
410-
}
354+
SuccessOrExit(err = _opCertStore->Init(_persistentStorageDelegate));
411355

412356
_productAttestationAuthorityCertificates = [startupParams.productAttestationAuthorityCertificates copy];
413357
_certificationDeclarationCertificates = [startupParams.certificationDeclarationCertificates copy];
414358

415-
chip::Controller::FactoryInitParams params;
416-
if (startupParams.port != nil) {
417-
params.listenPort = [startupParams.port unsignedShortValue];
418-
}
419-
params.enableServerInteractions = startupParams.shouldStartServer;
420-
421-
params.groupDataProvider = &_groupDataProvider;
422-
params.sessionKeystore = &_sessionKeystore;
423-
params.fabricIndependentStorage = _persistentStorageDelegate;
424-
params.operationalKeystore = _keystore;
425-
params.opCertStore = _opCertStore;
426-
params.certificateValidityPolicy = &_certificateValidityPolicy;
427-
params.sessionResumptionStorage = _sessionResumptionStorage;
428-
errorCode = _controllerFactory->Init(params);
429-
if (errorCode != CHIP_NO_ERROR) {
430-
MTR_LOG_ERROR("Error: %@", kErrorControllerFactoryInit);
431-
return;
359+
{
360+
chip::Controller::FactoryInitParams params;
361+
if (startupParams.port != nil) {
362+
params.listenPort = [startupParams.port unsignedShortValue];
363+
}
364+
params.enableServerInteractions = startupParams.shouldStartServer;
365+
366+
params.groupDataProvider = &_groupDataProvider;
367+
params.sessionKeystore = &_sessionKeystore;
368+
params.fabricIndependentStorage = _persistentStorageDelegate;
369+
params.operationalKeystore = _keystore;
370+
params.opCertStore = _opCertStore;
371+
params.certificateValidityPolicy = &_certificateValidityPolicy;
372+
params.sessionResumptionStorage = _sessionResumptionStorage;
373+
SuccessOrExit(err = _controllerFactory->Init(params));
432374
}
433375

434376
// This needs to happen after DeviceControllerFactory::Init,
435377
// because that creates (lazily, by calling functions with
436378
// static variables in them) some static-lifetime objects.
437379
if (!sExitHandlerRegistered) {
438-
int ret = atexit(ShutdownOnExit);
439-
if (ret != 0) {
440-
MTR_LOG_ERROR("Error registering exit handler: %d", ret);
441-
return;
380+
if (atexit(ShutdownOnExit) != 0) {
381+
char error[128];
382+
strerror_r(errno, error, sizeof(error));
383+
MTR_LOG_ERROR("Warning: Failed to register atexit handler: %s", error);
442384
}
385+
sExitHandlerRegistered = true;
443386
}
444387
HeapObjectPoolExitHandling::IgnoreLeaksOnExit();
445388

@@ -452,47 +395,44 @@ - (BOOL)_startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParam
452395
_controllerFactory->RetainSystemState();
453396
_controllerFactory->ReleaseSystemState();
454397

455-
self->_advertiseOperational = startupParams.shouldStartServer;
456-
self->_running = YES;
457-
});
398+
_advertiseOperational = startupParams.shouldStartServer;
399+
_running = YES;
400+
err = CHIP_NO_ERROR;
458401

459-
if (![self isRunning]) {
460-
dispatch_sync(_chipWorkQueue, ^{
402+
exit:
403+
if (err != CHIP_NO_ERROR) {
404+
// Note: Since we have failed no later than _controllerFactory->Init(),
405+
// there is no need to call _controllerFactory->Shutdown() here.
461406
[self cleanupStartupObjects];
462-
});
407+
}
408+
});
409+
if (err != CHIP_NO_ERROR) {
410+
MTR_LOG_ERROR("Failed to start Matter controller factory: %" CHIP_ERROR_FORMAT, err.Format());
463411
if (error != nil) {
464-
*error = [MTRError errorForCHIPErrorCode:errorCode];
412+
*error = [MTRError errorForCHIPErrorCode:err];
465413
}
466414
return NO;
467415
}
468-
469416
return YES;
470417
}
471418

472419
- (void)stopControllerFactory
473420
{
474421
[self _assertCurrentQueueIsNotMatterQueue];
475422

476-
if (![self isRunning]) {
477-
return;
478-
}
479-
480423
while ([_controllers count] != 0) {
481424
[_controllers[0] shutdown];
482425
}
483426

484427
dispatch_sync(_chipWorkQueue, ^{
428+
VerifyOrReturn(_running);
429+
485430
MTR_LOG_INFO("Shutting down the Matter controller factory");
486431
_controllerFactory->Shutdown();
487-
488432
[self cleanupStartupObjects];
433+
_running = NO;
434+
_advertiseOperational = NO;
489435
});
490-
491-
// NOTE: we do not call cleanupInitObjects because we can be restarted, and
492-
// that does not re-create the objects that we create inside init.
493-
// Maybe we should be creating them in startup?
494-
495-
_running = NO;
496436
}
497437

498438
/**
@@ -540,7 +480,7 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController *
540480
return nil;
541481
}
542482

543-
if (![self isRunning]) {
483+
if (!_running) { // Note: reading _running from outside of the Matter work queue
544484
if (storageDelegate != nil) {
545485
MTR_LOG_DEFAULT("Auto-starting Matter controller factory in per-controller storage mode");
546486
auto * params = [[MTRDeviceControllerFactoryParams alloc] initWithoutStorage];
@@ -744,13 +684,8 @@ - (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceCo
744684
keystore:self->_keystore
745685
advertiseOperational:self->_advertiseOperational
746686
params:startupParams];
747-
if (params == nil) {
748-
fabricError = CHIP_ERROR_NO_MEMORY;
749-
} else {
750-
params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
751-
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
752-
}
753-
687+
params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
688+
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
754689
return params;
755690
}
756691
error:error];
@@ -800,12 +735,8 @@ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControl
800735
keystore:self->_keystore
801736
advertiseOperational:self->_advertiseOperational
802737
params:startupParams];
803-
if (params == nil) {
804-
fabricError = CHIP_ERROR_NO_MEMORY;
805-
} else {
806-
params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
807-
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
808-
}
738+
params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
739+
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
809740
return params;
810741
}
811742
error:error];
@@ -907,12 +838,6 @@ - (MTRDeviceController * _Nullable)maybeInitializeOTAProvider:(MTRDeviceControll
907838

908839
- (void)resetOperationalAdvertising
909840
{
910-
if (![self checkIsRunning:nil]) {
911-
// No need to reset anything; we are not running, so not
912-
// advertising.
913-
return;
914-
}
915-
916841
if (!_advertiseOperational) {
917842
// No need to reset anything; we are not advertising the things that
918843
// would need to get reset.

src/darwin/Framework/CHIP/MTRManualSetupPayloadParser.mm

+5-13
Original file line numberDiff line numberDiff line change
@@ -50,19 +50,11 @@ - (MTRSetupPayload *)populatePayload:(NSError * __autoreleasing *)error
5050
chip::SetupPayload cPlusPluspayload;
5151
MTRSetupPayload * payload;
5252

53-
if (_chipManualSetupPayloadParser) {
54-
CHIP_ERROR chipError = _chipManualSetupPayloadParser->populatePayload(cPlusPluspayload);
55-
56-
if (chipError == CHIP_NO_ERROR) {
57-
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
58-
} else if (error) {
59-
*error = [MTRError errorForCHIPErrorCode:chipError];
60-
}
61-
} else {
62-
// Memory init has failed
63-
if (error) {
64-
*error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY];
65-
}
53+
CHIP_ERROR chipError = _chipManualSetupPayloadParser->populatePayload(cPlusPluspayload);
54+
if (chipError == CHIP_NO_ERROR) {
55+
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
56+
} else if (error) {
57+
*error = [MTRError errorForCHIPErrorCode:chipError];
6658
}
6759

6860
return payload;

src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm

+2-11
Original file line numberDiff line numberDiff line change
@@ -64,17 +64,8 @@
6464

6565
// Make copies of the certificates, just in case the API consumer
6666
// has them as MutableData.
67-
mRootCert = [NSData dataWithData:rootCert];
68-
if (mRootCert == nil) {
69-
return CHIP_ERROR_NO_MEMORY;
70-
}
71-
72-
if (icaCert != nil) {
73-
mIntermediateCert = [NSData dataWithData:icaCert];
74-
if (mIntermediateCert == nil) {
75-
return CHIP_ERROR_NO_MEMORY;
76-
}
77-
}
67+
mRootCert = [rootCert copy];
68+
mIntermediateCert = [icaCert copy];
7869

7970
return CHIP_NO_ERROR;
8071
}

src/darwin/Framework/CHIP/MTRQRCodeSetupPayloadParser.mm

+5-13
Original file line numberDiff line numberDiff line change
@@ -50,19 +50,11 @@ - (MTRSetupPayload *)populatePayload:(NSError * __autoreleasing *)error
5050
chip::SetupPayload cPlusPluspayload;
5151
MTRSetupPayload * payload;
5252

53-
if (_chipQRCodeSetupPayloadParser) {
54-
CHIP_ERROR chipError = _chipQRCodeSetupPayloadParser->populatePayload(cPlusPluspayload);
55-
56-
if (chipError == CHIP_NO_ERROR) {
57-
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
58-
} else if (error) {
59-
*error = [MTRError errorForCHIPErrorCode:chipError];
60-
}
61-
} else {
62-
// Memory init has failed
63-
if (error) {
64-
*error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY];
65-
}
53+
CHIP_ERROR chipError = _chipQRCodeSetupPayloadParser->populatePayload(cPlusPluspayload);
54+
if (chipError == CHIP_NO_ERROR) {
55+
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
56+
} else if (error) {
57+
*error = [MTRError errorForCHIPErrorCode:chipError];
6658
}
6759

6860
return payload;

0 commit comments

Comments
 (0)