aboutsummaryrefslogtreecommitdiffhomepage
path: root/src
diff options
context:
space:
mode:
authorGravatar apolcyn <apolcyn@google.com>2018-10-19 14:18:09 -0700
committerGravatar GitHub <noreply@github.com>2018-10-19 14:18:09 -0700
commit6c29457ccb637ebfe12585acdbc4d9206a485b6e (patch)
tree6e59a9a46ee586cd6f65a78cd528090bcb7f34f7 /src
parentcc3f52835ae491a05a069ad11c271fe5b22074d5 (diff)
parent0f4968a6d9244f3bcef89bbb1af60fdfcaf5d5b1 (diff)
Merge pull request #16913 from apolcyn/resolver_owns_ares_requests
Fix a dangling pointer on ares_request object in case of cancellation
Diffstat (limited to 'src')
-rw-r--r--src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc3
-rw-r--r--src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc80
-rw-r--r--src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h5
-rw-r--r--src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc5
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; }