From 9a71775e31329175294dd5dbad1cf54aa4d4867e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 30 Jan 2023 09:51:25 -0500 Subject: [PATCH] Make sure we stop resolves triggered by a browse when the browse stops on Darwin. Without this change, if there is a PTR record that matches whatever we are browsing but no corresponding SRV record, we would end up leaking a resolve forever. Tested by modifying minimal mdns SrvResponder::AddAllResponses to no-op instead of actually adding any responses, then trying to commission the device running the modified minimal mdns. Without this change, when the browse stops the resolves it triggered keep going. With this change, termination of the browse also terminates the resolves. Fixes https://github.com/project-chip/connectedhomeip/issues/24074 --- src/platform/Darwin/DnssdContexts.cpp | 31 +++++++++++++++++++++------ src/platform/Darwin/DnssdImpl.cpp | 26 ++++++++++++++++------ src/platform/Darwin/DnssdImpl.h | 25 +++++++++++++++++++++ 3 files changed, 68 insertions(+), 14 deletions(-) diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index b433dc47a418c7..f80f0c6de5ffbd 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -153,11 +153,17 @@ CHIP_ERROR MdnsContexts::Add(GenericContext * context, DNSServiceRef sdRef) return CHIP_ERROR_INVALID_ARGUMENT; } - auto err = DNSServiceSetDispatchQueue(sdRef, chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue()); - if (kDNSServiceErr_NoError != err) + if (context->OwnsServiceRef()) { - chip::Platform::Delete(context); - return Error::ToChipError(err); + auto err = DNSServiceSetDispatchQueue(sdRef, chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue()); + if (kDNSServiceErr_NoError != err) + { + // We can't just use our Delete to deallocate the service ref here, + // because our context may not have its serviceRef set yet. + DNSServiceRefDeallocate(sdRef); + chip::Platform::Delete(context); + return Error::ToChipError(err); + } } context->serviceRef = sdRef; @@ -217,7 +223,7 @@ CHIP_ERROR MdnsContexts::RemoveAllOfType(ContextType type) void MdnsContexts::Delete(GenericContext * context) { - if (context->serviceRef != nullptr) + if (context->serviceRef != nullptr && context->OwnsServiceRef()) { DNSServiceRefDeallocate(context->serviceRef); } @@ -297,6 +303,8 @@ void RegisterContext::DispatchSuccess() mHostNameRegistrar.Register(); } +BrowseContext * BrowseContext::sContextDispatchingSuccess = nullptr; + BrowseContext::BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServiceProtocol cbContextProtocol) { type = ContextType::Browse; @@ -314,13 +322,16 @@ void BrowseContext::DispatchFailure(DNSServiceErrorType err) void BrowseContext::DispatchSuccess() { - callback(context, services.data(), services.size(), true, CHIP_NO_ERROR); - MdnsContexts::GetInstance().Remove(this); + // This should never be called: We either DispatchPartialSuccess or + // DispatchFailure. + VerifyOrDie(false); } void BrowseContext::DispatchPartialSuccess() { + sContextDispatchingSuccess = this; callback(context, services.data(), services.size(), false, CHIP_NO_ERROR); + sContextDispatchingSuccess = nullptr; services.clear(); } @@ -491,6 +502,12 @@ bool ResolveContext::HasInterface() return interfaces.size(); } +void ResolveContext::ShareExistingConnection(DNSServiceRef existingConnection) +{ + serviceRef = existingConnection; + serviceRefIsOwned = false; +} + InterfaceInfo::InterfaceInfo() { service.mTextEntrySize = 0; diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index 3837f6390344c8..7aa61acd2650d3 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -37,7 +37,7 @@ constexpr const char * kProtocolTcp = "._tcp"; constexpr const char * kProtocolUdp = "._udp"; constexpr DNSServiceFlags kRegisterFlags = kDNSServiceFlagsNoAutoRename; -constexpr DNSServiceFlags kBrowseFlags = 0; +constexpr DNSServiceFlags kBrowseFlags = kDNSServiceFlagsShareConnection; constexpr DNSServiceFlags kGetAddrInfoFlags = kDNSServiceFlagsTimeout | kDNSServiceFlagsShareConnection; constexpr DNSServiceFlags kResolveFlags = kDNSServiceFlagsShareConnection; constexpr DNSServiceFlags kReconfirmRecordFlags = 0; @@ -270,11 +270,15 @@ CHIP_ERROR Browse(void * context, DnssdBrowseCallback callback, uint32_t interfa VerifyOrReturnError(nullptr != sdCtx, CHIP_ERROR_NO_MEMORY); ChipLogProgress(Discovery, "Browsing for: %s", StringOrNullMarker(type)); - DNSServiceRef sdRef; - auto err = DNSServiceBrowse(&sdRef, kBrowseFlags, interfaceId, type, kLocalDot, OnBrowse, sdCtx); + + auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - ReturnErrorOnFailure(MdnsContexts::GetInstance().Add(sdCtx, sdRef)); + auto sdRefCopy = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection + err = DNSServiceBrowse(&sdRefCopy, kBrowseFlags, interfaceId, type, kLocalDot, OnBrowse, sdCtx); + VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); + + ReturnErrorOnFailure(MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef)); *browseIdentifier = reinterpret_cast(sdCtx); return CHIP_NO_ERROR; } @@ -363,11 +367,19 @@ static CHIP_ERROR Resolve(void * context, DnssdResolveCallback callback, uint32_ auto sdCtx = chip::Platform::New(context, callback, addressType, name, std::move(counterHolder)); VerifyOrReturnError(nullptr != sdCtx, CHIP_ERROR_NO_MEMORY); - auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); - VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); + if (BrowseContext::sContextDispatchingSuccess == nullptr) + { + auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); + VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); + } + else + { + // Share the connection of the browse. + sdCtx->ShareExistingConnection(BrowseContext::sContextDispatchingSuccess->serviceRef); + } auto sdRefCopy = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection - err = DNSServiceResolve(&sdRefCopy, kResolveFlags, interfaceId, name, type, kLocalDot, OnResolve, sdCtx); + auto err = DNSServiceResolve(&sdRefCopy, kResolveFlags, interfaceId, name, type, kLocalDot, OnResolve, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); CHIP_ERROR retval = MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef); diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index ca0e2362071821..39df70e2b6a96c 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -42,11 +42,20 @@ struct GenericContext void * context; DNSServiceRef serviceRef; +protected: + // Whether the serviceRef is owned by this context. Some contexts share a + // service ref with other contexts, in which case serviceRefIsOwned will be + // false. + bool serviceRefIsOwned = true; + +public: virtual ~GenericContext() {} CHIP_ERROR Finalize(DNSServiceErrorType err = kDNSServiceErr_NoError); virtual void DispatchFailure(DNSServiceErrorType err) = 0; virtual void DispatchSuccess() = 0; + + bool OwnsServiceRef() const { return serviceRefIsOwned; } }; struct RegisterContext; @@ -133,6 +142,20 @@ struct BrowseContext : public GenericContext // Dispatch what we have found so far, but don't stop browsing. void DispatchPartialSuccess(); + + // While we are dispatching partial success, sContextDispatchingSuccess will + // be set to the BrowseContext doing the dispatch. This allows resolves + // triggered by the browse dispatch to be associated with the browse. This + // relies on our consumer starting the resolves synchronously from the + // partial success callback. + // + // The other option would be to do the resolve ourselves before signaling + // browse success, but that would only allow us to pass in one ip per + // discovered hostname, and we want to pass in all the IPs we resolve. + // + // TODO: Consider fixing the higher-level APIs to make it possible to pass + // in multiple IPs for a successful browse result. + static BrowseContext * sContextDispatchingSuccess; }; struct InterfaceInfo @@ -172,6 +195,8 @@ struct ResolveContext : public GenericContext const unsigned char * txtRecord); bool HasInterface(); bool Matches(const char * otherInstanceName) const { return instanceName == otherInstanceName; } + + void ShareExistingConnection(DNSServiceRef existingConnection); }; } // namespace Dnssd