Merge pull request #17790 from apolcyn/fix_sorting_bypass

Fix windows localhost address sorting bug

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc

@@ -548,13 +548,13 @@ static grpc_ares_request* grpc_dns_lookup_ares_locked_impl(
       r, name, default_port);
   // Early out if the target is an ipv4 or ipv6 literal.
   if (resolve_as_ip_literal_locked(name, default_port, addrs)) {
-    GRPC_CLOSURE_SCHED(on_done, GRPC_ERROR_NONE);
+    grpc_ares_complete_request_locked(r);
     return r;
   }
   // Early out if the target is localhost and we're on Windows.
   if (grpc_ares_maybe_resolve_localhost_manually_locked(name, default_port,
                                                         addrs)) {
-    GRPC_CLOSURE_SCHED(on_done, GRPC_ERROR_NONE);
+    grpc_ares_complete_request_locked(r);
     return r;
   }
   // Don't query for SRV and TXT records if the target is "localhost", so

test/core/iomgr/resolve_address_test.cc

@@ -23,6 +23,8 @@
 #include <grpc/support/sync.h>
 #include <grpc/support/time.h>
 
+#include <address_sorting/address_sorting.h>
+
 #include <string.h>
 
 #include "src/core/lib/gpr/env.h"
@@ -120,6 +122,35 @@ static void must_fail(void* argsp, grpc_error* err) {
   gpr_mu_unlock(args->mu);
 }
 
+// This test assumes the environment has an ipv6 loopback
+static void must_succeed_with_ipv6_first(void* argsp, grpc_error* err) {
+  args_struct* args = static_cast<args_struct*>(argsp);
+  GPR_ASSERT(err == GRPC_ERROR_NONE);
+  GPR_ASSERT(args->addrs != nullptr);
+  GPR_ASSERT(args->addrs->naddrs > 0);
+  const struct sockaddr* first_address =
+      reinterpret_cast<const struct sockaddr*>(args->addrs->addrs[0].addr);
+  GPR_ASSERT(first_address->sa_family == AF_INET6);
+  gpr_atm_rel_store(&args->done_atm, 1);
+  gpr_mu_lock(args->mu);
+  GRPC_LOG_IF_ERROR("pollset_kick", grpc_pollset_kick(args->pollset, nullptr));
+  gpr_mu_unlock(args->mu);
+}
+
+static void must_succeed_with_ipv4_first(void* argsp, grpc_error* err) {
+  args_struct* args = static_cast<args_struct*>(argsp);
+  GPR_ASSERT(err == GRPC_ERROR_NONE);
+  GPR_ASSERT(args->addrs != nullptr);
+  GPR_ASSERT(args->addrs->naddrs > 0);
+  const struct sockaddr* first_address =
+      reinterpret_cast<const struct sockaddr*>(args->addrs->addrs[0].addr);
+  GPR_ASSERT(first_address->sa_family == AF_INET);
+  gpr_atm_rel_store(&args->done_atm, 1);
+  gpr_mu_lock(args->mu);
+  GRPC_LOG_IF_ERROR("pollset_kick", grpc_pollset_kick(args->pollset, nullptr));
+  gpr_mu_unlock(args->mu);
+}
+
 static void test_localhost(void) {
   grpc_core::ExecCtx exec_ctx;
   args_struct args;
@@ -146,6 +177,33 @@ static void test_default_port(void) {
   args_finish(&args);
 }
 
+static void test_localhost_result_has_ipv6_first(void) {
+  grpc_core::ExecCtx exec_ctx;
+  args_struct args;
+  args_init(&args);
+  grpc_resolve_address("localhost:1", nullptr, args.pollset_set,
+                       GRPC_CLOSURE_CREATE(must_succeed_with_ipv6_first, &args,
+                                           grpc_schedule_on_exec_ctx),
+                       &args.addrs);
+  grpc_core::ExecCtx::Get()->Flush();
+  poll_pollset_until_request_done(&args);
+  args_finish(&args);
+}
+
+static void test_localhost_result_has_ipv4_first_when_ipv6_isnt_available(
+    void) {
+  grpc_core::ExecCtx exec_ctx;
+  args_struct args;
+  args_init(&args);
+  grpc_resolve_address("localhost:1", nullptr, args.pollset_set,
+                       GRPC_CLOSURE_CREATE(must_succeed_with_ipv4_first, &args,
+                                           grpc_schedule_on_exec_ctx),
+                       &args.addrs);
+  grpc_core::ExecCtx::Get()->Flush();
+  poll_pollset_until_request_done(&args);
+  args_finish(&args);
+}
+
 static void test_non_numeric_default_port(void) {
   grpc_core::ExecCtx exec_ctx;
   args_struct args;
@@ -245,6 +303,34 @@ static void test_unparseable_hostports(void) {
   }
 }
 
+typedef struct mock_ipv6_disabled_source_addr_factory {
+  address_sorting_source_addr_factory base;
+} mock_ipv6_disabled_source_addr_factory;
+
+static bool mock_ipv6_disabled_source_addr_factory_get_source_addr(
+    address_sorting_source_addr_factory* factory,
+    const address_sorting_address* dest_addr,
+    address_sorting_address* source_addr) {
+  // Mock lack of IPv6. For IPv4, set the source addr to be the same
+  // as the destination; tests won't actually connect on the result anyways.
+  if (address_sorting_abstract_get_family(dest_addr) ==
+      ADDRESS_SORTING_AF_INET6) {
+    return false;
+  }
+  memcpy(source_addr->addr, &dest_addr->addr, dest_addr->len);
+  source_addr->len = dest_addr->len;
+  return true;
+}
+
+void mock_ipv6_disabled_source_addr_factory_destroy(
+    address_sorting_source_addr_factory* factory) {}
+
+const address_sorting_source_addr_factory_vtable
+    kMockIpv6DisabledSourceAddrFactoryVtable = {
+        mock_ipv6_disabled_source_addr_factory_get_source_addr,
+        mock_ipv6_disabled_source_addr_factory_destroy,
+};
+
 int main(int argc, char** argv) {
   // First set the resolver type based off of --resolver
   const char* resolver_type = nullptr;
@@ -289,11 +375,26 @@ int main(int argc, char** argv) {
       // these unit tests under c-ares risks flakiness.
       test_invalid_ip_addresses();
       test_unparseable_hostports();
+    } else {
+      test_localhost_result_has_ipv6_first();
     }
     grpc_core::Executor::ShutdownAll();
   }
   gpr_cmdline_destroy(cl);
-
   grpc_shutdown();
+  // The following test uses
+  // "address_sorting_override_source_addr_factory_for_testing", which works
+  // on a per-grpc-init basis, and so it's simplest to run this next test
+  // within a standalone grpc_init/grpc_shutdown pair.
+  if (gpr_stricmp(resolver_type, "ares") == 0) {
+    // Run a test case in which c-ares's address sorter
+    // thinks that IPv4 is available and IPv6 isn't.
+    grpc_init();
+    mock_ipv6_disabled_source_addr_factory factory;
+    factory.base.vtable = &kMockIpv6DisabledSourceAddrFactoryVtable;
+    address_sorting_override_source_addr_factory_for_testing(&factory.base);
+    test_localhost_result_has_ipv4_first_when_ipv6_isnt_available();
+    grpc_shutdown();
+  }
   return 0;
 }