Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns: fix defunct fd bug in apple resolver #13641

Merged
merged 9 commits into from
Oct 20, 2020
Merged

dns: fix defunct fd bug in apple resolver #13641

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
56bb5b2
apple dns: fixups
Oct 19, 2020
4795fc3
error handling to event callback
Oct 19, 2020
d303b18
print events
Oct 19, 2020
47d392f
final comments
Oct 20, 2020
41ad80a
better comment
Oct 20, 2020
db1bdf1
fix
Oct 20, 2020
ba24e1f
forgot about cancellation
Oct 20, 2020
8c2b2bd
Merge branch 'master' into apple-dns-fixups
Oct 20, 2020
fa65f0d
Merge branch 'master' into apple-dns-fixups
Oct 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions source/common/network/apple_dns_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -71,9 +71,16 @@ 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_);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified that all other calls to the DNSService API that return DNSServiceErrorType are handled.

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 */);
}
}

ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name,
Expand All @@ -85,7 +92,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;
}

Expand Down Expand Up @@ -150,7 +157,12 @@ void AppleDnsResolverImpl::flushPendingQueries(const bool with_error) {

AppleDnsResolverImpl::PendingResolution::~PendingResolution() {
ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_);
DNSServiceRefDeallocate(individual_sd_ref_);
// 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_);
}
}

void AppleDnsResolverImpl::PendingResolution::cancel() {
Expand Down