From c05f963719cab35697e645ad5aaaa72794fcccfc Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Thu, 21 Apr 2016 12:24:53 +0200 Subject: [PATCH 1/4] Fail on debugging detected errors --- .../330-event-loops-unit-tests.test | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/test/tpkg/330-event-loops-unit-tests.tpkg/330-event-loops-unit-tests.test b/src/test/tpkg/330-event-loops-unit-tests.tpkg/330-event-loops-unit-tests.test index e80948e0..c6539115 100644 --- a/src/test/tpkg/330-event-loops-unit-tests.tpkg/330-event-loops-unit-tests.test +++ b/src/test/tpkg/330-event-loops-unit-tests.tpkg/330-event-loops-unit-tests.test @@ -5,4 +5,10 @@ [ -f .tpkg.var.test ] && source .tpkg.var.test cd "${BUILDDIR}/build-event-loops" -make test +if make test +then + if grep ERROR "${BUILDDIR}/build-event-loops/src/test/*.log" + then + exit 1 + fi +fi From 0bd40268985b12ab0cb418e27f925034e95981e2 Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Thu, 21 Apr 2016 12:37:09 +0200 Subject: [PATCH 2/4] Detect freed memory usage with recursive queries Only when using unbound-event-api and doing queries for names in /etc/hosts --- .../tpkg/125-valgrind-checks.tpkg/125-valgrind-checks.test | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/tpkg/125-valgrind-checks.tpkg/125-valgrind-checks.test b/src/test/tpkg/125-valgrind-checks.tpkg/125-valgrind-checks.test index 60f216c7..0e7c4981 100644 --- a/src/test/tpkg/125-valgrind-checks.tpkg/125-valgrind-checks.test +++ b/src/test/tpkg/125-valgrind-checks.tpkg/125-valgrind-checks.test @@ -6,8 +6,12 @@ cat >queries < Date: Thu, 21 Apr 2016 15:16:38 +0200 Subject: [PATCH 3/4] Account for callbacks fired during scheduling --- src/dnssec.c | 2 +- src/general.c | 64 ++++++++++++++++++++++++++++++++---------- src/general.h | 4 ++- src/request-internal.c | 4 ++- src/types-internal.h | 6 ++++ 5 files changed, 62 insertions(+), 18 deletions(-) diff --git a/src/dnssec.c b/src/dnssec.c index 9bb95471..3e26f7b7 100644 --- a/src/dnssec.c +++ b/src/dnssec.c @@ -3181,7 +3181,7 @@ static void check_chain_complete(chain_head *chain) && !dnsreq->avoid_dnssec_roadblocks && dnsreq->netreqs[0]->dnssec_status == GETDNS_DNSSEC_BOGUS) { - getdns_return_t r = GETDNS_RETURN_GOOD; + int r = GETDNS_RETURN_GOOD; getdns_network_req **netreq_p, *netreq; dnsreq->avoid_dnssec_roadblocks = 1; diff --git a/src/general.c b/src/general.c index 4de98853..687fca2c 100644 --- a/src/general.c +++ b/src/general.c @@ -161,7 +161,7 @@ void _getdns_check_dns_req_complete(getdns_dns_req *dns_req) { getdns_network_req **netreq_p, *netreq; - int results_found = 0; + int results_found = 0, r; for (netreq_p = dns_req->netreqs; (netreq = *netreq_p); netreq_p++) if (netreq->state != NET_REQ_FINISHED && @@ -198,8 +198,11 @@ _getdns_check_dns_req_complete(getdns_dns_req *dns_req) ; (netreq = *netreq_p) ; netreq_p++ ) { _getdns_netreq_reinit(netreq); - if (_getdns_submit_netreq(netreq)) + if ((r = _getdns_submit_netreq(netreq))) { + if (r == DNS_REQ_FINISHED) + return; netreq->state = NET_REQ_FINISHED; + } } _getdns_check_dns_req_complete(dns_req); return; @@ -233,8 +236,11 @@ _getdns_check_dns_req_complete(getdns_dns_req *dns_req) ; (netreq = *netreq_p) ; netreq_p++ ) { _getdns_netreq_reinit(netreq); - if (_getdns_submit_netreq(netreq)) + if ((r = _getdns_submit_netreq(netreq))) { + if (r == DNS_REQ_FINISHED) + return; netreq->state = NET_REQ_FINISHED; + } } _getdns_check_dns_req_complete(dns_req); return; @@ -312,13 +318,16 @@ ub_resolve_callback(void* arg, int err, struct ub_result* ub_res) #endif -getdns_return_t +int _getdns_submit_netreq(getdns_network_req *netreq) { getdns_return_t r; getdns_dns_req *dns_req = netreq->owner; char name[1024]; - + int dnsreq_freed = 0; +#ifdef HAVE_LIBUNBOUND + int ub_resolve_r; +#endif #ifdef STUB_NATIVE_DNSSEC # ifdef DNSSEC_ROADBLOCK_AVOIDANCE @@ -351,24 +360,34 @@ _getdns_submit_netreq(getdns_network_req *netreq) dns_req->name_len, name, sizeof(name)); #ifdef HAVE_LIBUNBOUND + dns_req->freed = &dnsreq_freed; #ifdef HAVE_UNBOUND_EVENT_API if (_getdns_ub_loop_enabled(&dns_req->context->ub_loop)) - return ub_resolve_event(dns_req->context->unbound_ctx, + ub_resolve_r = ub_resolve_event(dns_req->context->unbound_ctx, name, netreq->request_type, netreq->owner->request_class, netreq, ub_resolve_event_callback, &(netreq->unbound_id)) ? GETDNS_RETURN_GENERIC_ERROR : GETDNS_RETURN_GOOD; else #endif - return ub_resolve_async(dns_req->context->unbound_ctx, + ub_resolve_r = ub_resolve_async(dns_req->context->unbound_ctx, name, netreq->request_type, netreq->owner->request_class, netreq, ub_resolve_callback, &(netreq->unbound_id)) ? GETDNS_RETURN_GENERIC_ERROR : GETDNS_RETURN_GOOD; + if (dnsreq_freed) + return DNS_REQ_FINISHED; + dns_req->freed = NULL; + return ub_resolve_r ? GETDNS_RETURN_GENERIC_ERROR : GETDNS_RETURN_GOOD; #else return GETDNS_RETURN_NOT_IMPLEMENTED; #endif } /* Submit with stub resolver */ - return _getdns_submit_stub_request(netreq); + dns_req->freed = &dnsreq_freed; + r = _getdns_submit_stub_request(netreq); + if (dnsreq_freed) + return DNS_REQ_FINISHED; + dns_req->freed = NULL; + return r; } @@ -456,7 +475,7 @@ getdns_general_ns(getdns_context *context, getdns_eventloop *loop, void *userarg, getdns_network_req **return_netreq_p, getdns_callback_t callbackfn, internal_cb_t internal_cb, int usenamespaces) { - getdns_return_t r = GETDNS_RETURN_GOOD; + int r = GETDNS_RETURN_GOOD; getdns_network_req *netreq, **netreq_p; getdns_dns_req *req; getdns_dict *localnames_response; @@ -495,8 +514,16 @@ getdns_general_ns(getdns_context *context, getdns_eventloop *loop, /* issue all network requests */ for ( netreq_p = req->netreqs ; !r && (netreq = *netreq_p) - ; netreq_p++) - r = _getdns_submit_netreq(netreq); + ; netreq_p++) { + if ((r = _getdns_submit_netreq(netreq))) { + if (r == DNS_REQ_FINISHED) { + if (return_netreq_p) + *return_netreq_p = NULL; + return GETDNS_RETURN_GOOD; + } + netreq->state = NET_REQ_FINISHED; + } + } else for (i = 0; i < context->namespace_count; i++) { if (context->namespaces[i] == GETDNS_NAMESPACE_LOCALNAMES) { @@ -517,14 +544,21 @@ getdns_general_ns(getdns_context *context, getdns_eventloop *loop, r = GETDNS_RETURN_GOOD; for ( netreq_p = req->netreqs ; !r && (netreq = *netreq_p) - ; netreq_p++) - r = _getdns_submit_netreq(netreq); + ; netreq_p++) { + if ((r = _getdns_submit_netreq(netreq))) { + if (r == DNS_REQ_FINISHED) { + if (return_netreq_p) + *return_netreq_p = NULL; + return GETDNS_RETURN_GOOD; + } + netreq->state = NET_REQ_FINISHED; + } + } break; } else r = GETDNS_RETURN_BAD_CONTEXT; } - - if (r != 0) { + if (r > 0) { /* i.e. r != GETDNS_RETURN_GOOD && r != DNS_REQ_FINISHED */ /* clean up the request */ _getdns_context_clear_outbound_request(req); _getdns_dns_req_free(req); diff --git a/src/general.h b/src/general.h index e9a4529e..29b52f47 100644 --- a/src/general.h +++ b/src/general.h @@ -42,9 +42,11 @@ /* private inner helper used by sync and async */ +#define DNS_REQ_FINISHED -1 + void _getdns_call_user_callback(getdns_dns_req *, getdns_dict *); void _getdns_check_dns_req_complete(getdns_dns_req *dns_req); -getdns_return_t _getdns_submit_netreq(getdns_network_req *netreq); +int _getdns_submit_netreq(getdns_network_req *netreq); getdns_return_t diff --git a/src/request-internal.c b/src/request-internal.c index f619429c..b209a971 100644 --- a/src/request-internal.c +++ b/src/request-internal.c @@ -640,7 +640,8 @@ _getdns_dns_req_free(getdns_dns_req * req) req->loop->vmt->clear(req->loop, &req->timeout); req->timeout.timeout_cb = NULL; } - + if (req->freed) + *req->freed = 1; GETDNS_FREE(req->my_mf, req); } @@ -901,6 +902,7 @@ _getdns_dns_req_new(getdns_context *context, getdns_eventloop *loop, result->upstreams->referenced++; result->finished_next = NULL; + result->freed = NULL; network_req_init(result->netreqs[0], result, request_type, dnssec_extension_set, with_opt, diff --git a/src/types-internal.h b/src/types-internal.h index 9e9a4d34..b70b59f6 100644 --- a/src/types-internal.h +++ b/src/types-internal.h @@ -304,6 +304,12 @@ typedef struct getdns_dns_req { int dnssec_ok_checking_disabled; int is_sync_request; + /* Integer pointed to by pointer will be set to 1 (if set), + * before the request is freed. + * To be used by _getdns_submit_netreq only! + */ + int *freed; + /* internally scheduled request */ internal_cb_t internal_cb; From d61e64c9c7d32cee087f5e7c0cc1f6566e78292d Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Fri, 22 Apr 2016 14:09:18 +0200 Subject: [PATCH 4/4] Fix callbacks during scheduling in DNSSEC code too --- src/dnssec.c | 34 ++++++++++++++++++++++++++-------- src/request-internal.c | 1 + src/types-internal.h | 11 ++++++++--- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/dnssec.c b/src/dnssec.c index 3e26f7b7..7674636c 100644 --- a/src/dnssec.c +++ b/src/dnssec.c @@ -736,6 +736,8 @@ typedef struct chain_node chain_node; struct chain_head { struct mem_funcs my_mf; + size_t lock; + chain_head *next; chain_node *parent; size_t node_count; /* Number of nodes attached directly @@ -857,6 +859,7 @@ static chain_head *add_rrset2val_chain(struct mem_funcs *mf, head = *chain_p = (chain_head *)region; head->my_mf = *mf; + head->lock = 1; head->next = NULL; head->rrset.name = head->name_spc; memcpy(head->name_spc, rrset->name, dname_len); @@ -1319,6 +1322,7 @@ static void val_chain_node_cb(getdns_dns_req *dnsreq) default : check_chain_complete(node->chains); return; } + node->lock++; n_signers = 0; for ( i = rrset_iter_init(&i_spc,netreq->response,netreq->response_len) ; i @@ -1344,6 +1348,7 @@ static void val_chain_node_cb(getdns_dns_req *dnsreq) */ val_chain_sched_soa_node(node->parent); + node->lock--; check_chain_complete(node->chains); } @@ -1363,17 +1368,21 @@ static void val_chain_node_soa_cb(getdns_dns_req *dnsreq) ! _dname_equal(node->ds.name, rrset->name)) node = node->parent; - if (node) + if (node) { + node->lock++; val_chain_sched_ds_node(node); - else { + } else { /* SOA for a different name */ node = (chain_node *)dnsreq->user_pointer; + node->lock++; val_chain_sched_soa_node(node->parent); } - } else if (node->parent) + } else if (node->parent) { + node->lock++; val_chain_sched_soa_node(node->parent); - + } + node->lock--; check_chain_complete(node->chains); } @@ -3003,7 +3012,7 @@ static size_t count_outstanding_requests(chain_head *head) if (!head) return 0; - for ( node = head->parent, count = 0 + for ( node = head->parent, count = head->lock ; node ; node = node->parent) { @@ -3245,6 +3254,7 @@ static void check_chain_complete(chain_head *chain) } /* Final user callback */ + dnsreq->validating = 0; _getdns_call_user_callback(dnsreq, response_dict); } @@ -3252,7 +3262,11 @@ static void check_chain_complete(chain_head *chain) void _getdns_get_validation_chain(getdns_dns_req *dnsreq) { getdns_network_req *netreq, **netreq_p; - chain_head *chain = NULL; + chain_head *chain = NULL, *chain_p; + + if (dnsreq->validating) + return; + dnsreq->validating = 1; for (netreq_p = dnsreq->netreqs; (netreq = *netreq_p) ; netreq_p++) { if (! netreq->response @@ -3277,11 +3291,15 @@ void _getdns_get_validation_chain(getdns_dns_req *dnsreq) , netreq ); } - if (chain) + if (chain) { + for (chain_p = chain; chain_p; chain_p = chain_p->next) + chain_p->lock--; check_chain_complete(chain); - else + } else { + dnsreq->validating = 0; _getdns_call_user_callback(dnsreq, _getdns_create_getdns_response(dnsreq)); + } } diff --git a/src/request-internal.c b/src/request-internal.c index b209a971..52f91d89 100644 --- a/src/request-internal.c +++ b/src/request-internal.c @@ -903,6 +903,7 @@ _getdns_dns_req_new(getdns_context *context, getdns_eventloop *loop, result->finished_next = NULL; result->freed = NULL; + result->validating = 0; network_req_init(result->netreqs[0], result, request_type, dnssec_extension_set, with_opt, diff --git a/src/types-internal.h b/src/types-internal.h index b70b59f6..1a49b8e5 100644 --- a/src/types-internal.h +++ b/src/types-internal.h @@ -304,10 +304,15 @@ typedef struct getdns_dns_req { int dnssec_ok_checking_disabled; int is_sync_request; - /* Integer pointed to by pointer will be set to 1 (if set), - * before the request is freed. - * To be used by _getdns_submit_netreq only! + /* The validating and freed variables are used to make sure a single + * code path is followed while processing a DNS request, even when + * callbacks are already fired whilst the registering/scheduling call + * (i.e. ub_resolve_event) has not returned yet. + * + * validating is touched by _getdns_get_validation_chain only and + * freed is touched by _getdns_submit_netreq only */ + int validating; int *freed; /* internally scheduled request */