Fix the thread safety in the dns_dispatch unit

The dispatches are not thread-bound, and used freely between various
threads (see the dns_resolver and dns_request units for details).

This refactoring make sure that all non-const dns_dispatch_t and
dns_dispentry_t members are accessed under a lock, and both object now
track their internal state (NONE, CONNECTING, CONNECTED, CANCELED)
instead of guessing the state from the state of various struct members.

During the refactoring, the artificial limit DNS_DISPATCH_SOCKSQUOTA on
UDP sockets per dispatch was removed as the limiting needs to happen and
happens on in dns_resolver and limiting the number of UDP sockets
artificially in dispatch could lead to unpredictable behaviour in case
one dispatch has the limit exhausted by others are idle.

The TCP artificial limit of DNS_DISPATCH_MAXREQUESTS makes even less
sense as the TCP connections are only reused in the dns_request API
that's not a heavy user of the outgoing connections.

As a side note, the fact that UDP and TCP dispatch pretends to be same
thing, but in fact the connected UDP is handled from dns_dispentry_t and
dns_dispatch_t acts as a broker, but connected TCP is handled from
dns_dispatch_t and dns_dispatchmgr_t acts as a broker doesn't really
help the clarity of this unit.

This refactoring kept to API almost same - only dns_dispatch_cancel()
and dns_dispatch_done() were merged into dns_dispatch_done() as we need
to cancel active netmgr handles in any case to not leave dangling
connections around.  The functions handling UDP and TCP have been mostly
split to their matching counterparts and the dns_dispatch_<function>
functions are now thing wrappers that call <udp|tcp>_dispatch_<function>
based on the socket type.

More debugging-level logging was added to the unit to accomodate for
this fact.

bin/tests/system/cookie/tests.sh

@@ -221,7 +221,7 @@ nextpart ns8/named.run > /dev/null
 $DIG $DIGOPTS +cookie soa from-no-cookie-server.example @10.53.0.8 > dig.out.test$n
 grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
 wait_for_log_peek 3 "missing required cookie from 10.53.0.7#" ns8/named.run || ret=1
-wait_for_log_peek 3 "TCP connected" ns8/named.run || ret=1
+wait_for_log_peek 3 "connected from" ns8/named.run || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 

lib/dns/include/dns/dispatch.h

@@ -53,10 +53,13 @@
 #include <isc/lang.h>
 #include <isc/mutex.h>
 #include <isc/netmgr.h>
+#include <isc/refcount.h>
 #include <isc/types.h>
 
 #include <dns/types.h>
 
+#undef DNS_DISPATCH_TRACE
+
 ISC_LANG_BEGINDECLS
 
 /*%
@@ -94,25 +97,22 @@ dns_dispatchmgr_create(isc_mem_t *mctx, isc_nm_t *nm, dns_dispatchmgr_t **mgrp);
  *\li	anything else	-- failure
  */
 
-void
-dns_dispatchmgr_attach(dns_dispatchmgr_t *mgr, dns_dispatchmgr_t **mgrp);
-/*%<
- * Attach to a dispatch manger.
- *
- * Requires:
- *\li	 is valid.
- *
- *\li	mgrp != NULL && *mgrp == NULL
- */
+#if DNS_DISPATCH_TRACE
+#define dns_dispatchmgr_ref(ptr) \
+	dns_dispatchmgr__ref(ptr, __func__, __FILE__, __LINE__)
+#define dns_dispatchmgr_unref(ptr) \
+	dns_dispatchmgr__unref(ptr, __func__, __FILE__, __LINE__)
+#define dns_dispatchmgr_attach(ptr, ptrp) \
+	dns_dispatchmgr__attach(ptr, ptrp, __func__, __FILE__, __LINE__)
+#define dns_dispatchmgr_detach(ptrp) \
+	dns_dispatchmgr__detach(ptrp, __func__, __FILE__, __LINE__)
+ISC_REFCOUNT_TRACE_DECL(dns_dispatchmgr);
+#else
+ISC_REFCOUNT_DECL(dns_dispatchmgr);
+#endif
 
-void
-dns_dispatchmgr_detach(dns_dispatchmgr_t **mgrp);
 /*%<
- * Detach from the dispatch manager, and destroy it if no references
- * remain.
- *
- * Requires:
- *\li	mgrp != NULL && *mgrp is a valid dispatchmgr.
+ * Attach/Detach to a dispatch manager.
  */
 
 void
@@ -201,10 +201,21 @@ dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *localaddr,
  *\li	Anything else	-- failure.
  */
 
-void
-dns_dispatch_attach(dns_dispatch_t *disp, dns_dispatch_t **dispp);
+#if DNS_DISPATCH_TRACE
+#define dns_dispatch_ref(ptr) \
+	dns_dispatch__ref(ptr, __func__, __FILE__, __LINE__)
+#define dns_dispatch_unref(ptr) \
+	dns_dispatch__unref(ptr, __func__, __FILE__, __LINE__)
+#define dns_dispatch_attach(ptr, ptrp) \
+	dns_dispatch__attach(ptr, ptrp, __func__, __FILE__, __LINE__)
+#define dns_dispatch_detach(ptrp) \
+	dns_dispatch__detach(ptrp, __func__, __FILE__, __LINE__)
+ISC_REFCOUNT_TRACE_DECL(dns_dispatch);
+#else
+ISC_REFCOUNT_DECL(dns_dispatch);
+#endif
 /*%<
- * Attach to a dispatch handle.
+ * Attach/Detach to a dispatch handle.
  *
  * Requires:
  *\li	disp is valid.
@@ -212,15 +223,6 @@ dns_dispatch_attach(dns_dispatch_t *disp, dns_dispatch_t **dispp);
  *\li	dispp != NULL && *dispp == NULL
  */
 
-void
-dns_dispatch_detach(dns_dispatch_t **dispp);
-/*%<
- * Detaches from the dispatch.
- *
- * Requires:
- *\li	dispp != NULL and *dispp be a valid dispatch.
- */
-
 isc_result_t
 dns_dispatch_connect(dns_dispentry_t *resp);
 /*%<
@@ -253,11 +255,9 @@ dns_dispatch_resume(dns_dispentry_t *resp, uint16_t timeout);
 
 isc_result_t
 dns_dispatch_gettcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *destaddr,
-		    const isc_sockaddr_t *localaddr, bool *connected,
-		    dns_dispatch_t **dispp);
+		    const isc_sockaddr_t *localaddr, dns_dispatch_t **dispp);
 /*
- * Attempt to connect to a existing TCP connection (connection completed
- * if connected == NULL).
+ * Attempt to connect to a existing TCP connection.
  */
 
 typedef void (*dispatch_cb_t)(isc_result_t eresult, isc_region_t *region,
@@ -308,22 +308,10 @@ dns_dispatch_add(dns_dispatch_t *disp, unsigned int options,
 
 void
 dns_dispatch_done(dns_dispentry_t **respp);
-/*%<
- * Disconnects a dispatch response entry from its dispatch and shuts it
- * down. This is called when the dispatch is complete; use
- * dns_dispatch_cancel() if it is still pending.
- *
- * Requires:
- *\li	"resp" != NULL and "*resp" contain a value previously allocated
- *	by dns_dispatch_add();
- */
+/*<
+ * Disconnect a dispatch response entry from its dispatch, cancel all
+ * pending connects and reads in a dispatch entry and shut it down.
 
-void
-dns_dispatch_cancel(dns_dispentry_t **respp);
-/*%<
- * Cancel all pending connects and reads in a dispatch entry,
- * then call dns_dispatch_done(). This is used when the caller
- * cancels a dispatch response before it has completed.
  *
  * Requires:
  *\li	"resp" != NULL and "*resp" contain a value previously allocated

lib/dns/request.c

@@ -339,20 +339,18 @@ isblackholed(dns_dispatchmgr_t *dispatchmgr, const isc_sockaddr_t *destaddr) {
 static isc_result_t
 tcp_dispatch(bool newtcp, dns_requestmgr_t *requestmgr,
 	     const isc_sockaddr_t *srcaddr, const isc_sockaddr_t *destaddr,
-	     isc_dscp_t dscp, bool *connected, dns_dispatch_t **dispatchp) {
+	     isc_dscp_t dscp, dns_dispatch_t **dispatchp) {
 	isc_result_t result;
 
 	if (!newtcp) {
 		result = dns_dispatch_gettcp(requestmgr->dispatchmgr, destaddr,
-					     srcaddr, connected, dispatchp);
+					     srcaddr, dispatchp);
 		if (result == ISC_R_SUCCESS) {
 			char peer[ISC_SOCKADDR_FORMATSIZE];
 
 			isc_sockaddr_format(destaddr, peer, sizeof(peer));
 			req_log(ISC_LOG_DEBUG(1),
-				"attached to %s TCP "
-				"connection to %s",
-				*connected ? "existing" : "pending", peer);
+				"attached to TCP connection to %s", peer);
 			return (result);
 		}
 	}
@@ -394,12 +392,12 @@ udp_dispatch(dns_requestmgr_t *requestmgr, const isc_sockaddr_t *srcaddr,
 static isc_result_t
 get_dispatch(bool tcp, bool newtcp, dns_requestmgr_t *requestmgr,
 	     const isc_sockaddr_t *srcaddr, const isc_sockaddr_t *destaddr,
-	     isc_dscp_t dscp, bool *connected, dns_dispatch_t **dispatchp) {
+	     isc_dscp_t dscp, dns_dispatch_t **dispatchp) {
 	isc_result_t result;
 
 	if (tcp) {
 		result = tcp_dispatch(newtcp, requestmgr, srcaddr, destaddr,
-				      dscp, connected, dispatchp);
+				      dscp, dispatchp);
 	} else {
 		result = udp_dispatch(requestmgr, srcaddr, destaddr, dispatchp);
 	}
@@ -423,7 +421,6 @@ dns_request_createraw(dns_requestmgr_t *requestmgr, isc_buffer_t *msgbuf,
 	bool tcp = false;
 	bool newtcp = false;
 	isc_region_t r;
-	bool connected = false;
 	unsigned int dispopt = 0;
 
 	REQUIRE(VALID_REQUESTMGR(requestmgr));
@@ -497,7 +494,7 @@ dns_request_createraw(dns_requestmgr_t *requestmgr, isc_buffer_t *msgbuf,
 again:
 
 	result = get_dispatch(tcp, newtcp, requestmgr, srcaddr, destaddr, dscp,
-			      &connected, &request->dispatch);
+			      &request->dispatch);
 	if (result != ISC_R_SUCCESS) {
 		goto detach;
 	}
@@ -514,7 +511,6 @@ again:
 	if (result != ISC_R_SUCCESS) {
 		if ((options & DNS_REQUESTOPT_FIXEDID) != 0 && !newtcp) {
 			newtcp = true;
-			connected = false;
 			dns_dispatch_detach(&request->dispatch);
 			goto again;
 		}
@@ -534,21 +530,15 @@ again:
 	UNLOCK(&requestmgr->lock);
 
 	request->destaddr = *destaddr;
-	if (tcp && connected) {
-		req_send(request);
 
-		/* no need to call req_connected(), detach here */
-		req_detach(&(dns_request_t *){ request });
-	} else {
-		request->flags |= DNS_REQUEST_F_CONNECTING;
-		if (tcp) {
-			request->flags |= DNS_REQUEST_F_TCP;
-		}
+	request->flags |= DNS_REQUEST_F_CONNECTING;
+	if (tcp) {
+		request->flags |= DNS_REQUEST_F_TCP;
+	}
 
-		result = dns_dispatch_connect(request->dispentry);
-		if (result != ISC_R_SUCCESS) {
-			goto unlink;
-		}
+	result = dns_dispatch_connect(request->dispentry);
+	if (result != ISC_R_SUCCESS) {
+		goto unlink;
 	}
 
 	req_log(ISC_LOG_DEBUG(3), "dns_request_createraw: request %p", request);
@@ -660,7 +650,7 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message,
 
 again:
 	result = get_dispatch(tcp, false, requestmgr, srcaddr, destaddr, dscp,
-			      &connected, &request->dispatch);
+			      &request->dispatch);
 	if (result != ISC_R_SUCCESS) {
 		goto detach;
 	}
@@ -841,7 +831,7 @@ request_cancel(dns_request_t *request) {
 		request->flags &= ~DNS_REQUEST_F_CONNECTING;
 
 		if (request->dispentry != NULL) {
-			dns_dispatch_cancel(&request->dispentry);
+			dns_dispatch_done(&request->dispentry);
 		}
 
 		dns_dispatch_detach(&request->dispatch);
@@ -996,13 +986,13 @@ static void
 req_response(isc_result_t result, isc_region_t *region, void *arg) {
 	dns_request_t *request = (dns_request_t *)arg;
 
-	req_log(ISC_LOG_DEBUG(3), "req_response: request %p: %s", request,
-		isc_result_totext(result));
-
 	if (result == ISC_R_CANCELED) {
 		return;
 	}
 
+	req_log(ISC_LOG_DEBUG(3), "req_response: request %p: %s", request,
+		isc_result_totext(result));
+
 	if (result == ISC_R_TIMEDOUT) {
 		LOCK(&request->requestmgr->locks[request->hash]);
 		if (request->udpcount != 0) {

lib/dns/resolver.c

@@ -1494,7 +1494,7 @@ fctx_cancelquery(resquery_t **queryp, isc_time_t *finish, bool no_response,
 	 * exist, cancel them.
 	 */
 	if (query->dispentry != NULL) {
-		dns_dispatch_cancel(&query->dispentry);
+		dns_dispatch_done(&query->dispentry);
 	}
 
 	LOCK(&fctx->lock);