From 86d9d04fd8931e47c527cef08ae8ee89a695b707 Mon Sep 17 00:00:00 2001
From: Mark Andrews <marka@isc.org>
Date: Thu, 22 Oct 2020 16:13:06 +1100
Subject: [PATCH] Hold qid->lock when calling deref_portentry() as

socket_search() need portentry to be unchanging.

    WARNING: ThreadSanitizer: data race
    Write of size 8 at 0x000000000001 by thread T1 (mutexes: write M1):
    #0 deref_portentry lib/dns/dispatch.c:630
    #1 deactivate_dispsocket lib/dns/dispatch.c:861
    #2 udp_recv lib/dns/dispatch.c:1105
    #3 udp_exrecv lib/dns/dispatch.c:1028
    #4 dispatch lib/isc/task.c:1152
    #5 run lib/isc/task.c:1344
    #6 <null> <null>

    Previous read of size 8 at 0x000000000001 by thread T2 (mutexes: write M1, write M2):
    #0 socket_search lib/dns/dispatch.c:661
    #1 get_dispsocket lib/dns/dispatch.c:744
    #2 dns_dispatch_addresponse lib/dns/dispatch.c:3120
    #3 resquery_send lib/dns/resolver.c:2467
    #4 fctx_query lib/dns/resolver.c:2217
    #5 fctx_try lib/dns/resolver.c:4245
    #6 fctx_timeout lib/dns/resolver.c:4570
    #7 dispatch lib/isc/task.c:1152
    #8 run lib/isc/task.c:1344
    #9 <null> <null>

(cherry picked from commit 5c253c416d0bc0cce7606667c6703f44a98e9494)
Conflict: NA
Reference: https://gitlab.isc.org/isc-projects/bind9/-/commit/86d9d04fd8931e47c527cef08ae8ee89a695b707
---
 lib/dns/dispatch.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c
index 7cfff94868..80d76b79c9 100644
--- a/lib/dns/dispatch.c
+++ b/lib/dns/dispatch.c
@@ -599,34 +599,22 @@ new_portentry(dns_dispatch_t *disp, in_port_t port) {
 }
 
 /*%
- * The caller must not hold the qid->lock.
+ * The caller must hold the qid->lock.
  */
 static void
 deref_portentry(dns_dispatch_t *disp, dispportentry_t **portentryp) {
 	dispportentry_t *portentry = *portentryp;
-	dns_qid_t *qid;
+	*portentryp = NULL;
 
 	REQUIRE(disp->port_table != NULL);
 	REQUIRE(portentry != NULL && portentry->refs > 0);
 
-	qid = DNS_QID(disp);
-	LOCK(&qid->lock);
-	portentry->refs--;
-
-	if (portentry->refs == 0) {
+	if (--portentry->refs == 0) {
 		ISC_LIST_UNLINK(disp->port_table[portentry->port %
 						 DNS_DISPATCH_PORTTABLESIZE],
 				portentry, link);
 		isc_mempool_put(disp->portpool, portentry);
 	}
-
-	/*
-	 * Set '*portentryp' to NULL inside the lock so that
-	 * dispsock->portentry does not change in socket_search.
-	 */
-	*portentryp = NULL;
-
-	UNLOCK(&qid->lock);
 }
 
 /*%
@@ -764,9 +752,9 @@ get_dispsocket(dns_dispatch_t *disp, isc_sockaddr_t *dest,
 	if (result == ISC_R_SUCCESS) {
 		dispsock->socket = sock;
 		dispsock->host = *dest;
-		dispsock->portentry = portentry;
 		dispsock->bucket = bucket;
 		LOCK(&qid->lock);
+		dispsock->portentry = portentry;
 		ISC_LIST_APPEND(qid->sock_table[bucket], dispsock, blink);
 		UNLOCK(&qid->lock);
 		*dispsockp = dispsock;
@@ -791,7 +779,7 @@ get_dispsocket(dns_dispatch_t *disp, isc_sockaddr_t *dest,
 static void
 destroy_dispsocket(dns_dispatch_t *disp, dispsocket_t **dispsockp) {
 	dispsocket_t *dispsock;
-	dns_qid_t *qid;
+	dns_qid_t *qid = DNS_QID(disp);
 
 	/*
 	 * The dispatch must be locked.
@@ -803,19 +791,24 @@ destroy_dispsocket(dns_dispatch_t *disp, dispsocket_t **dispsockp) {
 
 	disp->nsockets--;
 	dispsock->magic = 0;
-	if (dispsock->portentry != NULL)
+	if (dispsock->portentry != NULL) {
+		/* socket_search() tests and dereferences portentry. */
+		LOCK(&qid->lock);
 		deref_portentry(disp, &dispsock->portentry);
-	if (dispsock->socket != NULL)
+		UNLOCK(&qid->lock);
+	}
+	if (dispsock->socket != NULL) {
 		isc_socket_detach(&dispsock->socket);
+	}
 	if (ISC_LINK_LINKED(dispsock, blink)) {
-		qid = DNS_QID(disp);
 		LOCK(&qid->lock);
 		ISC_LIST_UNLINK(qid->sock_table[dispsock->bucket], dispsock,
 				blink);
 		UNLOCK(&qid->lock);
 	}
-	if (dispsock->task != NULL)
+	if (dispsock->task != NULL) {
 		isc_task_detach(&dispsock->task);
+	}
 	isc_mempool_put(disp->mgr->spool, dispsock);
 
 	*dispsockp = NULL;
@@ -828,7 +821,7 @@ destroy_dispsocket(dns_dispatch_t *disp, dispsocket_t **dispsockp) {
 static void
 deactivate_dispsocket(dns_dispatch_t *disp, dispsocket_t *dispsock) {
 	isc_result_t result;
-	dns_qid_t *qid;
+	dns_qid_t *qid = DNS_QID(disp);
 
 	/*
 	 * The dispatch must be locked.
@@ -840,14 +833,16 @@ deactivate_dispsocket(dns_dispatch_t *disp, dispsocket_t *dispsock) {
 	}
 
 	INSIST(dispsock->portentry != NULL);
+	/* socket_search() tests and dereferences portentry. */
+	LOCK(&qid->lock);
 	deref_portentry(disp, &dispsock->portentry);
+	UNLOCK(&qid->lock);
 
 	if (disp->nsockets > DNS_DISPATCH_POOLSOCKS)
 		destroy_dispsocket(disp, &dispsock);
 	else {
 		result = isc_socket_close(dispsock->socket);
 
-		qid = DNS_QID(disp);
 		LOCK(&qid->lock);
 		ISC_LIST_UNLINK(qid->sock_table[dispsock->bucket], dispsock,
 				blink);
-- 
2.23.0