WIP: make UDP dispatch just pick random QID and don't check for duplicities

This is second in the series of fixing the usage of hashtables in the
dns_adb and the dns_resolver units.

Currently, the fetch buckets (used to hold the fetch context) and zone
buckets (used to hold per-domain counters) would never get cleaned from
the memory.  Combined with the fact that the hashtable now grows as
needed (instead of using hashtable as buckets), the memory usage in the
resolver can just grow and it never drops down.

In this commit, the usage of hashtables (hashmaps) has been completely
rewritten, so there are no "buckets" and all the matching conditions are
directly mapped into the hashtable key:

 1. For per-domain counter hashtable, this is simple as the lowercase
    domain name is used directly as a counter.

 2. For fetch context hashtable, this requires copying some extra flags
    back and forth in the key.

As we don't hold the "buckets" forever, the cleaning mechanism has been
rewritten as well:

 1. For per-domain counter hashtable, this is again much simpler, as we
    only need to check whether the usage counter is still zero under the
    lock and bail-out on cleaning if the counter is in use.

 2. For fetch context hashtable, this is more complicated as the fetch
    context cannot be reused after it has been finished.  The algorithm
    is different, the fetch context is always removed from the
    hashtable, but if we find the fetch context that has been marked
    as finished in the lookup function, we help with the cleaning from
    the hashtable and try again.

Couple of additional changes have been implemented in this refactoring
as those were needed for correct functionality and could be split into
individual commits (or would not make sense as seperate commits):

 1. The dns_resolver_createfetch() has an option to create "unshared"
    fetch.  The "unshared" fetch will never get matched, so there's
    little point in storing the "unshared" fetch in the hashtable.
    Therefore the "unshared" fetches are now detached from the
    hashtable and live just on their own.

 2. Replace the custom reference counting with ISC_REFCOUNT_DECL/IMPL
    macros for better tracing.

 3. fctx_done_detach() is idempotent, it makes the "final" detach (the
    one matching the create function) only once.  But that also means
    that it has to be called before the detach that kept the fetch
    context alive in the callback.  A new macro fctx_done_unref() has
    been added to allow this code flow:

    fctx_done_unref(fctx, result);
    fctx_detach(&fctx);

    Doing this the other way around could cause fctx to get destroyed in
    the fctx_unref() first and fctx_done_detach() would cause UAF.

 4. The resume_qmin() and resume_dslookup() callbacks have been
    refactored for more readability and simpler code paths.  The
    validated() callback has also received some of the simplifications,
    but it should be refactored in the future as it is bit of spaghetti
    now.

CHANGES

@@ -1,3 +1,8 @@
+6032.	[bug]		Refactor the dns_resolver unit to store the fetch
+			contexts and zone counter directly in the hash
+			tables without buckets and implement effective
+			cleaning of both objects. [GL #3709]
+
 6031.	[bug]		Move the "final reference detached" log message
 			from dns_zone unit to the DEBUG(1) log level.
 			[GL #3707]

lib/dns/adb.c

@@ -1020,7 +1020,7 @@ new_adbname(dns_adb_t *adb, const dns_name_t *dnsname, bool start_at_zone) {
 
 #if DNS_ADB_TRACE
 	fprintf(stderr, "dns_adbname__init:%s:%s:%d:%p->references = 1\n",
-		__func__, __FILE__, __LINE__ + 1, name);
+		__func__, __FILE__, __LINE__, name);
 #endif
 	isc_refcount_init(&name->references, 1);
 

lib/dns/dispatch.c

@@ -20,6 +20,7 @@
 #include <unistd.h>
 
 #include <isc/atomic.h>
+#include <isc/hashmap.h>
 #include <isc/mem.h>
 #include <isc/mutex.h>
 #include <isc/net.h>
@@ -44,13 +45,12 @@
 typedef ISC_LIST(dns_dispentry_t) dns_displist_t;
 
 typedef struct dns_qid {
-	unsigned int magic;
 	isc_mutex_t lock;
-	unsigned int qid_nbuckets;  /*%< hash table size */
-	unsigned int qid_increment; /*%< id increment on collision */
-	dns_displist_t *qid_table;  /*%< the table itself */
+	isc_hashmap_t *table;
 } dns_qid_t;
 
+#define QID_HASH_BITS 12
+
 struct dns_dispatchmgr {
 	/* Unlocked. */
 	unsigned int magic;
@@ -64,14 +64,16 @@ struct dns_dispatchmgr {
 	isc_mutex_t lock;
 	ISC_LIST(dns_dispatch_t) list;
 
-	dns_qid_t *qid;
-
 	in_port_t *v4ports;    /*%< available ports for IPv4 */
 	unsigned int nv4ports; /*%< # of available ports for IPv4 */
 	in_port_t *v6ports;    /*%< available ports for IPv4 */
 	unsigned int nv6ports; /*%< # of available ports for IPv4 */
 };
 
+#define QID_KEYSIZE                                                 \
+	sizeof(struct sockaddr_storage) + sizeof(dns_messageid_t) + \
+		sizeof(in_port_t) + sizeof(dns_messageid_t)
+
 struct dns_dispentry {
 	unsigned int magic;
 	isc_refcount_t references;
@@ -79,7 +81,6 @@ struct dns_dispentry {
 	isc_nmhandle_t *handle; /*%< netmgr handle for UDP connection */
 	dns_transport_t *transport;
 	isc_tlsctx_cache_t *tlsctx_cache;
-	unsigned int bucket;
 	unsigned int retries;
 	unsigned int timeout;
 	isc_time_t start;
@@ -96,6 +97,8 @@ struct dns_dispentry {
 	ISC_LINK(dns_dispentry_t) alink;
 	ISC_LINK(dns_dispentry_t) plink;
 	ISC_LINK(dns_dispentry_t) rlink;
+	size_t keylen;
+	uint8_t key[QID_KEYSIZE];
 };
 
 /*%
@@ -139,10 +142,9 @@ struct dns_dispatch {
 	unsigned int tcpbuffers; /*%< allocated buffers */
 
 	unsigned int timedout;
-};
 
-#define QID_MAGIC    ISC_MAGIC('Q', 'i', 'd', ' ')
-#define VALID_QID(e) ISC_MAGIC_VALID((e), QID_MAGIC)
+	dns_qid_t qid;
+};
 
 #define RESPONSE_MAGIC	  ISC_MAGIC('D', 'r', 's', 'p')
 #define VALID_RESPONSE(e) ISC_MAGIC_VALID((e), RESPONSE_MAGIC)
@@ -156,65 +158,33 @@ struct dns_dispatch {
 #define DNS_DISPATCHMGR_MAGIC ISC_MAGIC('D', 'M', 'g', 'r')
 #define VALID_DISPATCHMGR(e)  ISC_MAGIC_VALID((e), DNS_DISPATCHMGR_MAGIC)
 
-/*%
- * Quota to control the number of UDP dispatch sockets.  If a dispatch has
- * more than the quota of sockets, new queries will purge oldest ones, so
- * that a massive number of outstanding queries won't prevent subsequent
- * queries (especially if the older ones take longer time and result in
- * timeout).
- */
-#ifndef DNS_DISPATCH_SOCKSQUOTA
-#define DNS_DISPATCH_SOCKSQUOTA 3072
-#endif /* ifndef DNS_DISPATCH_SOCKSQUOTA */
-
-/*%
- * Quota to control the number of concurrent requests that can be handled
- * by each TCP dispatch. (UDP dispatches do not currently support socket
- * sharing.)
- */
-#ifndef DNS_DISPATCH_MAXREQUESTS
-#define DNS_DISPATCH_MAXREQUESTS 32768
-#endif /* ifndef DNS_DISPATCH_MAXREQUESTS */
-
-/*%
- * Number of buckets in the QID hash table, and the value to
- * increment the QID by when attempting to avoid collisions.
- * The number of buckets should be prime, and the increment
- * should be the next higher prime number.
- */
-#ifndef DNS_QID_BUCKETS
-#define DNS_QID_BUCKETS 16411
-#endif /* ifndef DNS_QID_BUCKETS */
-#ifndef DNS_QID_INCREMENT
-#define DNS_QID_INCREMENT 16433
-#endif /* ifndef DNS_QID_INCREMENT */
-
 /*
  * Statics.
  */
+
+ISC_REFCOUNT_DECL(dns_dispentry);
+
 static void
 dispatchmgr_destroy(dns_dispatchmgr_t *mgr);
 
-static dns_dispentry_t *
-entry_search(dns_qid_t *, const isc_sockaddr_t *, dns_messageid_t, in_port_t,
-	     unsigned int);
 static void
 udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region,
 	 void *arg);
 static void
 tcp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region,
 	 void *arg);
-static uint32_t
-dns_hash(dns_qid_t *, const isc_sockaddr_t *, dns_messageid_t, in_port_t);
 static void
 dispatch_free(dns_dispatch_t **dispp);
 static isc_result_t
 dispatch_createudp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *localaddr,
 		   dns_dispatch_t **dispp);
+static size_t
+qid_makekey(uint8_t *key, const isc_sockaddr_t *addr, dns_messageid_t id,
+	    in_port_t port);
 static void
-qid_allocate(dns_dispatchmgr_t *mgr, dns_qid_t **qidp);
+qid_init(isc_mem_t *mctx, dns_qid_t *qid);
 static void
-qid_destroy(isc_mem_t *mctx, dns_qid_t **qidp);
+qid_free(dns_qid_t *qid);
 static void
 startrecv(isc_nmhandle_t *handle, dns_dispatch_t *disp, dns_dispentry_t *resp);
 void
@@ -280,23 +250,6 @@ dispatch_log(dns_dispatch_t *disp, int level, const char *fmt, ...) {
 		      msgbuf);
 }
 
-/*
- * Return a hash of the destination and message id.
- */
-static uint32_t
-dns_hash(dns_qid_t *qid, const isc_sockaddr_t *dest, dns_messageid_t id,
-	 in_port_t port) {
-	uint32_t ret;
-
-	ret = isc_sockaddr_hash(dest, true);
-	ret ^= ((uint32_t)id << 16) | port;
-	ret %= qid->qid_nbuckets;
-
-	INSIST(ret < qid->qid_nbuckets);
-
-	return (ret);
-}
-
 /*%
  * Choose a random port number for a dispatch entry.
  * The caller must hold the disp->lock
@@ -358,43 +311,6 @@ deactivate_dispentry(dns_dispatch_t *disp, dns_dispentry_t *resp) {
 	disp->nsockets--;
 }
 
-/*
- * Find an entry for query ID 'id', socket address 'dest', and port number
- * 'port'.
- * Return NULL if no such entry exists.
- */
-static dns_dispentry_t *
-entry_search(dns_qid_t *qid, const isc_sockaddr_t *dest, dns_messageid_t id,
-	     in_port_t port, unsigned int bucket) {
-	dns_dispentry_t *res = NULL;
-
-	REQUIRE(VALID_QID(qid));
-	REQUIRE(bucket < qid->qid_nbuckets);
-
-	res = ISC_LIST_HEAD(qid->qid_table[bucket]);
-
-	while (res != NULL) {
-		if (res->id == id && isc_sockaddr_equal(dest, &res->peer) &&
-		    res->port == port)
-		{
-			return (res);
-		}
-		res = ISC_LIST_NEXT(res, link);
-	}
-
-	return (NULL);
-}
-
-static void
-dispentry_attach(dns_dispentry_t *resp, dns_dispentry_t **respp) {
-	REQUIRE(VALID_RESPONSE(resp));
-	REQUIRE(respp != NULL && *respp == NULL);
-
-	isc_refcount_increment(&resp->references);
-
-	*respp = resp;
-}
-
 static void
 dispentry_destroy(dns_dispentry_t *resp) {
 	dns_dispatch_t *disp = resp->disp;
@@ -427,21 +343,7 @@ dispentry_destroy(dns_dispentry_t *resp) {
 	dns_dispatch_detach(&disp);
 }
 
-static void
-dispentry_detach(dns_dispentry_t **respp) {
-	dns_dispentry_t *resp = NULL;
-	uint_fast32_t ref;
-
-	REQUIRE(respp != NULL && VALID_RESPONSE(*respp));
-
-	resp = *respp;
-	*respp = NULL;
-
-	ref = isc_refcount_decrement(&resp->references);
-	if (ref == 1) {
-		dispentry_destroy(resp);
-	}
-}
+ISC_REFCOUNT_IMPL(dns_dispentry, dispentry_destroy);
 
 /*
  * How long in milliseconds has it been since this dispentry
@@ -606,24 +508,23 @@ done:
 		response(eresult, region, resp->arg);
 	}
 
-	dispentry_detach(&resp);
+	dns_dispentry_detach(&resp);
 }
 
 static isc_result_t
 tcp_recv_timeout(dns_dispatch_t *disp, dns_dispentry_t **respp) {
 	dns_dispentry_t *resp = ISC_LIST_HEAD(disp->active);
-	if (resp != NULL) {
-		dispentry_attach(resp, &(dns_dispentry_t *){ NULL });
-		ISC_LIST_UNLINK(disp->active, resp, alink);
-		ISC_LIST_APPEND(disp->active, resp, alink);
-
-		disp->timedout++;
-
-		*respp = resp;
-		return (ISC_R_TIMEDOUT);
+	if (resp == NULL) {
+		return (ISC_R_NOTFOUND);
 	}
 
-	return (ISC_R_NOTFOUND);
+	ISC_LIST_UNLINK(disp->active, resp, alink);
+	ISC_LIST_APPEND(disp->active, resp, alink);
+
+	disp->timedout++;
+
+	*respp = dns_dispentry_ref(resp);
+	return (ISC_R_TIMEDOUT);
 }
 
 static isc_result_t
@@ -632,7 +533,6 @@ tcp_recv_success(dns_dispatch_t *disp, isc_region_t *region, dns_qid_t *qid,
 	isc_buffer_t source;
 	dns_messageid_t id;
 	unsigned int flags;
-	unsigned int bucket;
 	isc_result_t result = ISC_R_SUCCESS;
 	dns_dispentry_t *resp = NULL;
 
@@ -666,13 +566,19 @@ tcp_recv_success(dns_dispatch_t *disp, isc_region_t *region, dns_qid_t *qid,
 	/*
 	 * We have a valid response; find the associated dispentry object
 	 * and call the caller back.
+	 *
+	 * This is TCP, so we ignore the localport.
 	 */
-	bucket = dns_hash(qid, peer, id, disp->localport);
+	uint8_t key[QID_KEYSIZE];
+	size_t keylen = qid_makekey(key, peer, id, 0);
+	uint32_t hashval = isc_hashmap_hash(qid->table, key, keylen);
+
 	LOCK(&qid->lock);
-	resp = entry_search(qid, peer, id, disp->localport, bucket);
-	if (resp != NULL) {
-		dispentry_attach(resp, &(dns_dispentry_t *){ NULL });
-		*respp = resp;
+	result = isc_hashmap_find(qid->table, &hashval, key, keylen,
+				  (void *)&resp);
+	if (result == ISC_R_SUCCESS) {
+		INSIST(resp != NULL);
+		*respp = dns_dispentry_ref(resp);
 	} else if (disp->timedout > 0) {
 		/* There was active query that timed-out before */
 		disp->timedout--;
@@ -696,8 +602,8 @@ tcp_recv_success(dns_dispatch_t *disp, isc_region_t *region, dns_qid_t *qid,
 		/* We are not expecting this DNS message */
 		result = ISC_R_UNEXPECTED;
 	}
-	dispatch_log(disp, LVL(90), "search for response in bucket %d: %s",
-		     bucket, isc_result_totext(result));
+	dispatch_log(disp, LVL(90), "search for response in hashtable: %s",
+		     isc_result_totext(result));
 	UNLOCK(&qid->lock);
 
 	return (result);
@@ -712,7 +618,7 @@ tcp_recv_shutdown(dns_dispatch_t *disp, dns_displist_t *resps) {
 	 */
 	for (resp = ISC_LIST_HEAD(disp->active); resp != NULL; resp = next) {
 		next = ISC_LIST_NEXT(resp, alink);
-		dispentry_attach(resp, &(dns_dispentry_t *){ NULL });
+		dns_dispentry_ref(resp);
 		ISC_LIST_UNLINK(disp->active, resp, alink);
 		ISC_LIST_APPEND(*resps, resp, rlink);
 	}
@@ -722,7 +628,7 @@ static void
 tcp_recv_done(dns_dispentry_t *resp, isc_result_t eresult,
 	      isc_region_t *region) {
 	resp->response(eresult, region, resp->arg);
-	dispentry_detach(&resp);
+	dns_dispentry_detach(&resp);
 }
 
 static void
@@ -734,7 +640,7 @@ tcp_recv_cancelall(dns_displist_t *resps, isc_region_t *region,
 		next = ISC_LIST_NEXT(resp, rlink);
 		ISC_LIST_UNLINK(*resps, resp, rlink);
 		resp->response(result, region, resp->arg);
-		dispentry_detach(&resp);
+		dns_dispentry_detach(&resp);
 	}
 }
 
@@ -767,7 +673,7 @@ tcp_recv(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region,
 
 	atomic_store(&disp->tcpreading, false);
 
-	qid = disp->mgr->qid;
+	qid = &disp->qid;
 
 	ISC_LIST_INIT(resps);
 
@@ -929,7 +835,10 @@ dns_dispatchmgr_create(isc_mem_t *mctx, isc_nm_t *nm,
 	REQUIRE(mgrp != NULL && *mgrp == NULL);
 
 	mgr = isc_mem_get(mctx, sizeof(dns_dispatchmgr_t));
-	*mgr = (dns_dispatchmgr_t){ .magic = 0 };
+	*mgr = (dns_dispatchmgr_t){
+		.list = ISC_LIST_INITIALIZER,
+		.magic = DNS_DISPATCHMGR_MAGIC,
+	};
 
 	isc_refcount_init(&mgr->references, 1);
 
@@ -938,8 +847,6 @@ dns_dispatchmgr_create(isc_mem_t *mctx, isc_nm_t *nm,
 
 	isc_mutex_init(&mgr->lock);
 
-	ISC_LIST_INIT(mgr->list);
-
 	create_default_portset(mctx, AF_INET, &v4portset);
 	create_default_portset(mctx, AF_INET6, &v6portset);
 
@@ -948,38 +855,11 @@ dns_dispatchmgr_create(isc_mem_t *mctx, isc_nm_t *nm,
 	isc_portset_destroy(mctx, &v4portset);
 	isc_portset_destroy(mctx, &v6portset);
 
-	qid_allocate(mgr, &mgr->qid);
-	mgr->magic = DNS_DISPATCHMGR_MAGIC;
-
 	*mgrp = mgr;
 	return (ISC_R_SUCCESS);
 }
 
-void
-dns_dispatchmgr_attach(dns_dispatchmgr_t *mgr, dns_dispatchmgr_t **mgrp) {
-	REQUIRE(VALID_DISPATCHMGR(mgr));
-	REQUIRE(mgrp != NULL && *mgrp == NULL);
-
-	isc_refcount_increment(&mgr->references);
-
-	*mgrp = mgr;
-}
-
-void
-dns_dispatchmgr_detach(dns_dispatchmgr_t **mgrp) {
-	dns_dispatchmgr_t *mgr = NULL;
-	uint_fast32_t ref;
-
-	REQUIRE(mgrp != NULL && VALID_DISPATCHMGR(*mgrp));
-
-	mgr = *mgrp;
-	*mgrp = NULL;
-
-	ref = isc_refcount_decrement(&mgr->references);
-	if (ref == 1) {
-		dispatchmgr_destroy(mgr);
-	}
-}
+ISC_REFCOUNT_IMPL(dns_dispatchmgr, dispatchmgr_destroy);
 
 void
 dns_dispatchmgr_setblackhole(dns_dispatchmgr_t *mgr, dns_acl_t *blackhole) {
@@ -1012,8 +892,6 @@ dispatchmgr_destroy(dns_dispatchmgr_t *mgr) {
 	mgr->magic = 0;
 	isc_mutex_destroy(&mgr->lock);
 
-	qid_destroy(mgr->mctx, &mgr->qid);
-
 	if (mgr->blackhole != NULL) {
 		dns_acl_detach(&mgr->blackhole);
 	}
@@ -1045,43 +923,42 @@ dns_dispatchmgr_setstats(dns_dispatchmgr_t *mgr, isc_stats_t *stats) {
 	isc_stats_attach(stats, &mgr->stats);
 }
 
-static void
-qid_allocate(dns_dispatchmgr_t *mgr, dns_qid_t **qidp) {
-	dns_qid_t *qid = NULL;
-	unsigned int i;
+static size_t
+qid_makekey(uint8_t *key, const isc_sockaddr_t *addr, dns_messageid_t id,
+	    in_port_t port) {
+	size_t addr_len = 0;
+	((uint32_t *)key)[0] = ((uint32_t)id << 16) | port;
 
-	REQUIRE(qidp != NULL && *qidp == NULL);
-
-	qid = isc_mem_get(mgr->mctx, sizeof(*qid));
-	*qid = (dns_qid_t){ .qid_nbuckets = DNS_QID_BUCKETS,
-			    .qid_increment = DNS_QID_INCREMENT };
-
-	qid->qid_table = isc_mem_get(mgr->mctx,
-				     DNS_QID_BUCKETS * sizeof(dns_displist_t));
-	for (i = 0; i < qid->qid_nbuckets; i++) {
-		ISC_LIST_INIT(qid->qid_table[i]);
+	switch (addr->type.sa.sa_family) {
+	case AF_INET:
+		addr_len = sizeof(addr->type.sin.sin_addr);
+		memmove(&((uint32_t *)key)[1],
+			(uint8_t *)&addr->type.sin.sin_addr, addr_len);
+		break;
+	case AF_INET6:
+		addr_len = sizeof(addr->type.sin6.sin6_addr);
+		memmove(&((uint32_t *)key)[1],
+			(uint8_t *)&addr->type.sin6.sin6_addr, addr_len);
+		break;
+	default:
+		UNREACHABLE();
 	}
-
-	isc_mutex_init(&qid->lock);
-	qid->magic = QID_MAGIC;
-	*qidp = qid;
+	return (addr_len + sizeof(uint32_t));
 }
 
 static void
-qid_destroy(isc_mem_t *mctx, dns_qid_t **qidp) {
-	dns_qid_t *qid = NULL;
+qid_init(isc_mem_t *mctx, dns_qid_t *qid) {
+	*qid = (dns_qid_t){ 0 };
 
-	REQUIRE(qidp != NULL);
-	qid = *qidp;
-	*qidp = NULL;
+	isc_hashmap_create(mctx, QID_HASH_BITS, ISC_HASHMAP_CASE_SENSITIVE,
+			   &qid->table);
+	isc_mutex_init(&qid->lock);
+}
 
-	REQUIRE(VALID_QID(qid));
-
-	qid->magic = 0;
-	isc_mem_put(mctx, qid->qid_table,
-		    qid->qid_nbuckets * sizeof(dns_displist_t));
+static void
+qid_free(dns_qid_t *qid) {
+	isc_hashmap_destroy(&qid->table);
 	isc_mutex_destroy(&qid->lock);
-	isc_mem_put(mctx, qid, sizeof(*qid));
 }
 
 /*
@@ -1136,6 +1013,10 @@ dispatch_free(dns_dispatch_t **dispp) {
 	INSIST(disp->requests == 0);
 	INSIST(ISC_LIST_EMPTY(disp->active));
 
+	if (disp->socktype == isc_socktype_tcp) {
+		qid_free(&disp->qid);
+	}
+
 	isc_mutex_destroy(&disp->lock);
 
 	isc_mem_put(mgr->mctx, disp, sizeof(*disp));
@@ -1167,6 +1048,8 @@ dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *localaddr,
 		isc_sockaddr_setport(&disp->local, 0);
 	}
 
+	qid_init(mgr->mctx, &disp->qid);
+
 	/*
 	 * Append it to the dispatcher list.
 	 */
@@ -1343,6 +1226,11 @@ static void
 dispatch_destroy(dns_dispatch_t *disp) {
 	dns_dispatchmgr_t *mgr = disp->mgr;
 
+	LOCK(&disp->lock);
+	INSIST(ISC_LIST_EMPTY(disp->pending));
+	INSIST(ISC_LIST_EMPTY(disp->active));
+	UNLOCK(&disp->lock);
+
 	LOCK(&mgr->lock);
 	ISC_LIST_UNLINK(mgr->list, disp, link);
 	UNLOCK(&mgr->lock);
@@ -1363,37 +1251,7 @@ dispatch_destroy(dns_dispatch_t *disp) {
 	dns_dispatchmgr_detach(&mgr);
 }
 
-void
-dns_dispatch_attach(dns_dispatch_t *disp, dns_dispatch_t **dispp) {
-	REQUIRE(VALID_DISPATCH(disp));
-	REQUIRE(dispp != NULL && *dispp == NULL);
-
-	isc_refcount_increment(&disp->references);
-
-	*dispp = disp;
-}
-
-void
-dns_dispatch_detach(dns_dispatch_t **dispp) {
-	dns_dispatch_t *disp = NULL;
-	uint_fast32_t ref;
-
-	REQUIRE(dispp != NULL && VALID_DISPATCH(*dispp));
-
-	disp = *dispp;
-	*dispp = NULL;
-
-	ref = isc_refcount_decrement(&disp->references);
-	dispatch_log(disp, LVL(90), "detach: refcount %" PRIuFAST32, ref - 1);
-	if (ref == 1) {
-		LOCK(&disp->lock);
-		INSIST(ISC_LIST_EMPTY(disp->pending));
-		INSIST(ISC_LIST_EMPTY(disp->active));
-		UNLOCK(&disp->lock);
-
-		dispatch_destroy(disp);
-	}
-}
+ISC_REFCOUNT_IMPL(dns_dispatch, dispatch_destroy);
 
 isc_result_t
 dns_dispatch_add(dns_dispatch_t *disp, unsigned int options,
@@ -1401,142 +1259,138 @@ dns_dispatch_add(dns_dispatch_t *disp, unsigned int options,
 		 dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache,
 		 dispatch_cb_t connected, dispatch_cb_t sent,
 		 dispatch_cb_t response, void *arg, dns_messageid_t *idp,
-		 dns_dispentry_t **resp) {
-	dns_dispentry_t *res = NULL;
+		 dns_dispentry_t **respp) {
+	dns_dispentry_t *resp = NULL;
 	dns_qid_t *qid = NULL;
 	in_port_t localport = 0;
 	dns_messageid_t id;
-	unsigned int bucket;
 	bool ok = false;
-	int i = 0;
-	dispatch_cb_t oldest_response = NULL;
+	int tries = 64; /* FIXME: Why was this number picked */
+	isc_result_t result;
 
 	REQUIRE(VALID_DISPATCH(disp));
 	REQUIRE(dest != NULL);
-	REQUIRE(resp != NULL && *resp == NULL);
+	REQUIRE(respp != NULL && *respp == NULL);
 	REQUIRE(idp != NULL);
 	REQUIRE(disp->socktype == isc_socktype_tcp ||
 		disp->socktype == isc_socktype_udp);
 
 	LOCK(&disp->lock);
 
-	if (disp->requests >= DNS_DISPATCH_MAXREQUESTS) {
-		UNLOCK(&disp->lock);
-		return (ISC_R_QUOTA);
-	}
+	resp = isc_mem_get(disp->mgr->mctx, sizeof(*resp));
 
-	qid = disp->mgr->qid;
+	*resp = (dns_dispentry_t){
+		.port = localport,
+		.timeout = timeout,
+		.peer = *dest,
+		.connected = connected,
+		.sent = sent,
+		.response = response,
+		.arg = arg,
+		.link = ISC_LINK_INITIALIZER,
+		.alink = ISC_LINK_INITIALIZER,
+		.plink = ISC_LINK_INITIALIZER,
+		.rlink = ISC_LINK_INITIALIZER,
+		.magic = RESPONSE_MAGIC,
+	};
 
-	if (disp->socktype == isc_socktype_udp &&
-	    disp->nsockets > DNS_DISPATCH_SOCKSQUOTA)
-	{
-		dns_dispentry_t *oldest = NULL;
+	isc_refcount_init(&resp->references, 1);
 
-		/*
-		 * Kill oldest outstanding query if the number of sockets
-		 * exceeds the quota to keep the room for new queries.
-		 */
-		oldest = ISC_LIST_HEAD(disp->active);
-		if (oldest != NULL) {
-			oldest_response = oldest->response;
-			inc_stats(disp->mgr, dns_resstatscounter_dispabort);
-		}
-	}
-
-	res = isc_mem_get(disp->mgr->mctx, sizeof(*res));
-
-	*res = (dns_dispentry_t){ .port = localport,
-				  .timeout = timeout,
-				  .peer = *dest,
-				  .connected = connected,
-				  .sent = sent,
-				  .response = response,
-				  .arg = arg };
-
-	isc_refcount_init(&res->references, 1);
-
-	ISC_LINK_INIT(res, link);
-	ISC_LINK_INIT(res, alink);
-	ISC_LINK_INIT(res, plink);
-	ISC_LINK_INIT(res, rlink);
-
-	if (disp->socktype == isc_socktype_udp) {
-		isc_result_t result = setup_socket(disp, res, dest, &localport);
+	switch (disp->socktype) {
+	case isc_socktype_udp:
+		result = setup_socket(disp, resp, dest, &localport);
 		if (result != ISC_R_SUCCESS) {
-			isc_mem_put(disp->mgr->mctx, res, sizeof(*res));
-			UNLOCK(&disp->lock);
-			inc_stats(disp->mgr, dns_resstatscounter_dispsockfail);
-			return (result);
+			goto fail;
 		}
-	}
 
-	/*
-	 * Try somewhat hard to find a unique ID. Start with
-	 * a random number unless DNS_DISPATCHOPT_FIXEDID is set,
-	 * in which case we start with the ID passed in via *idp.
-	 */
-	if ((options & DNS_DISPATCHOPT_FIXEDID) != 0) {
-		id = *idp;
-	} else {
-		id = (dns_messageid_t)isc_random16();
-	}
+		/* Pick new messsage ID */
+		if ((options & DNS_DISPATCHOPT_FIXEDID) != 0) {
+			id = *idp;
+		} else {
+			id = (dns_messageid_t)isc_random16();
+		}
+		break;
+	case isc_socktype_tcp:
+		/*
+		 * Try somewhat hard to find a unique ID. Start with a random
+		 * number unless DNS_DISPATCHOPT_FIXEDID is set, in which case
+		 * we start with the ID passed in via *idp.
+		 */
 
-	LOCK(&qid->lock);
-	do {
-		dns_dispentry_t *entry = NULL;
-		bucket = dns_hash(qid, dest, id, localport);
-		entry = entry_search(qid, dest, id, localport, bucket);
-		if (entry == NULL) {
+		qid = &disp->qid;
+		if ((options & DNS_DISPATCHOPT_FIXEDID) != 0) {
+			tries = 1;
+		}
+		LOCK(&qid->lock);
+		for (int i = 0; i < tries; i++) {
+			/* Pick new messsage ID */
+			if ((options & DNS_DISPATCHOPT_FIXEDID) != 0) {
+				id = *idp;
+			} else {
+				id = (dns_messageid_t)isc_random16();
+			}
+
+			/* Compute the hashtable key */
+			INSIST(disp->socktype == isc_socktype_udp ||
+			       localport == 0);
+			resp->keylen = qid_makekey(resp->key, dest, id,
+						   localport);
+
+			uint32_t hashval = isc_hashmap_hash(
+				qid->table, resp->key, resp->keylen);
+
+			/* Try to add this to the hashtable */
+			result = isc_hashmap_add(qid->table, &hashval,
+						 resp->key, resp->keylen, resp);
+			if (result == ISC_R_EXISTS) {
+				/* Aw, try again */
+				continue;
+			}
+			/* Success */
 			ok = true;
 			break;
 		}
-		id += qid->qid_increment;
-		id &= 0x0000ffff;
-	} while (i++ < 64);
-	UNLOCK(&qid->lock);
-
-	if (!ok) {
-		isc_mem_put(disp->mgr->mctx, res, sizeof(*res));
-		UNLOCK(&disp->lock);
-		return (ISC_R_NOMORE);
+		UNLOCK(&qid->lock);
+		if (!ok) {
+			result = ISC_R_NOMORE;
+			goto fail;
+		}
+		break;
+	default:
+		UNREACHABLE();
 	}
 
 	if (transport != NULL) {
-		dns_transport_attach(transport, &res->transport);
+		dns_transport_attach(transport, &resp->transport);
 	}
 
 	if (tlsctx_cache != NULL) {
-		isc_tlsctx_cache_attach(tlsctx_cache, &res->tlsctx_cache);
+		isc_tlsctx_cache_attach(tlsctx_cache, &resp->tlsctx_cache);
 	}
 
-	dns_dispatch_attach(disp, &res->disp);
+	dns_dispatch_attach(disp, &resp->disp);
 
-	res->id = id;
-	res->bucket = bucket;
-	res->magic = RESPONSE_MAGIC;
+	resp->id = id;
 
 	disp->requests++;
 
-	LOCK(&qid->lock);
-	ISC_LIST_APPEND(qid->qid_table[bucket], res, link);
-	UNLOCK(&qid->lock);
-
 	inc_stats(disp->mgr, (disp->socktype == isc_socktype_udp)
 				     ? dns_resstatscounter_disprequdp
 				     : dns_resstatscounter_dispreqtcp);
 
-	ISC_LIST_APPEND(disp->active, res, alink);
+	ISC_LIST_APPEND(disp->active, resp, alink);
 
 	UNLOCK(&disp->lock);
 
-	if (oldest_response != NULL) {
-		oldest_response(ISC_R_CANCELED, NULL, res->arg);
-	}
-
 	*idp = id;
-	*resp = res;
+	*respp = resp;
 
 	return (ISC_R_SUCCESS);
+fail:
+	isc_mem_put(disp->mgr->mctx, resp, sizeof(*resp));
+	UNLOCK(&disp->lock);
+	inc_stats(disp->mgr, dns_resstatscounter_dispsockfail);
+	return (result);
 }
 
 void
@@ -1547,11 +1401,10 @@ dispatch_getnext(dns_dispatch_t *disp, dns_dispentry_t *resp, int32_t timeout) {
 	case isc_socktype_udp:
 		REQUIRE(resp != NULL);
 
-		dispentry_attach(resp, &(dns_dispentry_t *){ NULL });
 		if (timeout > 0) {
 			isc_nmhandle_settimeout(resp->handle, timeout);
 		}
-		isc_nm_read(resp->handle, udp_recv, resp);
+		isc_nm_read(resp->handle, udp_recv, dns_dispentry_ref(resp));
 		break;
 
 	case isc_socktype_tcp:
@@ -1621,8 +1474,6 @@ dns_dispatch_cancel(dns_dispentry_t **respp) {
 	LOCK(&disp->lock);
 	/* TCP pending connection. */
 	if (ISC_LINK_LINKED(resp, plink)) {
-		dns_dispentry_t *copy = resp;
-
 		ISC_LIST_UNLINK(disp->pending, resp, plink);
 		if (resp->connected != NULL) {
 			resp->connected(ISC_R_CANCELED, NULL, resp->arg);
@@ -1635,7 +1486,7 @@ dns_dispatch_cancel(dns_dispentry_t **respp) {
 		 * that we won't reach, and again later in
 		 * dns_dispatch_done().
 		 */
-		dispentry_detach(&copy);
+		dns_dispentry_unref(resp);
 		UNLOCK(&disp->lock);
 		goto done;
 	}
@@ -1681,8 +1532,6 @@ dns_dispatch_done(dns_dispentry_t **respp) {
 
 	REQUIRE(VALID_DISPATCHMGR(mgr));
 
-	qid = mgr->qid;
-
 	LOCK(&disp->lock);
 	INSIST(disp->requests > 0);
 	disp->requests--;
@@ -1693,12 +1542,20 @@ dns_dispatch_done(dns_dispentry_t **respp) {
 
 	deactivate_dispentry(disp, resp);
 
-	LOCK(&qid->lock);
-	ISC_LIST_UNLINK(qid->qid_table[resp->bucket], resp, link);
-	UNLOCK(&qid->lock);
+	if (disp->socktype == isc_socktype_tcp) {
+		qid = &disp->qid;
+
+		LOCK(&qid->lock);
+		uint32_t hashval = isc_hashmap_hash(qid->table, resp->key,
+						    resp->keylen);
+		isc_result_t result = isc_hashmap_delete(
+			qid->table, &hashval, resp->key, resp->keylen);
+		INSIST(result == ISC_R_SUCCESS);
+		UNLOCK(&qid->lock);
+	}
 	UNLOCK(&disp->lock);
 
-	dispentry_detach(respp);
+	dns_dispentry_detach(respp);
 }
 
 /*
@@ -1712,8 +1569,7 @@ startrecv(isc_nmhandle_t *handle, dns_dispatch_t *disp, dns_dispentry_t *resp) {
 
 		TIME_NOW(&resp->start);
 		isc_nmhandle_attach(handle, &resp->handle);
-		dispentry_attach(resp, &(dns_dispentry_t *){ NULL });
-		isc_nm_read(resp->handle, udp_recv, resp);
+		isc_nm_read(resp->handle, udp_recv, dns_dispentry_ref(resp));
 		break;
 
 	case isc_socktype_tcp:
@@ -1772,7 +1628,7 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) {
 		if (resp->connected != NULL) {
 			resp->connected(eresult, NULL, resp->arg);
 		}
-		dispentry_detach(&resp);
+		dns_dispentry_detach(&resp);
 	}
 
 	dns_dispatch_detach(&disp);
@@ -1808,7 +1664,7 @@ udp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) {
 	}
 
 detach:
-	dispentry_detach(&resp);
+	dns_dispentry_detach(&resp);
 }
 
 isc_result_t
@@ -1849,7 +1705,7 @@ dns_dispatch_connect(dns_dispentry_t *resp) {
 	}
 
 	/* This will be detached once we've connected. */
-	dispentry_attach(resp, &(dns_dispentry_t *){ NULL });
+	dns_dispentry_ref(resp);
 
 	switch (transport_type) {
 	case DNS_TRANSPORT_TCP:
@@ -1893,7 +1749,7 @@ dns_dispatch_connect(dns_dispentry_t *resp) {
 			if (resp->connected != NULL) {
 				resp->connected(ISC_R_SUCCESS, NULL, resp->arg);
 			}
-			dispentry_detach(&resp);
+			dns_dispentry_detach(&resp);
 			break;
 
 		default:
@@ -1926,7 +1782,7 @@ send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 		isc_nm_cancelread(handle);
 	}
 
-	dispentry_detach(&resp);
+	dns_dispentry_detach(&resp);
 }
 
 void
@@ -1970,8 +1826,7 @@ dns_dispatch_send(dns_dispentry_t *resp, isc_region_t *r, isc_dscp_t dscp) {
 		handle = resp->handle;
 	}
 
-	dispentry_attach(resp, &(dns_dispentry_t *){ NULL });
-	isc_nm_send(handle, r, send_done, resp);
+	isc_nm_send(handle, r, send_done, dns_dispentry_ref(resp));
 }
 
 isc_result_t

lib/dns/include/dns/dispatch.h

@@ -14,6 +14,7 @@
 #pragma once
 
 #include <isc/netmgr.h>
+#include <isc/refcount.h>
 
 /*****
 ***** Module Info
@@ -94,25 +95,9 @@ 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);
+ISC_REFCOUNT_DECL(dns_dispatchmgr);
 /*%<
- * Attach to a dispatch manger.
- *
- * Requires:
- *\li	 is valid.
- *
- *\li	mgrp != NULL && *mgrp == NULL
- */
-
-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/ref/unref to a dispatch manger.
  */
 
 void
@@ -201,24 +186,9 @@ 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);
+ISC_REFCOUNT_DECL(dns_dispatch);
 /*%<
- * Attach to a dispatch handle.
- *
- * Requires:
- *\li	disp is valid.
- *
- *\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.
+ * Attach/detach/ref/unref to a dispatch handle.
  */
 
 isc_result_t

lib/dns/include/dns/resolver.h

@@ -50,11 +50,16 @@
 
 #include <isc/event.h>
 #include <isc/lang.h>
+#include <isc/refcount.h>
 #include <isc/stats.h>
 
 #include <dns/fixedname.h>
 #include <dns/types.h>
 
+#include <netinet/in.h>
+
+#undef DNS_RESOLVER_TRACE
+
 ISC_LANG_BEGINDECLS
 
 /*%
@@ -253,11 +258,19 @@ dns_resolver_shutdown(dns_resolver_t *res);
  *\li	'res' is a valid resolver.
  */
 
-void
-dns_resolver_attach(dns_resolver_t *source, dns_resolver_t **targetp);
-
-void
-dns_resolver_detach(dns_resolver_t **resp);
+#if DNS_RESOLVER_TRACE
+#define dns_resolver_ref(ptr) \
+	dns_resolver__ref(ptr, __func__, __FILE__, __LINE__)
+#define dns_resolver_unref(ptr) \
+	dns_resolver__unref(ptr, __func__, __FILE__, __LINE__)
+#define dns_resolver_attach(ptr, ptrp) \
+	dns_resolver__attach(ptr, ptrp, __func__, __FILE__, __LINE__)
+#define dns_resolver_detach(ptrp) \
+	dns_resolver__detach(ptrp, __func__, __FILE__, __LINE__)
+ISC_REFCOUNT_TRACE_DECL(dns_resolver);
+#else
+ISC_REFCOUNT_DECL(dns_resolver);
+#endif
 
 isc_result_t
 dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,