From 0f4968a6d9244f3bcef89bbb1af60fdfcaf5d5b1 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Wed, 17 Oct 2018 11:29:45 -0700 Subject: Fix a dangling pointer on ares_request object in case of cancellation --- .../resolver/dns/c_ares/dns_resolver_ares.cc | 3 +- .../resolver/dns/c_ares/grpc_ares_wrapper.cc | 80 +++++++++++----------- .../resolver/dns/c_ares/grpc_ares_wrapper.h | 5 +- .../dns/c_ares/grpc_ares_wrapper_fallback.cc | 5 +- 4 files changed, 50 insertions(+), 43 deletions(-) (limited to 'src') 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 dfa52867d8..cfb90eb570 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(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(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(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(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; } -- cgit v1.2.3