Merge branch '3400-gracefully-handle-the-errors-from-uv_start_read-v9_18' into 'v9_18'

Gracefully handle uv_read_start() failures [v9.18]

See merge request isc-projects/bind9!6426

CHANGES

@@ -1,3 +1,8 @@
+5905.	[bug]		When the TCP connection would be closed/reset between
+			the connect/accept and the read, the uv_read_start()
+			return value would be unexpected and cause an assertion
+			failure. [GL #3400]
+
 5904.	[func]		Changed dnssec-signzone -H default to 0 additional
 			NSEC3 iterations. [GL #3395]
 

doc/notes/notes-current.rst

@@ -46,3 +46,6 @@ Bug Fixes
 - It was possible for a catalog zone consumer to process a catalog zone member
   zone when there was a configured pre-existing forward-only forward zone with
   the same name. This has been fixed. :gl:`#2506`.
+
+- Fix the assertion failure caused by TCP connection closing between the
+  connect (or accept) and the read from the socket. :gl:`#3400`

lib/isc/netmgr/netmgr-int.h

@@ -2130,11 +2130,11 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf);
 void
 isc__nm_tlsdns_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf);
 
-void
+isc_result_t
 isc__nm_start_reading(isc_nmsocket_t *sock);
 void
 isc__nm_stop_reading(isc_nmsocket_t *sock);
-void
+isc_result_t
 isc__nm_process_sock_buffer(isc_nmsocket_t *sock);
 void
 isc__nm_resume_processing(void *arg);

lib/isc/netmgr/netmgr.c

@@ -2201,39 +2201,42 @@ isc__nm_alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf) {
 	worker->recvbuf_inuse = true;
 }
 
-void
+isc_result_t
 isc__nm_start_reading(isc_nmsocket_t *sock) {
+	isc_result_t result = ISC_R_SUCCESS;
 	int r;
 
 	if (atomic_load(&sock->reading)) {
-		return;
+		return (ISC_R_SUCCESS);
 	}
 
 	switch (sock->type) {
 	case isc_nm_udpsocket:
 		r = uv_udp_recv_start(&sock->uv_handle.udp, isc__nm_alloc_cb,
 				      isc__nm_udp_read_cb);
-		UV_RUNTIME_CHECK(uv_udp_recv_start, r);
 		break;
 	case isc_nm_tcpsocket:
 		r = uv_read_start(&sock->uv_handle.stream, isc__nm_alloc_cb,
 				  isc__nm_tcp_read_cb);
-		UV_RUNTIME_CHECK(uv_read_start, r);
 		break;
 	case isc_nm_tcpdnssocket:
 		r = uv_read_start(&sock->uv_handle.stream, isc__nm_alloc_cb,
 				  isc__nm_tcpdns_read_cb);
-		UV_RUNTIME_CHECK(uv_read_start, r);
 		break;
 	case isc_nm_tlsdnssocket:
 		r = uv_read_start(&sock->uv_handle.stream, isc__nm_alloc_cb,
 				  isc__nm_tlsdns_read_cb);
-		UV_RUNTIME_CHECK(uv_read_start, r);
 		break;
 	default:
 		UNREACHABLE();
 	}
-	atomic_store(&sock->reading, true);
+	if (r != 0) {
+		result = isc__nm_uverr2result(r);
+	} else {
+		atomic_store(&sock->reading, true);
+	}
+
+	return (result);
 }
 
 void
@@ -2295,7 +2298,7 @@ processbuffer(isc_nmsocket_t *sock) {
  * has been set to sequential mode. In this case we'll be called again
  * later by isc__nm_resume_processing().
  */
-void
+isc_result_t
 isc__nm_process_sock_buffer(isc_nmsocket_t *sock) {
 	for (;;) {
 		int_fast32_t ah = atomic_load(&sock->ah);
@@ -2306,7 +2309,10 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) {
 			 * Don't reset the timer until we have a
 			 * full DNS message.
 			 */
-			isc__nm_start_reading(sock);
+			result = isc__nm_start_reading(sock);
+			if (result != ISC_R_SUCCESS) {
+				return (result);
+			}
 			/*
 			 * Start the timer only if there are no externally used
 			 * active handles, there's always one active handle
@@ -2316,11 +2322,11 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) {
 			if (ah == 1) {
 				isc__nmsocket_timer_start(sock);
 			}
-			return;
+			goto done;
 		case ISC_R_CANCELED:
 			isc__nmsocket_timer_stop(sock);
 			isc__nm_stop_reading(sock);
-			return;
+			goto done;
 		case ISC_R_SUCCESS:
 			/*
 			 * Stop the timer on the successful message read, this
@@ -2332,13 +2338,15 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) {
 			if (atomic_load(&sock->client) ||
 			    atomic_load(&sock->sequential)) {
 				isc__nm_stop_reading(sock);
-				return;
+				goto done;
 			}
 			break;
 		default:
 			UNREACHABLE();
 		}
 	}
+done:
+	return (ISC_R_SUCCESS);
 }
 
 void

lib/isc/netmgr/tcp.c

@@ -751,18 +751,24 @@ isc__nm_async_tcpstartread(isc__networker_t *worker, isc__netievent_t *ev0) {
 	isc__netievent_tcpstartread_t *ievent =
 		(isc__netievent_tcpstartread_t *)ev0;
 	isc_nmsocket_t *sock = ievent->sock;
+	isc_result_t result;
 
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
 	UNUSED(worker);
 
 	if (isc__nmsocket_closing(sock)) {
+		result = ISC_R_CANCELED;
+	} else {
+		result = isc__nm_start_reading(sock);
+	}
+
+	if (result != ISC_R_SUCCESS) {
 		atomic_store(&sock->reading, true);
-		isc__nm_tcp_failed_read_cb(sock, ISC_R_CANCELED);
+		isc__nm_tcp_failed_read_cb(sock, result);
 		return;
 	}
 
-	isc__nm_start_reading(sock);
 	isc__nmsocket_timer_start(sock);
 }
 

lib/isc/netmgr/tcpdns.c

@@ -718,6 +718,7 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) {
 	isc__netievent_tcpdnsread_t *ievent =
 		(isc__netievent_tcpdnsread_t *)ev0;
 	isc_nmsocket_t *sock = ievent->sock;
+	isc_result_t result;
 
 	UNUSED(worker);
 
@@ -725,12 +726,15 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) {
 	REQUIRE(sock->tid == isc_nm_tid());
 
 	if (isc__nmsocket_closing(sock)) {
-		atomic_store(&sock->reading, true);
-		isc__nm_failed_read_cb(sock, ISC_R_CANCELED, false);
-		return;
+		result = ISC_R_CANCELED;
+	} else {
+		result = isc__nm_process_sock_buffer(sock);
 	}
 
-	isc__nm_process_sock_buffer(sock);
+	if (result != ISC_R_SUCCESS) {
+		atomic_store(&sock->reading, true);
+		isc__nm_failed_read_cb(sock, result, false);
+	}
 }
 
 /*
@@ -840,6 +844,7 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread,
 	isc_nmsocket_t *sock = uv_handle_get_data((uv_handle_t *)stream);
 	uint8_t *base = NULL;
 	size_t len;
+	isc_result_t result;
 
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
@@ -882,7 +887,10 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread,
 		sock->read_timeout = atomic_load(&sock->mgr->idle);
 	}
 
-	isc__nm_process_sock_buffer(sock);
+	result = isc__nm_process_sock_buffer(sock);
+	if (result != ISC_R_SUCCESS) {
+		isc__nm_failed_read_cb(sock, result, true);
+	}
 free:
 	if (nread < 0) {
 		/*
@@ -1034,7 +1042,12 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
 	 * prep_destroy()->tcpdns_close_direct().
 	 */
 	isc_nmhandle_attach(handle, &csock->recv_handle);
-	isc__nm_process_sock_buffer(csock);
+	result = isc__nm_process_sock_buffer(csock);
+	if (result != ISC_R_SUCCESS) {
+		isc_nmhandle_detach(&csock->recv_handle);
+		isc_nmhandle_detach(&handle);
+		goto failure;
+	}
 
 	/*
 	 * The initial timer has been set, update the read timeout for the next

lib/isc/netmgr/tlsdns.c

@@ -303,7 +303,11 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) {
 	/* Setting pending req */
 	sock->tls.pending_req = req;
 
-	isc__nm_process_sock_buffer(sock);
+	result = isc__nm_process_sock_buffer(sock);
+	if (result != ISC_R_SUCCESS) {
+		sock->tls.pending_req = NULL;
+		goto error;
+	}
 
 	result = tls_cycle(sock);
 	if (result != ISC_R_SUCCESS) {
@@ -1043,8 +1047,10 @@ tls_cycle_input(isc_nmsocket_t *sock) {
 				/*
 				 * Process what's in the buffer so far
 				 */
-				isc__nm_process_sock_buffer(sock);
-
+				result = isc__nm_process_sock_buffer(sock);
+				if (result != ISC_R_SUCCESS) {
+					goto failure;
+				}
 				/*
 				 * FIXME: Should we call
 				 * isc__nm_failed_read_cb()?
@@ -1056,7 +1062,10 @@ tls_cycle_input(isc_nmsocket_t *sock) {
 
 			sock->buf_len += len;
 
-			isc__nm_process_sock_buffer(sock);
+			result = isc__nm_process_sock_buffer(sock);
+			if (result != ISC_R_SUCCESS) {
+				goto failure;
+			}
 		}
 	} else if (!SSL_is_init_finished(sock->tls.tls)) {
 		if (SSL_is_server(sock->tls.tls)) {
@@ -1078,7 +1087,10 @@ tls_cycle_input(isc_nmsocket_t *sock) {
 		if (sock->tls.state == TLS_STATE_NONE &&
 		    !SSL_is_init_finished(sock->tls.tls)) {
 			sock->tls.state = TLS_STATE_HANDSHAKE;
-			isc__nm_process_sock_buffer(sock);
+			result = isc__nm_process_sock_buffer(sock);
+			if (result != ISC_R_SUCCESS) {
+				goto failure;
+			}
 		}
 		/* else continue reading */
 		break;
@@ -1630,7 +1642,10 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
 
 	isc_nmhandle_detach(&handle);
 
-	isc__nm_process_sock_buffer(csock);
+	result = isc__nm_process_sock_buffer(csock);
+	if (result != ISC_R_SUCCESS) {
+		goto failure;
+	}
 
 	/*
 	 * sock is now attached to the handle.

lib/isc/netmgr/udp.c

@@ -1120,7 +1120,7 @@ void
 isc__nm_async_udpread(isc__networker_t *worker, isc__netievent_t *ev0) {
 	isc__netievent_udpread_t *ievent = (isc__netievent_udpread_t *)ev0;
 	isc_nmsocket_t *sock = ievent->sock;
-	isc_result_t result = ISC_R_SUCCESS;
+	isc_result_t result;
 
 	UNUSED(worker);
 
@@ -1131,6 +1131,8 @@ isc__nm_async_udpread(isc__networker_t *worker, isc__netievent_t *ev0) {
 		result = ISC_R_SHUTTINGDOWN;
 	} else if (isc__nmsocket_closing(sock)) {
 		result = ISC_R_CANCELED;
+	} else {
+		result = isc__nm_start_reading(sock);
 	}
 
 	if (result != ISC_R_SUCCESS) {
@@ -1139,7 +1141,6 @@ isc__nm_async_udpread(isc__networker_t *worker, isc__netievent_t *ev0) {
 		return;
 	}
 
-	isc__nm_start_reading(sock);
 	isc__nmsocket_timer_start(sock);
 }