From f65496a672397fe54e8885a47c214bfbdd1bc3c3 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 31 Jan 2023 12:41:25 -0500 Subject: [PATCH] Make sure we stop resolves triggered by a browse when the browse stops on Darwin. (#24733) * 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 * Also avoid leaking ResolveContext instances. * Fix handling of multiple interfaces. * Address review comment. --- src/platform/Darwin/DnssdContexts.cpp | 24 +++++++++++++++--- src/platform/Darwin/DnssdImpl.cpp | 22 +++++++++++++--- src/platform/Darwin/DnssdImpl.h | 36 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index e939c253fe..f6052669d9 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -145,7 +145,13 @@ MdnsContexts::~MdnsContexts() CHIP_ERROR MdnsContexts::Add(GenericContext * context, DNSServiceRef sdRef) { - VerifyOrReturnError(context != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(context != nullptr || sdRef != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + + if (context == nullptr) + { + DNSServiceRefDeallocate(sdRef); + return CHIP_ERROR_INVALID_ARGUMENT; + } if (sdRef == nullptr) { @@ -156,6 +162,9 @@ CHIP_ERROR MdnsContexts::Add(GenericContext * context, DNSServiceRef sdRef) 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); } @@ -297,6 +306,8 @@ void RegisterContext::DispatchSuccess() mHostNameRegistrar.Register(); } +BrowseContext * BrowseContext::sContextDispatchingSuccess = nullptr; + BrowseContext::BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServiceProtocol cbContextProtocol) { type = ContextType::Browse; @@ -314,18 +325,23 @@ 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(); } ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType, - const char * instanceNameToResolve, std::shared_ptr && consumerCounterToUse) + const char * instanceNameToResolve, BrowseContext * browseCausingResolve, + std::shared_ptr && consumerCounterToUse) : + browseThatCausedResolve(browseCausingResolve) { type = ContextType::Resolve; context = cbContext; diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index 3837f63903..54ed9c8dd4 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -360,7 +360,8 @@ static CHIP_ERROR Resolve(void * context, DnssdResolveCallback callback, uint32_ counterHolder = std::make_shared(0); } - auto sdCtx = chip::Platform::New(context, callback, addressType, name, std::move(counterHolder)); + auto sdCtx = chip::Platform::New(context, callback, addressType, name, + BrowseContext::sContextDispatchingSuccess, std::move(counterHolder)); VerifyOrReturnError(nullptr != sdCtx, CHIP_ERROR_NO_MEMORY); auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); @@ -455,8 +456,23 @@ CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier) // Just treat this as a timeout error. Don't bother delivering the partial // results we have queued up in the BrowseContext, if any. In practice // there shouldn't be anything there long-term anyway. - auto browseCtx = static_cast(ctx); - browseCtx->Finalize(kDNSServiceErr_Timeout); + // + // Make sure to time out all the resolves first, before we time out the + // browse (just to avoid dangling pointers in the resolves, even though we + // only use them for equality compares). + std::vector resolves; + MdnsContexts::GetInstance().FindAllMatchingPredicate( + [ctx](GenericContext * item) { + return item->type == ContextType::Resolve && static_cast(item)->browseThatCausedResolve == ctx; + }, + resolves); + + for (auto & resolve : resolves) + { + resolve->Finalize(kDNSServiceErr_Timeout); + } + + ctx->Finalize(kDNSServiceErr_Timeout); return CHIP_NO_ERROR; } diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index ca0e236207..42f5d246ec 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -96,6 +96,23 @@ class MdnsContexts void Delete(GenericContext * context); + /** + * Fill the provided vector with all contexts for which the given predicate + * returns true. + */ + template + void FindAllMatchingPredicate(F predicate, std::vector & results) + { + results.clear(); + for (auto & ctx : mContexts) + { + if (predicate(ctx)) + { + results.push_back(ctx); + } + } + } + private: MdnsContexts(){}; static MdnsContexts sInstance; @@ -133,6 +150,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 @@ -156,9 +187,12 @@ struct ResolveContext : public GenericContext DNSServiceProtocol protocol; std::string instanceName; std::shared_ptr consumerCounter; + BrowseContext * const browseThatCausedResolve; // Can be null + // browseCausingResolve can be null. ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType, - const char * instanceNameToResolve, std::shared_ptr && consumerCounterToUse); + const char * instanceNameToResolve, BrowseContext * browseCausingResolve, + std::shared_ptr && consumerCounterToUse); virtual ~ResolveContext(); void DispatchFailure(DNSServiceErrorType err) override;