Fix error handling in Darwin DNS-SD implementation to clean up properly.

We had several codepaths where we could create a service ref but then do some
fallible operations before calling Add() on the context.  If those failed, they
would call Finalize() on the context, which would delete the context, but _not_
DNSServiceRefDeallocate() the service ref.

The fix is to make sure that:

1) All cases where we create a service ref successfully immediately set it on
   the context.
2) All context deletion goes through MdnsContexts::Delete, so that we make sure
   to clean up the service ref if there is one.

Author: bzbarsky-apple

src/platform/Darwin/DnssdContexts.cpp

@@ -135,7 +135,8 @@ CHIP_ERROR GenericContext::FinalizeInternal(const char * errorStr, CHIP_ERROR er
     }
     else
     {
-        chip::Platform::Delete(this);
+        // Ensure that we clean up our service ref, if any, correctly.
+        MdnsContexts::GetInstance().Delete(this);
     }
 
     return err;
@@ -161,33 +162,22 @@ MdnsContexts::~MdnsContexts()
     }
 }
 
-CHIP_ERROR MdnsContexts::Add(GenericContext * context, DNSServiceRef sdRef)
+CHIP_ERROR MdnsContexts::Add(GenericContext * context)
 {
-    VerifyOrReturnError(context != nullptr || sdRef != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-
-    if (context == nullptr)
-    {
-        DNSServiceRefDeallocate(sdRef);
-        return CHIP_ERROR_INVALID_ARGUMENT;
-    }
-
-    if (sdRef == nullptr)
+    VerifyOrReturnError(context != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    if (!context->serviceRef)
     {
-        chip::Platform::Delete(context);
+        Delete(context);
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
-    auto err = DNSServiceSetDispatchQueue(sdRef, chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue());
+    auto err = DNSServiceSetDispatchQueue(context->serviceRef, 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);
+        Delete(context);
         return Error::ToChipError(err);
     }
 
-    context->serviceRef = sdRef;
     mContexts.push_back(context);
 
     return CHIP_NO_ERROR;
@@ -242,6 +232,8 @@ CHIP_ERROR MdnsContexts::RemoveAllOfType(ContextType type)
     return found ? CHIP_NO_ERROR : CHIP_ERROR_KEY_NOT_FOUND;
 }
 
+// TODO: Perhaps this cleanup code should just move into ~GenericContext, and
+// the places that call this method should just Platform::Delete() the context?
 void MdnsContexts::Delete(GenericContext * context)
 {
     if (context->serviceRef != nullptr)

src/platform/Darwin/DnssdImpl.cpp

@@ -188,12 +188,17 @@ CHIP_ERROR Register(void * context, DnssdPublishCallback callback, uint32_t inte
     auto err = sdCtx->mHostNameRegistrar.Init(hostname, addressType, interfaceId);
     VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err));
 
-    DNSServiceRef sdRef;
-    err = DNSServiceRegister(&sdRef, registerFlags, interfaceId, name, type, kLocalDot, hostname, htons(port), record.size(),
-                             record.data(), OnRegister, sdCtx);
-    VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err));
+    err = DNSServiceRegister(&sdCtx->serviceRef, registerFlags, interfaceId, name, type, kLocalDot, hostname, htons(port),
+                             record.size(), record.data(), OnRegister, sdCtx);
+    if (err != kDNSServiceErr_NoError)
+    {
+        // Just in case DNSServiceCreateConnection put garbage in the outparam
+        // on failure.
+        sdCtx->serviceRef = nullptr;
+        return sdCtx->Finalize(err);
+    }
 
-    return MdnsContexts::GetInstance().Add(sdCtx, sdRef);
+    return MdnsContexts::GetInstance().Add(sdCtx);
 }
 
 static void OnBrowse(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t interfaceId, DNSServiceErrorType err, const char * name,
@@ -216,16 +221,23 @@ CHIP_ERROR BrowseOnDomain(BrowseHandler * sdCtx, uint32_t interfaceId, const cha
 CHIP_ERROR Browse(BrowseHandler * sdCtx, uint32_t interfaceId, const char * type)
 {
     auto err = DNSServiceCreateConnection(&sdCtx->serviceRef);
-    VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err));
+    if (err != kDNSServiceErr_NoError)
+    {
+        // Just in case DNSServiceCreateConnection put garbage in the outparam
+        // on failure.
+        sdCtx->serviceRef = nullptr;
+        return sdCtx->Finalize(err);
+    }
 
     // We will browse on both the local domain and the SRP domain.
+    // BrowseOnDomain guarantees it will Finalize() on failure.
     ChipLogProgress(Discovery, "Browsing for: %s on local domain", StringOrNullMarker(type));
     ReturnErrorOnFailure(BrowseOnDomain(sdCtx, interfaceId, type, kLocalDot));
 
     ChipLogProgress(Discovery, "Browsing for: %s on %s domain", StringOrNullMarker(type), kSRPDot);
     ReturnErrorOnFailure(BrowseOnDomain(sdCtx, interfaceId, type, kSRPDot));
 
-    return MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef);
+    return MdnsContexts::GetInstance().Add(sdCtx);
 }
 
 CHIP_ERROR Browse(void * context, DnssdBrowseCallback callback, uint32_t interfaceId, const char * type,
@@ -380,10 +392,17 @@ static CHIP_ERROR Resolve(ResolveContext * sdCtx, uint32_t interfaceId, chip::In
                     StringOrNullMarker(name), StringOrNullMarker(domain), interfaceId);
 
     auto err = DNSServiceCreateConnection(&sdCtx->serviceRef);
-    VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err));
+    if (err != kDNSServiceErr_NoError)
+    {
+        // Just in case DNSServiceCreateConnection put garbage in the outparam
+        // on failure.
+        sdCtx->serviceRef = nullptr;
+        return sdCtx->Finalize(err);
+    }
 
     // If we have a single domain from a browse, we will use that for the Resolve.
     // Otherwise we will try to resolve using both the local domain and the SRP domain.
+    // ResolveWithContext guarantees it will Finalize() on failure.
     if (domain != nullptr)
     {
         ReturnErrorOnFailure(ResolveWithContext(sdCtx, interfaceId, type, name, domain, &sdCtx->resolveContextWithNonSRPType));
@@ -400,7 +419,7 @@ static CHIP_ERROR Resolve(ResolveContext * sdCtx, uint32_t interfaceId, chip::In
         sdCtx->shouldStartSRPTimerForResolve = true;
     }
 
-    auto retval = MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef);
+    auto retval = MdnsContexts::GetInstance().Add(sdCtx);
     if (retval == CHIP_NO_ERROR)
     {
         (*(sdCtx->consumerCounter))++;

src/platform/Darwin/DnssdImpl.h

@@ -43,7 +43,13 @@ struct GenericContext
 {
     ContextType type;
     void * context;
-    DNSServiceRef serviceRef;
+    // When using a GenericContext, if a DNSServiceRef is created successfully
+    // API consumers must ensure that it gets set as serviceRef on the context
+    // immediately, before any other operations that might fail can happen.
+    //
+    // In all cases, once a context has been created, Finalize() must be called
+    // on it to clean it up properly.
+    DNSServiceRef serviceRef = nullptr;
 
     virtual ~GenericContext() {}
 
@@ -69,7 +75,8 @@ class MdnsContexts
     ~MdnsContexts();
     static MdnsContexts & GetInstance() { return sInstance.get(); }
 
-    CHIP_ERROR Add(GenericContext * context, DNSServiceRef sdRef);
+    // The context being added is expected to have a valid serviceRef.
+    CHIP_ERROR Add(GenericContext * context);
     CHIP_ERROR Remove(GenericContext * context);
     CHIP_ERROR RemoveAllOfType(ContextType type);
     CHIP_ERROR Has(GenericContext * context);