dns resolver: invoke resolve callback if not cancelled (#10060)

Description:PendingResolutions get destroyed when complete or when c-ares sent ARES_EDESTRUCTION. Prior to #9899 ARES_EDESTRUCTION only happened when the resolver was destroyed. However, after #9899 the channel, and thus PendingResolutions can be destroyed, without the callback target being aware. This leads to potential use after free issues. This PR calls the resolve callback when the status received is ARES_EDESTRUCTION.
Risk Level: med
Testing: added test that reproduced segfault and passed after fix.

Signed-off-by: Jose Nino <jnino@lyft.com>

source/common/network/dns_impl.cc

@@ -84,6 +84,13 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
   // We receive ARES_EDESTRUCTION when destructing with pending queries.
   if (status == ARES_EDESTRUCTION) {
     ASSERT(owned_);
+    // This destruction might have been triggered by a peer PendingResolution that received a
+    // ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the
+    // callback_ target _should_ still be around. In that case, raise the callback_ so the target
+    // can be done with this query and initiate a new one.
+    if (!cancelled_) {
+      callback_({});
+    }
     delete this;
     return;
   }
@@ -251,8 +258,9 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name,
     // Enable timer to wake us up if the request times out.
     updateAresTimer();
 
-    // The PendingResolution will self-delete when the request completes
-    // (including if cancelled or if ~DnsResolverImpl() happens).
+    // The PendingResolution will self-delete when the request completes (including if cancelled or
+    // if ~DnsResolverImpl() happens via ares_destroy() and subsequent handling of ARES_EDESTRUCTION
+    // in DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback()).
     pending_resolution->owned_ = true;
     return pending_resolution.release();
   }

test/common/network/dns_impl_test.cc

@@ -476,20 +476,41 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplTest,
                          TestUtility::ipTestParamsToString);
 
 // Validate that when DnsResolverImpl is destructed with outstanding requests,
-// that we don't invoke any callbacks. This is a regression test from
+// that we don't invoke any callbacks if the query was cancelled. This is a regression test from
 // development, where segfaults were encountered due to callback invocations on
 // destruction.
 TEST_P(DnsImplTest, DestructPending) {
-  EXPECT_NE(nullptr, resolver_->resolve("", DnsLookupFamily::V4Only,
-                                        [&](std::list<DnsResponse>&& results) -> void {
-                                          FAIL();
-                                          UNREFERENCED_PARAMETER(results);
-                                        }));
+  ActiveDnsQuery* query = resolver_->resolve("", DnsLookupFamily::V4Only,
+                                             [&](std::list<DnsResponse>&& results) -> void {
+                                               FAIL();
+                                               UNREFERENCED_PARAMETER(results);
+                                             });
+  ASSERT_NE(nullptr, query);
+  query->cancel();
   // Also validate that pending events are around to exercise the resource
   // reclamation path.
   EXPECT_GT(peer_->events().size(), 0U);
 }
 
+TEST_P(DnsImplTest, DestructCallback) {
+  server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A);
+
+  ActiveDnsQuery* query = resolver_->resolve("some.domain", DnsLookupFamily::Auto,
+                                             [&](std::list<DnsResponse> &&) -> void {
+                                               query = nullptr;
+                                               dispatcher_->exit();
+                                             });
+
+  // This simulates destruction thanks to another query setting the dirty_channel_ bit, thus causing
+  // a subsequent result to call ares_destroy.
+  peer_->resetChannelTcpOnly(zero_timeout());
+  ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str());
+
+  dispatcher_->run(Event::Dispatcher::RunType::Block);
+
+  ASSERT_EQ(nullptr, query);
+}
+
 // Validate basic success/fail lookup behavior. The empty request will connect
 // to TestDnsServer, but localhost should resolve via the hosts file with no
 // asynchronous behavior or network events.
@@ -946,16 +967,12 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplAresFlagsForTcpTest,
 TEST_P(DnsImplAresFlagsForTcpTest, TcpLookupsEnabled) {
   server_->addCName("root.cnam.domain", "result.cname.domain");
   server_->addHosts("result.cname.domain", {"201.134.56.7"}, RecordType::A);
-  std::list<Address::InstanceConstSharedPtr> address_list;
   ares_options opts{};
   int optmask = 0;
   EXPECT_EQ(ARES_SUCCESS, ares_save_options(peer_->channel(), &opts, &optmask));
   EXPECT_TRUE((opts.flags & ARES_FLAG_USEVC) == ARES_FLAG_USEVC);
   EXPECT_NE(nullptr, resolver_->resolve("root.cnam.domain", DnsLookupFamily::Auto,
-                                        [&](std::list<DnsResponse>&& results) -> void {
-                                          address_list = getAddressList(results);
-                                          dispatcher_->exit();
-                                        }));
+                                        [&](std::list<DnsResponse> &&) -> void {}));
   ares_destroy_options(&opts);
 }
 
@@ -974,16 +991,12 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplAresFlagsForUdpTest,
 TEST_P(DnsImplAresFlagsForUdpTest, UdpLookupsEnabled) {
   server_->addCName("root.cnam.domain", "result.cname.domain");
   server_->addHosts("result.cname.domain", {"201.134.56.7"}, RecordType::A);
-  std::list<Address::InstanceConstSharedPtr> address_list;
   ares_options opts{};
   int optmask = 0;
   EXPECT_EQ(ARES_SUCCESS, ares_save_options(peer_->channel(), &opts, &optmask));
   EXPECT_FALSE((opts.flags & ARES_FLAG_USEVC) == ARES_FLAG_USEVC);
   EXPECT_NE(nullptr, resolver_->resolve("root.cnam.domain", DnsLookupFamily::Auto,
-                                        [&](std::list<DnsResponse>&& results) -> void {
-                                          address_list = getAddressList(results);
-                                          dispatcher_->exit();
-                                        }));
+                                        [&](std::list<DnsResponse> &&) -> void {}));
   ares_destroy_options(&opts);
 }