diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index bb317ae397f0..f7ddc24245bf 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -78,13 +78,10 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, int time hostent* hostent) { // We receive ARES_EDESTRUCTION when destructing with pending queries. if (status == ARES_EDESTRUCTION) { - ASSERT(owned_); delete this; return; } - if (!fallback_if_failed_) { - completed_ = true; - } + bool completed = !fallback_if_failed_; std::list address_list; if (status == ARES_SUCCESS) { @@ -110,7 +107,7 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, int time } } if (!address_list.empty()) { - completed_ = true; + completed = true; } } @@ -118,23 +115,16 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, int time ENVOY_LOG(debug, "DNS request timed out {} times", timeouts); } - if (completed_) { - if (!cancelled_) { - dispatcher_.post( - [callback = callback_, al = std::move(address_list)] { callback(std::move(al)); }); - } - if (owned_) { + if (completed) { + dispatcher_.post([this, al = std::move(address_list)] { + if (!cancelled_) { + callback_(std::move(al)); + } delete this; - return; - } - } - - if (!completed_ && fallback_if_failed_) { + }); + } else if (fallback_if_failed_) { fallback_if_failed_ = false; getHostByName(AF_INET); - // Note: Nothing can follow this call to getHostByName due to deletion of this - // object upon synchronous resolution. - return; } } @@ -197,19 +187,16 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, pending_resolution->getHostByName(AF_INET6); } - if (pending_resolution->completed_) { - // Resolution does not need asynchronous behavior or network events. For - // example, localhost lookup. - return nullptr; - } else { - // Enable timer to wake us up if the request times out. - updateAresTimer(); + // Enable timer to wake us up if there is a timeout set so we can detect when + // therequest times out. + updateAresTimer(); - // The PendingResolution will self-delete when the request completes - // (including if cancelled or if ~DnsResolverImpl() happens). - pending_resolution->owned_ = true; - return pending_resolution.release(); - } + // The PendingResolution will self-delete when the request completes + // (including if cancelled or if ~DnsResolverImpl() happens). We always + // return an ActiveDnsQuery, since even synchronous resolution, for example + // localhost lookup, involves a dispatcher post and deferred callback due to + // #4307. + return pending_resolution.release(); } void DnsResolverImpl::PendingResolution::getHostByName(int family) { diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 2f79190e8e18..42d2781251a1 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -37,6 +37,8 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggableresolve( + EXPECT_NE(nullptr, resolver_->resolve( "localhost", DnsLookupFamily::V4Only, [&](const std::list&& results) -> void { address_list = results; @@ -501,7 +501,7 @@ TEST_P(DnsImplTest, LocalLookup) { const std::string error_msg = "Synchronous DNS IPv6 localhost resolution failed. Please verify localhost resolves to ::1 " "in /etc/hosts, since this misconfiguration is a common cause of these failures."; - EXPECT_EQ(nullptr, resolver_->resolve( + EXPECT_NE(nullptr, resolver_->resolve( "localhost", DnsLookupFamily::V6Only, [&](const std::list&& results) -> void { address_list = results; @@ -511,7 +511,7 @@ TEST_P(DnsImplTest, LocalLookup) { EXPECT_TRUE(hasAddress(address_list, "::1")) << error_msg; EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); - EXPECT_EQ(nullptr, resolver_->resolve( + EXPECT_NE(nullptr, resolver_->resolve( "localhost", DnsLookupFamily::Auto, [&](const std::list&& results) -> void { address_list = results; @@ -728,6 +728,18 @@ TEST_P(DnsImplTest, Cancel) { EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); } +// Validate working of cancellation provided by ActiveDnsQuery return when +// resolution is synchronous. +TEST_P(DnsImplTest, CancelImmediate) { + ActiveDnsQuery* query = resolver_->resolve( + "127.0.0.1", DnsLookupFamily::Auto, + [](const std::list &&) -> void { FAIL(); }); + ASSERT_NE(nullptr, query); + query->cancel(); + + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); +} + class DnsImplZeroTimeoutTest : public DnsImplTest { protected: bool zero_timeout() const override { return true; } diff --git a/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-4797819802615808 b/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-4797819802615808 new file mode 100644 index 000000000000..c6469d2f3c47 --- /dev/null +++ b/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-4797819802615808 @@ -0,0 +1,99 @@ +static_resources { + clusters { + name: "i" + type: STRICT_DNS + connect_timeout { + nanos: 65535 + } + hosts { + pipe { + path: "i" + } + } + tls_context { + common_tls_context { + validation_context { + verify_certificate_hash: "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" + } + } + } + dns_lookup_family: V4_ONLY + } +} +cluster_manager { + load_stats_config { + api_type: GRPC + grpc_services { + google_grpc { + target_uri: "[" + call_credentials { + service_account_jwt_access { + json_key: "[]" + token_lifetime_seconds: 1 + } + } + call_credentials { + access_token: "[" + } + call_credentials { + google_iam { + authorization_token: "/" + } + } + call_credentials { + service_account_jwt_access { + json_key: "[]" + token_lifetime_seconds: 1 + } + } + call_credentials { + access_token: "[" + } + call_credentials { + google_iam { + authorization_token: "/" + } + } + call_credentials { + service_account_jwt_access { + json_key: "[[" + token_lifetime_seconds: 1 + } + } + call_credentials { + access_token: "/" + } + call_credentials { + service_account_jwt_access { + json_key: "[[" + token_lifetime_seconds: 3 + } + } + call_credentials { + access_token: "[" + } + call_credentials { + google_iam { + authorization_token: "/" + } + } + call_credentials { + service_account_jwt_access { + json_key: "/" + token_lifetime_seconds: 1 + } + } + stat_prefix: "`" + } + } + } +} +flags_path: ",istener_9223372036854775808" +admin { + access_log_path: "@@" + address { + pipe { + path: "R" + } + } +}