Make sure we stop resolves triggered by a browse when the browse stop…

…s 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 #24074

* Also avoid leaking ResolveContext instances.

* Fix handling of multiple interfaces.

* Address review comment.

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<uint32_t> && consumerCounterToUse)
+                               const char * instanceNameToResolve, BrowseContext * browseCausingResolve,
+                               std::shared_ptr<uint32_t> && consumerCounterToUse) :
+    browseThatCausedResolve(browseCausingResolve)
 {
     type            = ContextType::Resolve;
     context         = cbContext;

src/platform/Darwin/DnssdImpl.cpp

@@ -360,7 +360,8 @@ static CHIP_ERROR Resolve(void * context, DnssdResolveCallback callback, uint32_
         counterHolder = std::make_shared<uint32_t>(0);
     }
 
-    auto sdCtx = chip::Platform::New<ResolveContext>(context, callback, addressType, name, std::move(counterHolder));
+    auto sdCtx = chip::Platform::New<ResolveContext>(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<BrowseContext *>(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<GenericContext *> resolves;
+    MdnsContexts::GetInstance().FindAllMatchingPredicate(
+        [ctx](GenericContext * item) {
+            return item->type == ContextType::Resolve && static_cast<ResolveContext *>(item)->browseThatCausedResolve == ctx;
+        },
+        resolves);
+
+    for (auto & resolve : resolves)
+    {
+        resolve->Finalize(kDNSServiceErr_Timeout);
+    }
+
+    ctx->Finalize(kDNSServiceErr_Timeout);
     return CHIP_NO_ERROR;
 }
 

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 <typename F>
+    void FindAllMatchingPredicate(F predicate, std::vector<GenericContext *> & 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<uint32_t> 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<uint32_t> && consumerCounterToUse);
+                   const char * instanceNameToResolve, BrowseContext * browseCausingResolve,
+                   std::shared_ptr<uint32_t> && consumerCounterToUse);
     virtual ~ResolveContext();
 
     void DispatchFailure(DNSServiceErrorType err) override;