dns: fix callback contract bug in #4307.

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::InstanceConstSharedPtr> address_list;
   if (status == ARES_SUCCESS) {
@@ -110,31 +107,24 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, int time
       }
     }
     if (!address_list.empty()) {
-      completed_ = true;
+      completed = true;
     }
   }
 
   if (timeouts > 0) {
     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) {

source/common/network/dns_impl.h

@@ -37,6 +37,8 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
 
 private:
   friend class DnsResolverImplPeer;
+  // This object is self-owned, resource reclamation occurs via self-deleting on
+  // query completion or error.
   struct PendingResolution : public ActiveDnsQuery {
     // Network::ActiveDnsQuery
     PendingResolution(ResolveCb callback, Event::Dispatcher& dispatcher, ares_channel channel,
@@ -66,11 +68,6 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
     const ResolveCb callback_;
     // Dispatcher to post callback_ to.
     Event::Dispatcher& dispatcher_;
-    // Does the object own itself? Resource reclamation occurs via self-deleting
-    // on query completion or error.
-    bool owned_ = false;
-    // Has the query completed? Only meaningful if !owned_;
-    bool completed_ = false;
     // Was the query cancelled via cancel()?
     bool cancelled_ = false;
     // If dns_lookup_family is "fallback", fallback to v4 address if v6

test/common/network/dns_impl_test.cc

@@ -487,7 +487,7 @@ TEST_P(DnsImplTest, LocalLookup) {
 
   if (GetParam() == Address::IpVersion::v4) {
     // EXPECT_CALL(dispatcher_, post(_));
-    EXPECT_EQ(nullptr, resolver_->resolve(
+    EXPECT_NE(nullptr, resolver_->resolve(
                            "localhost", DnsLookupFamily::V4Only,
                            [&](const std::list<Address::InstanceConstSharedPtr>&& 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<Address::InstanceConstSharedPtr>&& 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<Address::InstanceConstSharedPtr>&& 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<Address::InstanceConstSharedPtr> &&) -> 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; }

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"
+    }
+  }
+}