diff --git a/evdns.c b/evdns.c index 4d25cd3d..b357606d 100644 --- a/evdns.c +++ b/evdns.c @@ -139,7 +139,7 @@ /* Persistent handle. We keep this separate from 'struct request' since we * need some object to last for as long as an evdns_request is outstanding so - * that it can be cancelled, whereas a search request can lead to multiple + * that it can be canceled, whereas a search request can lead to multiple * 'struct request' instances being created over its lifetime. */ struct evdns_request { struct request *current_req; @@ -4052,11 +4052,17 @@ struct evdns_getaddrinfo_request { /* If we have one request answered and one request still inflight, * then this field holds the answer from the first request... */ struct evutil_addrinfo *pending_result; - /* And this field holds the error code from the first request... */ - int pending_error; /* And this event is a timeout that will tell us to cancel the second * request if it's taking a long time. */ struct event timeout; + + /* And this field holds the error code from the first request... */ + int pending_error; + /* If this is set, the user canceled this request. */ + unsigned user_canceled : 1; + /* If this is set, the user can no longer cancel this request; we're + * just waiting for the free. */ + unsigned request_done : 1; }; /* Convert an evdns errors to the equivalent getaddrinfo error. */ @@ -4086,6 +4092,8 @@ getaddrinfo_merge_err(int e1, int e2) static void free_getaddrinfo_request(struct evdns_getaddrinfo_request *data) { + /* DO NOT CALL this if either of the requests is pending. Only once + * both callbacks have been invoked is it safe to free the request */ if (data->pending_result) evutil_freeaddrinfo(data->pending_result); if (data->cname_result) @@ -4118,7 +4126,6 @@ evdns_getaddrinfo_timeout_cb(evutil_socket_t fd, short what, void *ptr) /* Cancel any pending requests, and note which one */ if (data->ipv4_request.r) { evdns_cancel_request(NULL, data->ipv4_request.r); - data->ipv4_request.r = NULL; v4_timedout = 1; EVDNS_LOCK(data->evdns_base); ++data->evdns_base->getaddrinfo_ipv4_timeouts; @@ -4126,7 +4133,6 @@ evdns_getaddrinfo_timeout_cb(evutil_socket_t fd, short what, void *ptr) } if (data->ipv6_request.r) { evdns_cancel_request(NULL, data->ipv6_request.r); - data->ipv6_request.r = NULL; v6_timedout = 1; EVDNS_LOCK(data->evdns_base); ++data->evdns_base->getaddrinfo_ipv6_timeouts; @@ -4149,7 +4155,10 @@ evdns_getaddrinfo_timeout_cb(evutil_socket_t fd, short what, void *ptr) data->user_cb(e, NULL, data->user_data); } - free_getaddrinfo_request(data); + if (!v4_timedout && !v6_timedout) { + /* should be impossible? XXXX */ + free_getaddrinfo_request(data); + } } static int @@ -4159,6 +4168,13 @@ evdns_getaddrinfo_set_timeout(struct evdns_base *evdns_base, return event_add(&data->timeout, &evdns_base->global_getaddrinfo_allow_skew); } +static inline int +evdns_result_is_answer(int result) +{ + return (result != DNS_ERR_NOTIMPL && result != DNS_ERR_REFUSED && + result != DNS_ERR_SERVERFAILED && result != DNS_ERR_CANCEL); +} + static void evdns_getaddrinfo_gotresolve(int result, char type, int count, int ttl, void *addresses, void *arg) @@ -4176,33 +4192,39 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count, int socklen, addrlen; void *addrp; int err; - - if (result == DNS_ERR_CANCEL) - return; + int user_canceled; EVUTIL_ASSERT(req->type == DNS_IPv4_A || req->type == DNS_IPv6_AAAA); if (req->type == DNS_IPv4_A) { data = EVUTIL_UPCAST(req, struct evdns_getaddrinfo_request, ipv4_request); other_req = &data->ipv6_request; - if (result != DNS_ERR_NOTIMPL && result != DNS_ERR_REFUSED && - result != DNS_ERR_SERVERFAILED) { - EVDNS_LOCK(data->evdns_base); - ++data->evdns_base->getaddrinfo_ipv4_answered; - EVDNS_UNLOCK(data->evdns_base); - } } else { data = EVUTIL_UPCAST(req, struct evdns_getaddrinfo_request, ipv6_request); other_req = &data->ipv4_request; - if (result != DNS_ERR_NOTIMPL && result != DNS_ERR_REFUSED && - result != DNS_ERR_SERVERFAILED) { - EVDNS_LOCK(data->evdns_base); - ++data->evdns_base->getaddrinfo_ipv6_answered; - EVDNS_UNLOCK(data->evdns_base); - } } + EVDNS_LOCK(data->evdns_base); + if (evdns_result_is_answer(result)) { + if (req->type == DNS_IPv4_A) + ++data->evdns_base->getaddrinfo_ipv4_answered; + else + ++data->evdns_base->getaddrinfo_ipv6_answered; + } + user_canceled = data->user_canceled; + if (other_req->r == NULL) + data->request_done = 1; + EVDNS_UNLOCK(data->evdns_base); + req->r = NULL; + if (result == DNS_ERR_CANCEL && ! user_canceled) { + /* Internal cancel request from timeout or internal error. + * we already answered the user. */ + if (other_req->r == NULL) + free_getaddrinfo_request(data); + return; + } + if (result == DNS_ERR_NONE) { if (count == 0) err = EVUTIL_EAI_NODATA; @@ -4223,8 +4245,11 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count, return; } - if (data->pending_result) { - /* If we have an answer waiting, ignore this error. */ + if (user_canceled) { + data->user_cb(EVUTIL_EAI_CANCEL, NULL, data->user_data); + } else if (data->pending_result) { + /* If we have an answer waiting, and we weren't + * canceled, ignore this error. */ add_cname_to_reply(data, data->pending_result); data->user_cb(0, data->pending_result, data->user_data); data->pending_result = NULL; @@ -4236,6 +4261,16 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count, } free_getaddrinfo_request(data); return; + } else if (user_canceled) { + if (other_req->r) { + /* The other request is still working; let it hit this + * callback with EVUTIL_EAI_CANCEL callback and report + * the failure. */ + return; + } + data->user_cb(EVUTIL_EAI_CANCEL, NULL, data->user_data); + free_getaddrinfo_request(data); + return; } /* Looks like we got some answers. We should turn them into addrinfos @@ -4270,12 +4305,12 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count, if (!ai) { if (other_req->r) { evdns_cancel_request(NULL, other_req->r); - other_req->r = NULL; } data->user_cb(EVUTIL_EAI_MEMORY, NULL, data->user_data); evutil_freeaddrinfo(res); - free_getaddrinfo_request(data); + if (other_req->r == NULL) + free_getaddrinfo_request(data); return; } res = evutil_addrinfo_append(res, ai); @@ -4495,14 +4530,16 @@ evdns_getaddrinfo(struct evdns_base *dns_base, void evdns_getaddrinfo_cancel(struct evdns_getaddrinfo_request *data) { + EVDNS_LOCK(data->evdns_base); + if (data->request_done) { + EVDNS_UNLOCK(data->evdns_base); + return; + } event_del(&data->timeout); + data->user_canceled = 1; if (data->ipv4_request.r) evdns_cancel_request(data->evdns_base, data->ipv4_request.r); if (data->ipv6_request.r) evdns_cancel_request(data->evdns_base, data->ipv6_request.r); - data->ipv4_request.r = data->ipv6_request.r = NULL; - - data->user_cb(EVUTIL_EAI_CANCEL, NULL, data->user_data); - - free_getaddrinfo_request(data); + EVDNS_UNLOCK(data->evdns_base); } diff --git a/evutil.c b/evutil.c index 46c8333d..db827cfd 100644 --- a/evutil.c +++ b/evutil.c @@ -1348,7 +1348,7 @@ evutil_gai_strerror(int err) * conflict with the platform's native error codes. */ switch (err) { case EVUTIL_EAI_CANCEL: - return "Request cancelled"; + return "Request canceled"; case 0: return "No error"; diff --git a/include/event2/dns.h b/include/event2/dns.h index bfb6dc9f..3bb6f9eb 100644 --- a/include/event2/dns.h +++ b/include/event2/dns.h @@ -628,7 +628,7 @@ struct evdns_getaddrinfo_request *evdns_getaddrinfo( evdns_getaddrinfo_cb cb, void *arg); /* Cancel an in-progress evdns_getaddrinfo. This MUST NOT be called after the - * getaddrinfo's callback has been invoked. The resolves will be cancelled, + * getaddrinfo's callback has been invoked. The resolves will be canceled, * and the callback will be invoked with the error EVUTIL_EAI_CANCEL. */ void evdns_getaddrinfo_cancel(struct evdns_getaddrinfo_request *req); diff --git a/test/regress_dns.c b/test/regress_dns.c index 55f1158b..c69370ef 100644 --- a/test/regress_dns.c +++ b/test/regress_dns.c @@ -1543,6 +1543,136 @@ end: evdns_base_free(dns_base, 0); } +struct gaic_request_status { + int magic; + struct event_base *base; + struct evdns_base *dns_base; + struct evdns_getaddrinfo_request *request; + struct event cancel_event; + int canceled; +}; + +#define GAIC_MAGIC 0x1234abcd + +static int pending = 0; + +static void +gaic_cancel_request_cb(evutil_socket_t fd, short what, void *arg) +{ + struct gaic_request_status *status = arg; + + tt_assert(status->magic == GAIC_MAGIC); + status->canceled = 1; + evdns_getaddrinfo_cancel(status->request); + return; +end: + event_base_loopexit(status->base, NULL); +} + +static void +gaic_server_cb(struct evdns_server_request *req, void *arg) +{ + ev_uint32_t answer = 0x7f000001; + tt_assert(req->nquestions); + evdns_server_request_add_a_reply(req, req->questions[0]->name, 1, + &answer, 100); + evdns_server_request_respond(req, 0); + return; +end: + evdns_server_request_respond(req, DNS_ERR_REFUSED); +} + + +static void +gaic_getaddrinfo_cb(int result, struct evutil_addrinfo *res, void *arg) +{ + struct gaic_request_status *status = arg; + struct event_base *base = status->base; + tt_assert(status->magic == GAIC_MAGIC); + + if (result == EVUTIL_EAI_CANCEL) { + tt_assert(status->canceled); + } + event_del(&status->cancel_event); + + memset(status, 0xf0, sizeof(*status)); + free(status); + +end: + if (--pending <= 0) + event_base_loopexit(base, NULL); +} + +static void +gaic_launch(struct event_base *base, struct evdns_base *dns_base) +{ + struct gaic_request_status *status = calloc(1,sizeof(*status)); + struct timeval tv = { 0, 10000 }; + status->magic = GAIC_MAGIC; + status->base = base; + status->dns_base = dns_base; + event_assign(&status->cancel_event, base, -1, 0, gaic_cancel_request_cb, + status); + status->request = evdns_getaddrinfo(dns_base, + "foobar.bazquux.example.com", "80", NULL, gaic_getaddrinfo_cb, + status); + event_add(&status->cancel_event, &tv); + ++pending; +} + +static void +test_getaddrinfo_async_cancel_stress(void *arg) +{ + struct basic_test_data *data = arg; + struct event_base *base = data->base; + struct evdns_base *dns_base = NULL; + struct evdns_server_port *server = NULL; + evutil_socket_t fd = -1; + struct sockaddr_in sin; + struct sockaddr_storage ss; + ev_socklen_t slen; + int i; + + base = event_base_new(); + dns_base = evdns_base_new(base, 0); + + memset(&sin, 0, sizeof(sin)); + sin.sin_family = AF_INET; + sin.sin_port = 0; + sin.sin_addr.s_addr = htonl(0x7f000001); + if ((fd = socket(AF_INET, SOCK_DGRAM, 0)) < 0) { + tt_abort_perror("socket"); + } + evutil_make_socket_nonblocking(fd); + if (bind(fd, (struct sockaddr*)&sin, sizeof(sin))<0) { + tt_abort_perror("bind"); + } + server = evdns_add_server_port_with_base(base, fd, 0, gaic_server_cb, + base); + + memset(&ss, 0, sizeof(ss)); + slen = sizeof(ss); + if (getsockname(fd, (struct sockaddr*)&ss, &slen)<0) { + tt_abort_perror("getsockname"); + } + evdns_base_nameserver_sockaddr_add(dns_base, + (struct sockaddr*)&ss, slen, 0); + + for (i = 0; i < 1000; ++i) { + gaic_launch(base, dns_base); + } + + event_base_dispatch(base); + +end: + if (dns_base) + evdns_base_free(dns_base, 1); + if (server) + evdns_close_server_port(server); + if (fd >= 0) + evutil_closesocket(fd); +} + #define DNS_LEGACY(name, flags) \ { #name, run_legacy_test_fn, flags|TT_LEGACY, &legacy_setup, \ @@ -1565,6 +1695,8 @@ struct testcase_t dns_testcases[] = { { "getaddrinfo_async", test_getaddrinfo_async, TT_FORK|TT_NEED_BASE, &basic_setup, (char*)"" }, + { "getaddrinfo_cancel_stress", test_getaddrinfo_async_cancel_stress, + TT_FORK|TT_NEED_BASE, &basic_setup, (char*)"" }, END_OF_TESTCASES };