From 74b1f77357beabca1ceb4dd12cc6e2f56c39f529 Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Sat, 18 Feb 2017 13:16:25 +0100 Subject: [PATCH] Cancel get validation chain getdns_dns_reqs And miscellaneous little other scheduling fixes and optimizations --- src/context.c | 238 ++++++++++++++++++++--------------------- src/context.h | 37 +++++-- src/dnssec.c | 39 +++---- src/general.c | 20 ++-- src/request-internal.c | 6 +- src/stub.c | 2 +- src/types-internal.h | 3 - 7 files changed, 168 insertions(+), 177 deletions(-) diff --git a/src/context.c b/src/context.c index 39f2dbb0..39334b82 100644 --- a/src/context.c +++ b/src/context.c @@ -135,8 +135,7 @@ static getdns_return_t create_default_namespaces(struct getdns_context *context) static getdns_return_t create_default_dns_transports(struct getdns_context *context); static int transaction_id_cmp(const void *, const void *); static void dispatch_updated(struct getdns_context *, uint16_t); -static void cancel_dns_req(getdns_dns_req *); -static void cancel_outstanding_requests(struct getdns_context*, int); +static void cancel_outstanding_requests(getdns_context*); /* unbound helpers */ #ifdef HAVE_LIBUNBOUND @@ -682,8 +681,7 @@ _getdns_upstreams_dereference(getdns_upstreams *upstreams) while (upstream->finished_dnsreqs) { dnsreq = upstream->finished_dnsreqs; upstream->finished_dnsreqs = dnsreq->finished_next; - (void) _getdns_context_cancel_request(dnsreq->context, - dnsreq->trans_id, 1); + _getdns_context_cancel_request(dnsreq); } if (upstream->tls_obj != NULL) { if (upstream->tls_session != NULL) @@ -1521,7 +1519,7 @@ getdns_context_destroy(struct getdns_context *context) context->destroying = 1; /* cancel all outstanding requests */ - cancel_outstanding_requests(context, 1); + cancel_outstanding_requests(context); /* Destroy listening addresses */ (void) getdns_context_set_listen_addresses(context, NULL, NULL, NULL); @@ -1705,7 +1703,7 @@ static getdns_return_t rebuild_ub_ctx(struct getdns_context* context) { if (context->unbound_ctx != NULL) { /* cancel all requests and delete */ - cancel_outstanding_requests(context, 1); + cancel_outstanding_requests(context); ub_ctx_delete(context->unbound_ctx); context->unbound_ctx = NULL; } @@ -2882,28 +2880,68 @@ getdns_context_set_memory_functions(struct getdns_context *context, context, MF_PLAIN, mf.ext.malloc, mf.ext.realloc, mf.ext.free); } /* getdns_context_set_memory_functions*/ -/* cancel the request */ -static void -cancel_dns_req(getdns_dns_req *req) +void +_getdns_context_track_outbound_request(getdns_dns_req *dnsreq) +{ + /* Called only by getdns_general_ns() after successful allocation */ + assert(dnsreq); + + dnsreq->node.key = &(dnsreq->trans_id); + if (_getdns_rbtree_insert( + &dnsreq->context->outbound_requests, &dnsreq->node)) + getdns_context_request_count_changed(dnsreq->context); +} + +void +_getdns_context_clear_outbound_request(getdns_dns_req *dnsreq) +{ + if (!dnsreq) return; + + if (dnsreq->loop && dnsreq->loop->vmt && dnsreq->timeout.timeout_cb) { + dnsreq->loop->vmt->clear(dnsreq->loop, &dnsreq->timeout); + dnsreq->timeout.timeout_cb = NULL; + } + /* delete the node from the tree */ + if (_getdns_rbtree_delete( + &dnsreq->context->outbound_requests, &dnsreq->trans_id)) + getdns_context_request_count_changed(dnsreq->context); + + if (dnsreq->chain) + _getdns_cancel_validation_chain(dnsreq); +} + +void +_getdns_context_cancel_request(getdns_dns_req *dnsreq) { getdns_network_req *netreq, **netreq_p; - for (netreq_p = req->netreqs; (netreq = *netreq_p); netreq_p++) + DEBUG_SCHED("%s(%p)\n", __FUNC__, (void *)dnsreq); + if (!dnsreq) return; + + _getdns_context_clear_outbound_request(dnsreq); + + /* cancel network requests */ + for (netreq_p = dnsreq->netreqs; (netreq = *netreq_p); netreq_p++) #ifdef HAVE_LIBUNBOUND if (netreq->unbound_id != -1) { - ub_cancel(req->context->unbound_ctx, + ub_cancel(dnsreq->context->unbound_ctx, netreq->unbound_id); netreq->unbound_id = -1; } else #endif _getdns_cancel_stub_request(netreq); - req->canceled = 1; + /* clean up */ + _getdns_dns_req_free(dnsreq); } +/* + * getdns_cancel_callback + * + */ getdns_return_t -_getdns_context_cancel_request(getdns_context *context, - getdns_transaction_t transaction_id, int fire_callback) +getdns_cancel_callback(getdns_context *context, + getdns_transaction_t transaction_id) { getdns_dns_req *dnsreq; @@ -2915,40 +2953,69 @@ _getdns_context_cancel_request(getdns_context *context, &context->outbound_requests, &transaction_id))) return GETDNS_RETURN_UNKNOWN_TRANSACTION; - if (dnsreq->chain) - _getdns_cancel_validation_chain(dnsreq); - - /* do the cancel */ - cancel_dns_req(dnsreq); - - if (fire_callback && dnsreq->user_callback) { - context->processing = 1; - dnsreq->user_callback(context, GETDNS_CALLBACK_CANCEL, - NULL, dnsreq->user_pointer, transaction_id); - context->processing = 0; - } - - /* clean up */ - _getdns_dns_req_free(dnsreq); - return GETDNS_RETURN_GOOD; -} - -/* - * getdns_cancel_callback - * - */ -getdns_return_t -getdns_cancel_callback(getdns_context *context, - getdns_transaction_t transaction_id) -{ - if (!context) - return GETDNS_RETURN_INVALID_PARAMETER; - - getdns_return_t r = _getdns_context_cancel_request(context, transaction_id, 1); getdns_context_request_count_changed(context); - return r; + + if (dnsreq->user_callback) { + dnsreq->context->processing = 1; + dnsreq->user_callback(dnsreq->context, GETDNS_CALLBACK_CANCEL, + NULL, dnsreq->user_pointer, dnsreq->trans_id); + dnsreq->context->processing = 0; + } + _getdns_context_cancel_request(dnsreq); + return GETDNS_RETURN_GOOD; } /* getdns_cancel_callback */ +void +_getdns_context_request_timed_out(getdns_dns_req *dnsreq) +{ + DEBUG_SCHED("%s(%p)\n", __FUNC__, (void *)dnsreq); + + if (dnsreq->user_callback) { + dnsreq->context->processing = 1; + dnsreq->user_callback(dnsreq->context, GETDNS_CALLBACK_TIMEOUT, + _getdns_create_getdns_response(dnsreq), + dnsreq->user_pointer, dnsreq->trans_id); + dnsreq->context->processing = 0; + } + _getdns_context_cancel_request(dnsreq); +} + +static void +accumulate_outstanding_transactions(_getdns_rbnode_t *node, void* arg) +{ + *(*(getdns_dns_req ***)arg)++ = (getdns_dns_req *)node; +} + +static void +cancel_outstanding_requests(getdns_context* context) +{ + getdns_dns_req **dnsreqs, **dnsreq_a, **dnsreq_i; + + if (context->outbound_requests.count == 0) + return; + + dnsreq_i = dnsreq_a = dnsreqs = GETDNS_XMALLOC(context->my_mf, + getdns_dns_req *, context->outbound_requests.count); + + _getdns_traverse_postorder(&context->outbound_requests, + accumulate_outstanding_transactions, &dnsreq_a); + + while (dnsreq_i < dnsreq_a) { + getdns_dns_req *dnsreq = *dnsreq_i; + + if (dnsreq->user_callback) { + dnsreq->context->processing = 1; + dnsreq->user_callback(dnsreq->context, + GETDNS_CALLBACK_CANCEL, NULL, + dnsreq->user_pointer, dnsreq->trans_id); + dnsreq->context->processing = 0; + } + _getdns_context_cancel_request(dnsreq); + + dnsreq_i += 1; + } + GETDNS_FREE(context->my_mf, dnsreqs); +} #ifndef STUB_NATIVE_DNSSEC @@ -3234,56 +3301,6 @@ _getdns_context_prepare_for_resolution(struct getdns_context *context, return r; } /* _getdns_context_prepare_for_resolution */ -getdns_return_t -_getdns_context_track_outbound_request(getdns_dns_req *dnsreq) -{ - if (!dnsreq) - return GETDNS_RETURN_INVALID_PARAMETER; - - dnsreq->node.key = &(dnsreq->trans_id); - if (!_getdns_rbtree_insert( - &dnsreq->context->outbound_requests, &dnsreq->node)) - return GETDNS_RETURN_GENERIC_ERROR; - - getdns_context_request_count_changed(dnsreq->context); - return GETDNS_RETURN_GOOD; -} - -getdns_return_t -_getdns_context_clear_outbound_request(getdns_dns_req *dnsreq) -{ - if (!dnsreq) - return GETDNS_RETURN_INVALID_PARAMETER; - - if (!_getdns_rbtree_delete( - &dnsreq->context->outbound_requests, &dnsreq->trans_id)) - return GETDNS_RETURN_GENERIC_ERROR; - - getdns_context_request_count_changed(dnsreq->context); - return GETDNS_RETURN_GOOD; -} - -getdns_return_t -_getdns_context_request_timed_out(getdns_dns_req *req) -{ - /* Don't use req after callback */ - getdns_context* context = req->context; - getdns_transaction_t trans_id = req->trans_id; - getdns_callback_t cb = req->user_callback; - void *user_arg = req->user_pointer; - getdns_dict *response = _getdns_create_getdns_response(req); - - /* cancel the req - also clears it from outbound and cleans up*/ - _getdns_context_cancel_request(context, trans_id, 0); - if (cb) { - context->processing = 1; - cb(context, GETDNS_CALLBACK_TIMEOUT, response, user_arg, trans_id); - context->processing = 0; - } - getdns_context_request_count_changed(context); - return GETDNS_RETURN_GOOD; -} - char * _getdns_strdup(const struct mem_funcs *mfs, const char *s) { @@ -3365,33 +3382,6 @@ getdns_context_run(getdns_context *context) context->extension->vmt->run(context->extension); } -typedef struct timeout_accumulator { - getdns_transaction_t* ids; - int idx; -} timeout_accumulator; - -static void -accumulate_outstanding_transactions(_getdns_rbnode_t* node, void* arg) { - timeout_accumulator* acc = (timeout_accumulator*) arg; - acc->ids[acc->idx] = *((getdns_transaction_t*) node->key); - acc->idx++; -} - -static void -cancel_outstanding_requests(struct getdns_context* context, int fire_callback) { - if (context->outbound_requests.count > 0) { - timeout_accumulator acc; - int i; - acc.idx = 0; - acc.ids = GETDNS_XMALLOC(context->my_mf, getdns_transaction_t, context->outbound_requests.count); - _getdns_traverse_postorder(&context->outbound_requests, accumulate_outstanding_transactions, &acc); - for (i = 0; i < acc.idx; ++i) { - _getdns_context_cancel_request(context, acc.ids[i], fire_callback); - } - GETDNS_FREE(context->my_mf, acc.ids); - } -} - getdns_return_t getdns_context_detach_eventloop(struct getdns_context* context) { @@ -3405,7 +3395,7 @@ getdns_context_detach_eventloop(struct getdns_context* context) * and they may destroy the context ) */ /* cancel all outstanding requests */ - cancel_outstanding_requests(context, 1); + cancel_outstanding_requests(context); context->extension->vmt->cleanup(context->extension); context->extension = &context->default_eventloop.loop; _getdns_default_eventloop_init(&context->mf, &context->default_eventloop); @@ -3423,7 +3413,7 @@ getdns_context_set_eventloop(getdns_context* context, getdns_eventloop* loop) return GETDNS_RETURN_INVALID_PARAMETER; if (context->extension) { - cancel_outstanding_requests(context, 1); + cancel_outstanding_requests(context); context->extension->vmt->cleanup(context->extension); } context->extension = loop; diff --git a/src/context.h b/src/context.h index b89a5876..e6ed49fb 100644 --- a/src/context.h +++ b/src/context.h @@ -349,19 +349,34 @@ struct getdns_context { getdns_return_t _getdns_context_prepare_for_resolution(struct getdns_context *context, int usenamespaces); -/* track an outbound request */ -getdns_return_t _getdns_context_track_outbound_request(struct getdns_dns_req - *req); -/* clear the outbound request from being tracked - does not cancel it */ -getdns_return_t _getdns_context_clear_outbound_request(struct getdns_dns_req - *req); +/* Register a getdns_dns_req with context. + * - Without pluggable unbound event API, + * ub_fd() is scheduled when this was the first request. + */ +void _getdns_context_track_outbound_request(getdns_dns_req *dnsreq); -getdns_return_t _getdns_context_request_timed_out(struct getdns_dns_req - *req); +/* Deregister getdns_dns_req from the context. + * - Without pluggable unbound event API, + * ub_fd() is scheduled when this was the first request. + * - Potential timeout events will be cleared. + * - All associated getdns_dns_reqs (to get the validation chain) + * will be canceled. + */ +void _getdns_context_clear_outbound_request(getdns_dns_req *dnsreq); -/* cancel callback internal - flag to indicate if req should be freed and callback fired */ -getdns_return_t _getdns_context_cancel_request(struct getdns_context *context, - getdns_transaction_t transaction_id, int fire_callback); +/* Cancels and frees a getdns_dns_req (without calling user callbacks) + * - Deregisters getdns_dns_req with _getdns_context_clear_outbound_request() + * - Cancels associated getdns_network_reqs + * (by calling ub_cancel() or _getdns_cancel_stub_request()) + * - Frees the getdns_dns_req + */ +void _getdns_context_cancel_request(getdns_dns_req *dnsreq); + + +/* Calls user callback (with GETDNS_CALLBACK_TIMEOUT + response dict), then + * cancels and frees the getdns_dns_req with _getdns_context_cancel_request() + */ +void _getdns_context_request_timed_out(getdns_dns_req *dnsreq); char *_getdns_strdup(const struct mem_funcs *mfs, const char *str); diff --git a/src/dnssec.c b/src/dnssec.c index 905f778d..0396f045 100644 --- a/src/dnssec.c +++ b/src/dnssec.c @@ -1044,7 +1044,6 @@ static void val_chain_node_cb(getdns_dns_req *dnsreq) _getdns_rrsig_iter *rrsig, rrsig_spc; size_t n_signers; - _getdns_context_clear_outbound_request(dnsreq); switch (netreq->request_type) { case GETDNS_RRTYPE_DS : node->ds.pkt = netreq->response; node->ds.pkt_len = netreq->response_len; @@ -1094,7 +1093,6 @@ static void val_chain_node_soa_cb(getdns_dns_req *dnsreq) _getdns_rrset_iter i_spc, *i; _getdns_rrset *rrset; - _getdns_context_clear_outbound_request(dnsreq); /* A SOA query is always scheduled with a node as the user argument. */ assert(node != NULL); @@ -1121,8 +1119,10 @@ static void val_chain_node_soa_cb(getdns_dns_req *dnsreq) } else { /* SOA for a different name */ node = (chain_node *)dnsreq->user_pointer; - node->lock++; - val_chain_sched_soa_node(node->parent); + if (node->parent) { + node->lock++; + val_chain_sched_soa_node(node->parent); + } } } else if (node->parent) { @@ -3049,7 +3049,10 @@ static void check_chain_complete(chain_head *chain) val_chain_list = dnsreq->dnssec_return_validation_chain ? getdns_list_create_with_context(context) : NULL; - /* Walk chain to add values to val_chain_list and to cleanup */ + /* Walk chain to add values to val_chain_list. We do not cleanup yet. + * The chain will eventually be freed when the dns request is descheduled + * with getdns_context_clear_outbound_request(). + */ for ( head = chain; head ; head = next ) { next = head->next; if (dnsreq->dnssec_return_full_validation_chain && @@ -3076,7 +3079,6 @@ static void check_chain_complete(chain_head *chain) context, val_chain_list, node->dnskey_req, node->dnskey_signer); - _getdns_dns_req_free(node->dnskey_req->owner); } if (node->ds_req) { if (val_chain_list) @@ -3094,13 +3096,8 @@ static void check_chain_complete(chain_head *chain) context, val_chain_list, &node->ds); } - _getdns_dns_req_free(node->ds_req->owner); - } - if (node->soa_req) { - _getdns_dns_req_free(node->soa_req->owner); } } - GETDNS_FREE(head->my_mf, head); } response_dict = _getdns_create_getdns_response(dnsreq); @@ -3117,11 +3114,12 @@ static void check_chain_complete(chain_head *chain) void _getdns_cancel_validation_chain(getdns_dns_req *dnsreq) { - chain_head *head, *next; + chain_head *head = dnsreq->chain, *next; chain_node *node; size_t node_count; - for ( head = dnsreq->chain; head ; head = next ) { + dnsreq->chain = NULL; + while (head) { next = head->next; for ( node_count = head->node_count, node = head->parent @@ -3130,22 +3128,19 @@ void _getdns_cancel_validation_chain(getdns_dns_req *dnsreq) if (node->dnskey_req) _getdns_context_cancel_request( - node->dnskey_req->owner->context, - node->dnskey_req->owner->trans_id, 0); + node->dnskey_req->owner); if (node->ds_req) _getdns_context_cancel_request( - node->ds_req->owner->context, - node->ds_req->owner->trans_id, 0); - - if (node->soa_req) + node->ds_req->owner); + + if (node->soa_req) _getdns_context_cancel_request( - node->soa_req->owner->context, - node->soa_req->owner->trans_id, 0); + node->soa_req->owner); } GETDNS_FREE(head->my_mf, head); + head = next; } - dnsreq->chain = NULL; } void _getdns_get_validation_chain(getdns_dns_req *dnsreq) diff --git a/src/general.c b/src/general.c index 97a97dcf..b31e2405 100644 --- a/src/general.c +++ b/src/general.c @@ -54,14 +54,6 @@ #include "dict.h" #include "mdns.h" -/* cancel, cleanup and send timeout to callback */ -static void -ub_resolve_timeout(void *arg) -{ - getdns_dns_req *dns_req = (getdns_dns_req *) arg; - (void) _getdns_context_request_timed_out(dns_req); -} - void _getdns_call_user_callback(getdns_dns_req *dns_req, struct getdns_dict *response) { @@ -72,13 +64,14 @@ void _getdns_call_user_callback(getdns_dns_req *dns_req, /* clean up */ _getdns_context_clear_outbound_request(dns_req); - _getdns_dns_req_free(dns_req); context->processing = 1; cb(context, (response ? GETDNS_CALLBACK_COMPLETE : GETDNS_CALLBACK_ERROR), response, user_arg, trans_id); context->processing = 0; + + _getdns_dns_req_free(dns_req); } static int @@ -186,9 +179,10 @@ _getdns_check_dns_req_complete(getdns_dns_req *dns_req) return; } } - if (dns_req->internal_cb) + if (dns_req->internal_cb) { + _getdns_context_clear_outbound_request(dns_req); dns_req->internal_cb(dns_req); - else if (! results_found) + } else if (! results_found) _getdns_call_user_callback(dns_req, NULL); else if (dns_req->dnssec_return_validation_chain #ifdef DNSSEC_ROADBLOCK_AVOIDANCE @@ -290,7 +284,9 @@ _getdns_submit_netreq(getdns_network_req *netreq) dns_req->timeout.userarg = dns_req; dns_req->timeout.read_cb = NULL; dns_req->timeout.write_cb = NULL; - dns_req->timeout.timeout_cb = ub_resolve_timeout; + dns_req->timeout.timeout_cb = + (getdns_eventloop_callback) + _getdns_context_request_timed_out; dns_req->timeout.ev = NULL; if ((r = dns_req->loop->vmt->schedule(dns_req->loop, -1, dns_req->context->timeout, &dns_req->timeout))) diff --git a/src/request-internal.c b/src/request-internal.c index 3f180d56..eae467e9 100644 --- a/src/request-internal.c +++ b/src/request-internal.c @@ -644,7 +644,7 @@ _getdns_dns_req_free(getdns_dns_req * req) network_req_cleanup(*net_req); /* clear timeout event */ - if (req->timeout.timeout_cb) { + if (req->loop && req->loop->vmt && req->timeout.timeout_cb) { req->loop->vmt->clear(req->loop, &req->timeout); req->timeout.timeout_cb = NULL; } @@ -896,9 +896,7 @@ _getdns_dns_req_new(getdns_context *context, getdns_eventloop *loop, } result->context = context; result->loop = loop; - result->canceled = 0; - result->trans_id = (((uint64_t)arc4random()) << 32) | - ((uint64_t)arc4random()); + result->trans_id = (uint64_t) (intptr_t) result; result->dnssec_return_status = dnssec_return_status; result->dnssec_return_only_secure = dnssec_return_only_secure; result->dnssec_return_all_statuses = dnssec_return_all_statuses; diff --git a/src/stub.c b/src/stub.c index 9421f5d8..a377228f 100644 --- a/src/stub.c +++ b/src/stub.c @@ -602,7 +602,7 @@ stub_timeout_cb(void *userarg) if (netreq->owner->user_callback) { netreq->debug_end_time = _getdns_get_time_as_uintt64(); /* Note this calls cancel_request which calls stub_cleanup again....!*/ - (void) _getdns_context_request_timed_out(netreq->owner); + _getdns_context_request_timed_out(netreq->owner); } else _getdns_check_dns_req_complete(netreq->owner); } diff --git a/src/types-internal.h b/src/types-internal.h index 137892f1..78f1935a 100644 --- a/src/types-internal.h +++ b/src/types-internal.h @@ -290,9 +290,6 @@ typedef struct getdns_dns_req { size_t suffix_len; unsigned suffix_appended : 1; - /* canceled flag */ - unsigned canceled : 1; - /* request extensions */ unsigned dnssec_return_status : 1; unsigned dnssec_return_only_secure : 1;