From 1069988d62b823c870b77148ef600b2aedeba2e5 Mon Sep 17 00:00:00 2001 From: Vivien Nicolas Date: Fri, 14 Apr 2023 21:33:49 +0200 Subject: [PATCH] [darwin-framework-tool] heap-use-after-free when calling chip::app::DnssdServer::StartServer at startup (#26000) --- src/app/server/Dnssd.cpp | 24 +++++++++++++++++++ src/app/server/Dnssd.h | 3 +++ .../CHIPDeviceControllerFactory.cpp | 15 ++++++++++++ src/controller/CHIPDeviceControllerFactory.h | 4 ++-- src/lib/dnssd/Advertiser.h | 7 ++++++ src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 17 ++++++++++++- src/lib/dnssd/Advertiser_ImplNone.cpp | 2 ++ src/lib/dnssd/Discovery_ImplPlatform.cpp | 5 ++-- src/lib/dnssd/Discovery_ImplPlatform.h | 2 +- 9 files changed, 73 insertions(+), 6 deletions(-) diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index fc1faeb25b328a..7c58541766aff0 100644 --- a/src/app/server/Dnssd.cpp +++ b/src/app/server/Dnssd.cpp @@ -343,6 +343,30 @@ void DnssdServer::StartServer() return StartServer(mode); } +void DnssdServer::StopServer() +{ + DeviceLayer::PlatformMgr().RemoveEventHandler(OnPlatformEventWrapper, 0); + +#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY + if (mExtendedDiscoveryExpiration != kTimeoutCleared) + { + DeviceLayer::SystemLayer().CancelTimer(HandleExtendedDiscoveryExpiration, nullptr); + mExtendedDiscoveryExpiration = kTimeoutCleared; + } +#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY + + if (Dnssd::ServiceAdvertiser::Instance().IsInitialized()) + { + auto err = Dnssd::ServiceAdvertiser::Instance().RemoveServices(); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Discovery, "Failed to remove advertised services: %" CHIP_ERROR_FORMAT, err.Format()); + } + + Dnssd::ServiceAdvertiser::Instance().Shutdown(); + } +} + void DnssdServer::StartServer(Dnssd::CommissioningMode mode) { ChipLogProgress(Discovery, "Updating services using commissioning mode %d", static_cast(mode)); diff --git a/src/app/server/Dnssd.h b/src/app/server/Dnssd.h index 883f90eebf6747..174b5dbd91d9b5 100644 --- a/src/app/server/Dnssd.h +++ b/src/app/server/Dnssd.h @@ -94,6 +94,9 @@ class DLL_EXPORT DnssdServer /// (Re-)starts the Dnssd server, using the provided commissioning mode. void StartServer(Dnssd::CommissioningMode mode); + //// Stop the Dnssd server. + void StopServer(); + CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize); /// Generates the (random) instance name that a CHIP device is to use for pre-commissioning DNS-SD diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 4cc20bfc3f10a7..19af89c60828d9 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -340,6 +340,21 @@ CHIP_ERROR DeviceControllerFactory::ServiceEvents() return CHIP_NO_ERROR; } +void DeviceControllerFactory::RetainSystemState() +{ + (void) mSystemState->Retain(); +} + +void DeviceControllerFactory::ReleaseSystemState() +{ + mSystemState->Release(); + + if (!mSystemState->IsInitialized() && mEnableServerInteractions) + { + app::DnssdServer::Instance().StopServer(); + } +} + DeviceControllerFactory::~DeviceControllerFactory() { Shutdown(); diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index 1fa0563b6bd8e1..a1c41e9fcfa370 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -174,7 +174,7 @@ class DeviceControllerFactory // created to permit retention of the underlying system state. // // NB: The system state will still be freed in Shutdown() regardless of this call. - void RetainSystemState() { (void) mSystemState->Retain(); } + void RetainSystemState(); // // To initiate shutdown of the stack upon termination of all resident controllers in the @@ -183,7 +183,7 @@ class DeviceControllerFactory // // This should only be invoked if a matching call to RetainSystemState() was called prior. // - void ReleaseSystemState() { mSystemState->Release(); } + void ReleaseSystemState(); // // Retrieve a read-only pointer to the system state object that contains pointers to key stack diff --git a/src/lib/dnssd/Advertiser.h b/src/lib/dnssd/Advertiser.h index 45cd072c1db11c..322ee3c871d60e 100644 --- a/src/lib/dnssd/Advertiser.h +++ b/src/lib/dnssd/Advertiser.h @@ -299,6 +299,13 @@ class ServiceAdvertiser */ virtual CHIP_ERROR Init(chip::Inet::EndPointManager * udpEndPointManager) = 0; + /** + * Returns whether the advertiser has completed the initialization. + * + * Returns true if the advertiser is ready to advertise services. + */ + virtual bool IsInitialized() = 0; + /** * Shuts down the advertiser. */ diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp index 679c8bae7d3e48..2c9c021906cecf 100644 --- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp @@ -181,11 +181,12 @@ class AdvertiserMinMdns : public ServiceAdvertiser, // Service advertiser CHIP_ERROR Init(chip::Inet::EndPointManager * udpEndPointManager) override; + bool IsInitialized() override { return mIsInitialized; } void Shutdown() override; CHIP_ERROR RemoveServices() override; CHIP_ERROR Advertise(const OperationalAdvertisingParameters & params) override; CHIP_ERROR Advertise(const CommissionAdvertisingParameters & params) override; - CHIP_ERROR FinalizeServiceUpdate() override { return CHIP_NO_ERROR; } + CHIP_ERROR FinalizeServiceUpdate() override; CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) const override; CHIP_ERROR UpdateCommissionableInstanceName() override; @@ -363,6 +364,8 @@ CHIP_ERROR AdvertiserMinMdns::Init(chip::Inet::EndPointManager *) override { return InitImpl(); } + bool IsInitialized() override; void Shutdown() override; // Members that implement ServiceAdvertiser interface. @@ -49,7 +50,6 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver CHIP_ERROR UpdateCommissionableInstanceName() override; // Members that implement Resolver interface. - bool IsInitialized() override; void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mResolverProxy.SetOperationalDelegate(delegate); } void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override {