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

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

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;

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<intptr_t>(sdCtx);
     return CHIP_NO_ERROR;
 }
@@ -363,11 +367,19 @@ static CHIP_ERROR Resolve(void * context, DnssdResolveCallback callback, uint32_
     auto sdCtx = chip::Platform::New<ResolveContext>(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);

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