Fix a dangling pointer on ares_request object in case of cancellation

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

@@ -201,7 +201,7 @@ void AresDnsResolver::ShutdownLocked() {
     grpc_timer_cancel(&next_resolution_timer_);
   }
   if (pending_request_ != nullptr) {
-    grpc_cancel_ares_request(pending_request_);
+    grpc_cancel_ares_request_locked(pending_request_);
   }
   if (next_completion_ != nullptr) {
     *target_result_ = nullptr;
@@ -298,6 +298,7 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) {
   grpc_channel_args* result = nullptr;
   GPR_ASSERT(r->resolving_);
   r->resolving_ = false;
+  gpr_free(r->pending_request_);
   r->pending_request_ = nullptr;
   if (r->lb_addresses_ != nullptr) {
     static const char* args_to_remove[2];

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

@@ -144,12 +144,12 @@ static void grpc_ares_request_unref_locked(grpc_ares_request* r) {
 void grpc_ares_complete_request_locked(grpc_ares_request* r) {
   /* Invoke on_done callback and destroy the
      request */
+  r->ev_driver = nullptr;
   grpc_lb_addresses* lb_addrs = *(r->lb_addrs_out);
   if (lb_addrs != nullptr) {
     grpc_cares_wrapper_address_sorting_sort(lb_addrs);
   }
   GRPC_CLOSURE_SCHED(r->on_done, r->error);
-  gpr_free(r);
 }
 
 static grpc_ares_hostbyname_request* create_hostbyname_request_locked(
@@ -356,15 +356,12 @@ done:
   grpc_ares_request_unref_locked(r);
 }
 
-static grpc_ares_request*
-grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
-    const char* dns_server, const char* name, const char* default_port,
-    grpc_pollset_set* interested_parties, grpc_closure* on_done,
-    grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json,
-    grpc_combiner* combiner) {
+void grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
+    grpc_ares_request* r, const char* dns_server, const char* name,
+    const char* default_port, grpc_pollset_set* interested_parties,
+    bool check_grpclb, grpc_combiner* combiner) {
   grpc_error* error = GRPC_ERROR_NONE;
   grpc_ares_hostbyname_request* hr = nullptr;
-  grpc_ares_request* r = nullptr;
   ares_channel* channel = nullptr;
   /* TODO(zyc): Enable tracing after #9603 is checked in */
   /* if (grpc_dns_trace) {
@@ -390,14 +387,6 @@ grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
     }
     port = gpr_strdup(default_port);
   }
-  r = static_cast<grpc_ares_request*>(gpr_zalloc(sizeof(grpc_ares_request)));
-  r->ev_driver = nullptr;
-  r->on_done = on_done;
-  r->lb_addrs_out = addrs;
-  r->service_config_json_out = service_config_json;
-  r->success = false;
-  r->error = GRPC_ERROR_NONE;
-  r->pending_queries = 0;
   error = grpc_ares_ev_driver_create_locked(&r->ev_driver, interested_parties,
                                             combiner, r);
   if (error != GRPC_ERROR_NONE) goto error_cleanup;
@@ -458,7 +447,7 @@ grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
                on_srv_query_done_locked, r);
     gpr_free(service_name);
   }
-  if (service_config_json != nullptr) {
+  if (r->service_config_json_out != nullptr) {
     grpc_ares_request_ref_locked(r);
     char* config_name;
     gpr_asprintf(&config_name, "_grpc_config.%s", host);
@@ -470,14 +459,12 @@ grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
   grpc_ares_request_unref_locked(r);
   gpr_free(host);
   gpr_free(port);
-  return r;
+  return;
 
 error_cleanup:
-  GRPC_CLOSURE_SCHED(on_done, error);
-  gpr_free(r);
+  GRPC_CLOSURE_SCHED(r->on_done, error);
   gpr_free(host);
   gpr_free(port);
-  return nullptr;
 }
 
 static bool inner_resolve_as_ip_literal_locked(const char* name,
@@ -536,21 +523,31 @@ static grpc_ares_request* grpc_dns_lookup_ares_locked_impl(
     grpc_pollset_set* interested_parties, grpc_closure* on_done,
     grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json,
     grpc_combiner* combiner) {
+  grpc_ares_request* r =
+      static_cast<grpc_ares_request*>(gpr_zalloc(sizeof(grpc_ares_request)));
+  r->ev_driver = nullptr;
+  r->on_done = on_done;
+  r->lb_addrs_out = addrs;
+  r->service_config_json_out = service_config_json;
+  r->success = false;
+  r->error = GRPC_ERROR_NONE;
+  r->pending_queries = 0;
   // 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);
-    return nullptr;
+    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);
-    return nullptr;
+    return r;
   }
   // Look up name using c-ares lib.
-  return grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
-      dns_server, name, default_port, interested_parties, on_done, addrs,
-      check_grpclb, service_config_json, combiner);
+  grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
+      r, dns_server, name, default_port, interested_parties, check_grpclb,
+      combiner);
+  return r;
 }
 
 grpc_ares_request* (*grpc_dns_lookup_ares_locked)(
@@ -559,13 +556,15 @@ grpc_ares_request* (*grpc_dns_lookup_ares_locked)(
     grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json,
     grpc_combiner* combiner) = grpc_dns_lookup_ares_locked_impl;
 
-void grpc_cancel_ares_request(grpc_ares_request* r) {
-  if (grpc_dns_lookup_ares_locked == grpc_dns_lookup_ares_locked_impl) {
-    if (r != nullptr) {
+static void grpc_cancel_ares_request_locked_impl(grpc_ares_request* r) {
+  GPR_ASSERT(r != nullptr);
+  if (r->ev_driver != nullptr) {
     grpc_ares_ev_driver_shutdown_locked(r->ev_driver);
   }
 }
-}
+
+void (*grpc_cancel_ares_request_locked)(grpc_ares_request* r) =
+    grpc_cancel_ares_request_locked_impl;
 
 grpc_error* grpc_ares_init(void) {
   gpr_once_init(&g_basic_init, do_basic_init);
@@ -603,20 +602,23 @@ typedef struct grpc_resolve_address_ares_request {
   grpc_lb_addresses* lb_addrs;
   /** closure to call when the resolve_address_ares request completes */
   grpc_closure* on_resolve_address_done;
-  /** a closure wrapping on_dns_lookup_done_cb, which should be invoked when the
-      grpc_dns_lookup_ares_locked operation is done. */
-  grpc_closure on_dns_lookup_done;
+  /** a closure wrapping on_resolve_address_done, which should be invoked when
+     the grpc_dns_lookup_ares_locked operation is done. */
+  grpc_closure on_dns_lookup_done_locked;
   /* target name */
   const char* name;
   /* default port to use if none is specified */
   const char* default_port;
   /* pollset_set to be driven by */
   grpc_pollset_set* interested_parties;
+  /* underlying ares_request that the query is performed on */
+  grpc_ares_request* ares_request;
 } grpc_resolve_address_ares_request;
 
-static void on_dns_lookup_done_cb(void* arg, grpc_error* error) {
+static void on_dns_lookup_done_locked(void* arg, grpc_error* error) {
   grpc_resolve_address_ares_request* r =
       static_cast<grpc_resolve_address_ares_request*>(arg);
+  gpr_free(r->ares_request);
   grpc_resolved_addresses** resolved_addresses = r->addrs_out;
   if (r->lb_addrs == nullptr || r->lb_addrs->num_addresses == 0) {
     *resolved_addresses = nullptr;
@@ -643,9 +645,9 @@ static void grpc_resolve_address_invoke_dns_lookup_ares_locked(
     void* arg, grpc_error* unused_error) {
   grpc_resolve_address_ares_request* r =
       static_cast<grpc_resolve_address_ares_request*>(arg);
-  grpc_dns_lookup_ares_locked(
+  r->ares_request = grpc_dns_lookup_ares_locked(
       nullptr /* dns_server */, r->name, r->default_port, r->interested_parties,
-      &r->on_dns_lookup_done, &r->lb_addrs, false /* check_grpclb */,
+      &r->on_dns_lookup_done_locked, &r->lb_addrs, false /* check_grpclb */,
       nullptr /* service_config_json */, r->combiner);
 }
 
@@ -660,8 +662,8 @@ static void grpc_resolve_address_ares_impl(const char* name,
   r->combiner = grpc_combiner_create();
   r->addrs_out = addrs;
   r->on_resolve_address_done = on_done;
-  GRPC_CLOSURE_INIT(&r->on_dns_lookup_done, on_dns_lookup_done_cb, r,
-                    grpc_schedule_on_exec_ctx);
+  GRPC_CLOSURE_INIT(&r->on_dns_lookup_done_locked, on_dns_lookup_done_locked, r,
+                    grpc_combiner_scheduler(r->combiner));
   r->name = name;
   r->default_port = default_port;
   r->interested_parties = interested_parties;

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

@@ -54,7 +54,8 @@ extern void (*grpc_resolve_address_ares)(const char* name,
   port in \a name. grpc_ares_init() must be called at least once before this
   function. \a on_done may be called directly in this function without being
   scheduled with \a exec_ctx, so it must not try to acquire locks that are
-  being held by the caller. */
+  being held by the caller. The returned grpc_ares_request object is owned
+  by the caller and it is safe to free after on_done is called back. */
 extern grpc_ares_request* (*grpc_dns_lookup_ares_locked)(
     const char* dns_server, const char* name, const char* default_port,
     grpc_pollset_set* interested_parties, grpc_closure* on_done,
@@ -62,7 +63,7 @@ extern grpc_ares_request* (*grpc_dns_lookup_ares_locked)(
     char** service_config_json, grpc_combiner* combiner);
 
 /* Cancel the pending grpc_ares_request \a request */
-void grpc_cancel_ares_request(grpc_ares_request* request);
+extern void (*grpc_cancel_ares_request_locked)(grpc_ares_request* request);
 
 /* Initialize gRPC ares wrapper. Must be called at least once before
    grpc_resolve_address_ares(). */

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

@@ -40,7 +40,10 @@ grpc_ares_request* (*grpc_dns_lookup_ares_locked)(
     grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json,
     grpc_combiner* combiner) = grpc_dns_lookup_ares_locked_impl;
 
-void grpc_cancel_ares_request(grpc_ares_request* r) {}
+static void grpc_cancel_ares_request_locked_impl(grpc_ares_request* r) {}
+
+void (*grpc_cancel_ares_request_locked)(grpc_ares_request* r) =
+    grpc_cancel_ares_request_locked_impl;
 
 grpc_error* grpc_ares_init(void) { return GRPC_ERROR_NONE; }
 

test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc

@@ -82,6 +82,10 @@ static grpc_ares_request* my_dns_lookup_ares_locked(
   return nullptr;
 }
 
+static void my_cancel_ares_request_locked(grpc_ares_request* request) {
+  GPR_ASSERT(request == nullptr);
+}
+
 static grpc_core::OrphanablePtr<grpc_core::Resolver> create_resolver(
     const char* name) {
   grpc_core::ResolverFactory* factory =
@@ -148,6 +152,7 @@ int main(int argc, char** argv) {
   g_combiner = grpc_combiner_create();
   grpc_set_resolver_impl(&test_resolver);
   grpc_dns_lookup_ares_locked = my_dns_lookup_ares_locked;
+  grpc_cancel_ares_request_locked = my_cancel_ares_request_locked;
   grpc_channel_args* result = (grpc_channel_args*)1;
 
   {

test/core/end2end/fuzzers/api_fuzzer.cc

@@ -390,6 +390,10 @@ grpc_ares_request* my_dns_lookup_ares_locked(
   return nullptr;
 }
 
+static void my_cancel_ares_request_locked(grpc_ares_request* request) {
+  GPR_ASSERT(request == nullptr);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // client connection
 
@@ -705,6 +709,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   }
   grpc_set_resolver_impl(&fuzzer_resolver);
   grpc_dns_lookup_ares_locked = my_dns_lookup_ares_locked;
+  grpc_cancel_ares_request_locked = my_cancel_ares_request_locked;
 
   GPR_ASSERT(g_channel == nullptr);
   GPR_ASSERT(g_server == nullptr);

test/core/end2end/goaway_server_test.cc

@@ -50,6 +50,8 @@ static grpc_ares_request* (*iomgr_dns_lookup_ares_locked)(
     grpc_lb_addresses** addresses, bool check_grpclb,
     char** service_config_json, grpc_combiner* combiner);
 
+static void (*iomgr_cancel_ares_request_locked)(grpc_ares_request* request);
+
 static void set_resolve_port(int port) {
   gpr_mu_lock(&g_mu);
   g_resolve_port = port;
@@ -130,6 +132,12 @@ static grpc_ares_request* my_dns_lookup_ares_locked(
   return nullptr;
 }
 
+static void my_cancel_ares_request_locked(grpc_ares_request* request) {
+  if (request != nullptr) {
+    iomgr_cancel_ares_request_locked(request);
+  }
+}
+
 int main(int argc, char** argv) {
   grpc_completion_queue* cq;
   cq_verifier* cqv;
@@ -143,7 +151,9 @@ int main(int argc, char** argv) {
   default_resolver = grpc_resolve_address_impl;
   grpc_set_resolver_impl(&test_resolver);
   iomgr_dns_lookup_ares_locked = grpc_dns_lookup_ares_locked;
+  iomgr_cancel_ares_request_locked = grpc_cancel_ares_request_locked;
   grpc_dns_lookup_ares_locked = my_dns_lookup_ares_locked;
+  grpc_cancel_ares_request_locked = my_cancel_ares_request_locked;
 
   int was_cancelled1;
   int was_cancelled2;