From 15779634a5f38e62989f4359a3d668c6ac803919 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 5 Apr 2023 12:59:37 -0400 Subject: [PATCH] Make Darwin HostNameRegistrar safer to use. (#25983) There are two main changes here: 1) When we shut down the Matter stack, we should remove all our advertisements adn stop monitoring for network interface changes. That's what the change to ChipDnssdShutdown does. 2) It's theoretically possible for the block HostNameRegistrar passes to nw_path_monitor_set_update_handler to run after the HostNameRegistrar is destroyed. Guard against that with a shared_ptr that keeps track of the liveness state of the HostNameRegistrar. The changes to the member initialization of HostNameRegistrar are just for safety's sake, in case its destructor ever gets called without Init() having been called first. Right now that is not possible, but better to not rely on that. --- .../Darwin/DnssdHostNameRegistrar.cpp | 30 ++++++++++++++++++- src/platform/Darwin/DnssdHostNameRegistrar.h | 19 +++++++++--- src/platform/Darwin/DnssdImpl.cpp | 16 +++++++--- src/platform/Darwin/MdnsError.cpp | 2 ++ 4 files changed, 58 insertions(+), 9 deletions(-) diff --git a/src/platform/Darwin/DnssdHostNameRegistrar.cpp b/src/platform/Darwin/DnssdHostNameRegistrar.cpp index fd360f66237c74..572aa043043e4d 100644 --- a/src/platform/Darwin/DnssdHostNameRegistrar.cpp +++ b/src/platform/Darwin/DnssdHostNameRegistrar.cpp @@ -256,13 +256,30 @@ void GetInterfaceAddresses(uint32_t interfaceId, chip::Inet::IPAddressType addre } } // namespace -void HostNameRegistrar::Init(const char * hostname, Inet::IPAddressType addressType, uint32_t interfaceId) +HostNameRegistrar::~HostNameRegistrar() +{ + Unregister(); + if (mLivenessTracker != nullptr) + { + *mLivenessTracker = false; + } +} + +DNSServiceErrorType HostNameRegistrar::Init(const char * hostname, Inet::IPAddressType addressType, uint32_t interfaceId) { mHostname = hostname; mInterfaceId = interfaceId; mAddressType = addressType; mServiceRef = nullptr; mInterfaceMonitor = nullptr; + + mLivenessTracker = std::make_shared(true); + if (mLivenessTracker == nullptr) + { + return kDNSServiceErr_NoMemory; + } + + return kDNSServiceErr_NoError; } CHIP_ERROR HostNameRegistrar::Register() @@ -303,7 +320,18 @@ CHIP_ERROR HostNameRegistrar::StartMonitorInterfaces(OnInterfaceChanges interfac nw_path_monitor_set_queue(mInterfaceMonitor, chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue()); + // Our update handler closes over "this", but can't keep us alive (because we + // are not refcounted). Make sure it closes over a shared ref to our + // liveness tracker, which it _can_ keep alive, so it can bail out if we + // have been destroyed between when the task was queued and when it ran. + std::shared_ptr livenessTracker = mLivenessTracker; nw_path_monitor_set_update_handler(mInterfaceMonitor, ^(nw_path_t path) { + if (!*livenessTracker) + { + // The HostNameRegistrar has been destroyed; just bail out. + return; + } + #if CHIP_PROGRESS_LOGGING LogDetails(path); #endif // CHIP_PROGRESS_LOGGING diff --git a/src/platform/Darwin/DnssdHostNameRegistrar.h b/src/platform/Darwin/DnssdHostNameRegistrar.h index c3f44884767b14..9bb5ce463cfdf6 100644 --- a/src/platform/Darwin/DnssdHostNameRegistrar.h +++ b/src/platform/Darwin/DnssdHostNameRegistrar.h @@ -36,7 +36,9 @@ namespace Dnssd { class HostNameRegistrar { public: - void Init(const char * hostname, Inet::IPAddressType addressType, uint32_t interfaceId); + ~HostNameRegistrar(); + + DNSServiceErrorType Init(const char * hostname, Inet::IPAddressType addressType, uint32_t interfaceId); CHIP_ERROR Register(); void Unregister(); @@ -61,15 +63,24 @@ namespace Dnssd { CHIP_ERROR StartSharedConnection(); void StopSharedConnection(); CHIP_ERROR ResetSharedConnection(); - DNSServiceRef mServiceRef; CHIP_ERROR StartMonitorInterfaces(OnInterfaceChanges interfaceChangesBlock); void StopMonitorInterfaces(); - nw_path_monitor_t mInterfaceMonitor; + + DNSServiceRef mServiceRef = nullptr; + nw_path_monitor_t mInterfaceMonitor = nullptr; std::string mHostname; - uint32_t mInterfaceId; + // Default to kDNSServiceInterfaceIndexLocalOnly so we don't mess around + // with un-registration if we never get Init() called. + uint32_t mInterfaceId = kDNSServiceInterfaceIndexLocalOnly; Inet::IPAddressType mAddressType; + + // We use mLivenessTracker to indicate to blocks that close over us that + // we've been destroyed. This is needed because we're not a refcounted + // object, so the blocks can't keep us alive; they just close over the + // raw pointer to "this". + std::shared_ptr mLivenessTracker; }; } // namespace Dnssd diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index 584b9b553d655c..d859f2b8d1391d 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -195,11 +195,12 @@ CHIP_ERROR Register(void * context, DnssdPublishCallback callback, uint32_t inte sdCtx = chip::Platform::New(type, name, callback, context); VerifyOrReturnError(nullptr != sdCtx, CHIP_ERROR_NO_MEMORY); - sdCtx->mHostNameRegistrar.Init(hostname, addressType, interfaceId); + auto err = sdCtx->mHostNameRegistrar.Init(hostname, addressType, interfaceId); + VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); DNSServiceRef sdRef; - auto err = DNSServiceRegister(&sdRef, kRegisterFlags, interfaceId, name, type, kLocalDot, hostname, htons(port), record.size(), - record.data(), OnRegister, sdCtx); + err = DNSServiceRegister(&sdRef, kRegisterFlags, interfaceId, name, type, kLocalDot, hostname, htons(port), record.size(), + record.data(), OnRegister, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); return MdnsContexts::GetInstance().Add(sdCtx, sdRef); @@ -394,7 +395,12 @@ CHIP_ERROR ChipDnssdInit(DnssdAsyncReturnCallback successCallback, DnssdAsyncRet return CHIP_NO_ERROR; } -void ChipDnssdShutdown() {} +void ChipDnssdShutdown() +{ + // Drop our existing advertisements now, so they don't stick around while we + // are not actually in a responsive state. + ChipDnssdRemoveServices(); +} CHIP_ERROR ChipDnssdPublishService(const DnssdService * service, DnssdPublishCallback callback, void * context) { @@ -416,6 +422,8 @@ CHIP_ERROR ChipDnssdPublishService(const DnssdService * service, DnssdPublishCal CHIP_ERROR ChipDnssdRemoveServices() { + assertChipStackLockedByCurrentThread(); + auto err = MdnsContexts::GetInstance().RemoveAllOfType(ContextType::Register); if (CHIP_ERROR_KEY_NOT_FOUND == err) { diff --git a/src/platform/Darwin/MdnsError.cpp b/src/platform/Darwin/MdnsError.cpp index 9ad52cb9840fee..0664887ea9bbe0 100644 --- a/src/platform/Darwin/MdnsError.cpp +++ b/src/platform/Darwin/MdnsError.cpp @@ -102,6 +102,8 @@ CHIP_ERROR ToChipError(DNSServiceErrorType errorCode) return CHIP_NO_ERROR; case kDNSServiceErr_NameConflict: return CHIP_ERROR_MDNS_COLLISION; + case kDNSServiceErr_NoMemory: + return CHIP_ERROR_NO_MEMORY; default: return CHIP_ERROR_INTERNAL; }