Merge pull request #16913 from apolcyn/resolver_owns_ares_requests

Fix a dangling pointer on ares_request object in case of cancellation

4 files changed, 50 insertions, 43 deletions

diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc
index c2a4d42203..01796ca08f 100644
--- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc
+++ b/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];
diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc
index 4c795c34c8..582e2203fc 100644
--- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc
+++ b/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,14 +556,16 @@ 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) {
-      grpc_ares_ev_driver_shutdown_locked(r->ev_driver);
-    }
+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);
   gpr_mu_lock(&g_init_mu);
@@ -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;
diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h
index 1bc457d4cf..a1231cc4e0 100644
--- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h
+++ b/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(). */
diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc
index d6a76fc8b6..9f293c1ac0 100644
--- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc
+++ b/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; }