Fix error handling in Darwin DNS-SD implementation to clean up properly. (#35725)
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.
diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp
index ff86991..9d293c0 100644
--- a/src/platform/Darwin/DnssdContexts.cpp
+++ b/src/platform/Darwin/DnssdContexts.cpp
@@ -135,7 +135,8 @@
}
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 @@
}
}
-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)
+ VerifyOrReturnError(context != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+ if (!context->serviceRef)
{
- DNSServiceRefDeallocate(sdRef);
+ Delete(context);
return CHIP_ERROR_INVALID_ARGUMENT;
}
- if (sdRef == nullptr)
- {
- chip::Platform::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 @@
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)
diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp
index 3740600..5cd3a2d 100644
--- a/src/platform/Darwin/DnssdImpl.cpp
+++ b/src/platform/Darwin/DnssdImpl.cpp
@@ -188,12 +188,17 @@
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 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 @@
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 @@
sdCtx->shouldStartSRPTimerForResolve = true;
}
- auto retval = MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef);
+ auto retval = MdnsContexts::GetInstance().Add(sdCtx);
if (retval == CHIP_NO_ERROR)
{
(*(sdCtx->consumerCounter))++;
diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h
index 2304407..d4cd05f 100644
--- a/src/platform/Darwin/DnssdImpl.h
+++ b/src/platform/Darwin/DnssdImpl.h
@@ -43,7 +43,13 @@
{
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 @@
~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);
