Merge branch 'devel/scheduling_bug_detection' into features/canonical_dnssec_chain

src/dnssec.c

@@ -719,6 +719,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
@@ -840,6 +842,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);
@@ -1302,6 +1305,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
@@ -1327,6 +1331,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);
 }
 
@@ -1346,17 +1351,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);
 }
 
@@ -2972,7 +2981,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) {
 
@@ -3221,7 +3230,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;
@@ -3302,6 +3311,7 @@ static void check_chain_complete(chain_head *chain)
 	}
 
 	/* Final user callback */
+	dnsreq->validating = 0;
 	_getdns_call_user_callback(dnsreq, response_dict);
 }
 
@@ -3309,7 +3319,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
@@ -3334,11 +3348,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));
+	}
 }
 
 

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;
 }
 
 
@@ -457,7 +476,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;
@@ -496,8 +515,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++)
+		    ; netreq_p++) {
-			r = _getdns_submit_netreq(netreq);
+			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) {
@@ -518,14 +545,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++)
+			    ; netreq_p++) {
-				r = _getdns_submit_netreq(netreq);
+				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) { /* i.e. r != GETDNS_RETURN_GOOD && r != DNS_REQ_FINISHED */
-	if (r != 0) {
 		/* clean up the request */
 		_getdns_context_clear_outbound_request(req);
 		_getdns_dns_req_free(req);

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

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);
 }
 
@@ -907,6 +908,8 @@ _getdns_dns_req_new(getdns_context *context, getdns_eventloop *loop,
 		result->upstreams->referenced++;
 
 	result->finished_next = NULL;
+	result->freed = NULL;
+	result->validating = 0;
 
 	network_req_init(result->netreqs[0], result,
 	    request_type, dnssec_extension_set, with_opt,

src/test/tpkg/125-valgrind-checks.tpkg/125-valgrind-checks.test

@@ -6,8 +6,12 @@
 
 cat >queries <<EOT
 NS .
+localhost
+localhost.
+A localhost.
 -A getdnsapi.net
 qwerlkjhasdfpuiqwyerm.1234kjhrqwersv.com
+localhost.
 -G TXT bogus.nlnetlabs.nl
 -H 8.8.8.8
 -H 2a04:b900:0:100::37

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

src/types-internal.h

@@ -306,6 +306,17 @@ typedef struct getdns_dns_req {
 
 	uint16_t tls_query_padding_blocksize;
 
+	/* 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 */
 	internal_cb_t internal_cb;