From 56bb5b2ca120fc2c328aec6cb350bf748f2984fc Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 19 Oct 2020 16:12:41 -0700 Subject: [PATCH 1/7] apple dns: fixups Signed-off-by: Jose Nino --- source/common/network/apple_dns_impl.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 05e430076056..0b98f19e6e6d 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -85,7 +85,7 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, DNSServiceErrorType error = pending_resolution->dnsServiceGetAddrInfo(dns_lookup_family); if (error != kDNSServiceErr_NoError) { - ENVOY_LOG(warn, "DNS resolver error in dnsServiceGetAddrInfo for {}", dns_name); + ENVOY_LOG(warn, "DNS resolver error ({}) in dnsServiceGetAddrInfo for {}", error, dns_name); return nullptr; } @@ -150,7 +150,10 @@ void AppleDnsResolverImpl::flushPendingQueries(const bool with_error) { AppleDnsResolverImpl::PendingResolution::~PendingResolution() { ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_); - DNSServiceRefDeallocate(individual_sd_ref_); + // FIXME: add comment about why the ref might be null. + if (individual_sd_ref_) { + DNSServiceRefDeallocate(individual_sd_ref_); + } } void AppleDnsResolverImpl::PendingResolution::cancel() { From 4795fc36094c6e8d0740adc229a49e3098701a95 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 19 Oct 2020 16:35:35 -0700 Subject: [PATCH 2/7] error handling to event callback Signed-off-by: Jose Nino --- source/common/network/apple_dns_impl.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 0b98f19e6e6d..7350ecea8039 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -71,9 +71,13 @@ void AppleDnsResolverImpl::initializeMainSdRef() { } void AppleDnsResolverImpl::onEventCallback(uint32_t events) { - ENVOY_LOG(debug, "DNS resolver file event"); + ENVOY_LOG(debug, "DNS resolver file event ()", events); ASSERT(events & Event::FileReadyType::Read); - DNSServiceProcessResult(main_sd_ref_); + DNSServiceErrorType error = DNSServiceProcessResult(main_sd_ref_); + if (error != kDNSServiceErr_NoError) { + ENVOY_LOG(warn, "DNS resolver error ({}) in DNSServiceProcessResult", error); + flushPendingQueries(true /* with_error */); + } } ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, From d303b1830caa416a2b676cb57c3a5c246c727ad0 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 19 Oct 2020 16:49:14 -0700 Subject: [PATCH 3/7] print events Signed-off-by: Jose Nino --- source/common/network/apple_dns_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 7350ecea8039..b35cecafa43b 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -71,7 +71,7 @@ void AppleDnsResolverImpl::initializeMainSdRef() { } void AppleDnsResolverImpl::onEventCallback(uint32_t events) { - ENVOY_LOG(debug, "DNS resolver file event ()", events); + ENVOY_LOG(debug, "DNS resolver file event ({})", events); ASSERT(events & Event::FileReadyType::Read); DNSServiceErrorType error = DNSServiceProcessResult(main_sd_ref_); if (error != kDNSServiceErr_NoError) { From 47d392f670305323882bd39995989a4952b24501 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 19 Oct 2020 17:27:23 -0700 Subject: [PATCH 4/7] final comments Signed-off-by: Jose Nino --- source/common/network/apple_dns_impl.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index b35cecafa43b..a3e096caee46 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -76,6 +76,9 @@ void AppleDnsResolverImpl::onEventCallback(uint32_t events) { DNSServiceErrorType error = DNSServiceProcessResult(main_sd_ref_); if (error != kDNSServiceErr_NoError) { ENVOY_LOG(warn, "DNS resolver error ({}) in DNSServiceProcessResult", error); + // Similar to receiving an error in onDNSServiceGetAddrInfoReply, an error while processing fd + // events indicates that the sd_ref state is broken. + // Therefore, flush queries with_error == true. flushPendingQueries(true /* with_error */); } } @@ -154,8 +157,10 @@ void AppleDnsResolverImpl::flushPendingQueries(const bool with_error) { AppleDnsResolverImpl::PendingResolution::~PendingResolution() { ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_); - // FIXME: add comment about why the ref might be null. + // It is possible that DNSServiceGetAddrInfo returns a synchronous error, with a NULLed + // DNSServiceRef in AppleDnsResolverImpl::resolve. if (individual_sd_ref_) { + RELEASE_ASSERT(!owned, "individual_sd_ref_ should only be null on a synchronous error."); DNSServiceRefDeallocate(individual_sd_ref_); } } From 41ad80a1a634022c0aa165b460f9da930cc4752b Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 19 Oct 2020 17:36:36 -0700 Subject: [PATCH 5/7] better comment Signed-off-by: Jose Nino --- source/common/network/apple_dns_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index a3e096caee46..7f009df8b70b 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -55,7 +55,7 @@ void AppleDnsResolverImpl::initializeMainSdRef() { // for kDNSServiceFlagsShareConnection in dns_sd.h, and copied (and edited) in this implementation // where relevant. auto error = DNSServiceCreateConnection(&main_sd_ref_); - RELEASE_ASSERT(!error, "error in DNSServiceCreateConnection"); + RELEASE_ASSERT(!error, fmt::format("error ({}) in DNSServiceCreateConnection", error)); auto fd = DNSServiceRefSockFD(main_sd_ref_); RELEASE_ASSERT(fd != -1, "error in DNSServiceRefSockFD"); @@ -158,7 +158,7 @@ void AppleDnsResolverImpl::flushPendingQueries(const bool with_error) { AppleDnsResolverImpl::PendingResolution::~PendingResolution() { ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_); // It is possible that DNSServiceGetAddrInfo returns a synchronous error, with a NULLed - // DNSServiceRef in AppleDnsResolverImpl::resolve. + // DNSServiceRef, in AppleDnsResolverImpl::resolve. if (individual_sd_ref_) { RELEASE_ASSERT(!owned, "individual_sd_ref_ should only be null on a synchronous error."); DNSServiceRefDeallocate(individual_sd_ref_); From db1bdf1ddf2908c5445e64a71503be5c7b1a6209 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 19 Oct 2020 17:53:25 -0700 Subject: [PATCH 6/7] fix Signed-off-by: Jose Nino --- source/common/network/apple_dns_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 7f009df8b70b..adbe9ef31227 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -160,7 +160,7 @@ AppleDnsResolverImpl::PendingResolution::~PendingResolution() { // It is possible that DNSServiceGetAddrInfo returns a synchronous error, with a NULLed // DNSServiceRef, in AppleDnsResolverImpl::resolve. if (individual_sd_ref_) { - RELEASE_ASSERT(!owned, "individual_sd_ref_ should only be null on a synchronous error."); + RELEASE_ASSERT(!owned_, "individual_sd_ref_ should only be null on a synchronous error."); DNSServiceRefDeallocate(individual_sd_ref_); } } From ba24e1f49459767e428e13542d5978a6d6f1df8d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 20 Oct 2020 10:10:42 -0700 Subject: [PATCH 7/7] forgot about cancellation Signed-off-by: Jose Nino --- source/common/network/apple_dns_impl.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index adbe9ef31227..543cf4def9c5 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -159,8 +159,10 @@ AppleDnsResolverImpl::PendingResolution::~PendingResolution() { ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_); // It is possible that DNSServiceGetAddrInfo returns a synchronous error, with a NULLed // DNSServiceRef, in AppleDnsResolverImpl::resolve. + // Additionally, it is also possible that the query is cancelled before resolution starts, and + // thus the DNSServiceRef is null. + // Therefore, only deallocate if the ref is not null. if (individual_sd_ref_) { - RELEASE_ASSERT(!owned_, "individual_sd_ref_ should only be null on a synchronous error."); DNSServiceRefDeallocate(individual_sd_ref_); } }