diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index e939c253fe6cd2..f6052669d91853 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 3837f6390344c8..54ed9c8dd4bea0 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 ca0e2362071821..42f5d246ecb6fc 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;