Merge branch 'fix-shutdown-issues' into 'master'

Fix shutdown issues

See merge request isc-projects/bind9!2907

lib/isc/netmgr/netmgr-int.h

@@ -603,6 +603,13 @@ isc__nmsocket_prep_destroy(isc_nmsocket_t *sock);
  * if there are no remaining references or active handles.
  */
 
+bool
+isc__nmsocket_active(isc_nmsocket_t *sock);
+/*%<
+ * Determine whether 'sock' is active by checking 'sock->active'
+ * or, for child sockets, 'sock->parent->active'.
+ */
+
 void
 isc__nm_async_closecb(isc__networker_t *worker, isc__netievent_t *ev0);
 /*%<

lib/isc/netmgr/netmgr.c

@@ -681,7 +681,7 @@ isc__nm_enqueue_ievent(isc__networker_t *worker, isc__netievent_t *event) {
 	uv_async_send(&worker->async);
 }
 
-static bool
+bool
 isc__nmsocket_active(isc_nmsocket_t *sock) {
 	REQUIRE(VALID_NMSOCK(sock));
 	if (sock->parent != NULL) {
@@ -1158,7 +1158,7 @@ nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) {
 
 void
 isc_nmhandle_unref(isc_nmhandle_t *handle) {
-	isc_nmsocket_t *sock = NULL;
+	isc_nmsocket_t *sock = NULL, *tmp = NULL;
 
 	REQUIRE(VALID_NMHANDLE(handle));
 
@@ -1199,8 +1199,14 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
 		}
 	}
 
+	/*
+	 * Temporarily reference the socket to ensure that it can't
+	 * be deleted by another thread while we're deactivating the
+	 * handle.
+	 */
+	isc_nmsocket_attach(sock, &tmp);
 	nmhandle_deactivate(sock, handle);
-	nmsocket_maybe_destroy(sock);
+	isc_nmsocket_detach(&tmp);
 }
 
 void *

lib/isc/netmgr/tcp.c

@@ -419,7 +419,7 @@ stoplistening(isc_nmsocket_t *sock) {
 		event = isc__nm_get_ievent(sock->mgr, netievent_tcpchildstop);
 		isc_nmsocket_attach(&sock->children[i], &event->sock);
 
-		if (i == sock->tid) {
+		if (isc_nm_tid() == sock->children[i].tid) {
 			isc__nm_async_tcpchildstop(&sock->mgr->workers[i],
 						   (isc__netievent_t *) event);
 			isc__nm_put_ievent(sock->mgr, event);

lib/isc/netmgr/tcpdns.c

@@ -470,8 +470,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 	REQUIRE(sock->type == isc_nm_tcpdnssocket);
 
 	if (sock->outer == NULL) {
-		/* The socket is closed, just issue the callback */
-		cb(handle, ISC_R_FAILURE, cbarg);
+		/* The socket is closed */
 		return (ISC_R_NOTCONNECTED);
 	}
 

lib/isc/netmgr/udp.c

@@ -122,6 +122,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 	REQUIRE(sock->type == isc_nm_udpsocket);
 	REQUIRE(sock->iface != NULL);
 	REQUIRE(sock->parent != NULL);
+	REQUIRE(sock->tid == isc_nm_tid());
 
 	uv_udp_init(&worker->loop, &sock->uv_handle.udp);
 	uv_handle_set_data(&sock->uv_handle.handle, NULL);
@@ -164,7 +165,8 @@ udp_close_cb(uv_handle_t *handle) {
 
 static void
 stop_udp_child(isc_nmsocket_t *sock) {
-	INSIST(sock->type == isc_nm_udpsocket);
+	REQUIRE(sock->type == isc_nm_udpsocket);
+	REQUIRE(sock->tid == isc_nm_tid());
 
 	uv_udp_recv_stop(&sock->uv_handle.udp);
 	uv_close((uv_handle_t *) &sock->uv_handle.udp, udp_close_cb);
@@ -179,19 +181,25 @@ stop_udp_child(isc_nmsocket_t *sock) {
 
 static void
 stoplistening(isc_nmsocket_t *sock) {
+	REQUIRE(sock->type == isc_nm_udplistener);
+
 	/*
 	 * Socket is already closing; there's nothing to do.
 	 */
-	if (uv_is_closing((uv_handle_t *) &sock->uv_handle.udp)) {
+	if (!isc__nmsocket_active(sock)) {
 		return;
 	}
 
-	INSIST(sock->type == isc_nm_udplistener);
+	/*
+	 * Mark it inactive now so that all sends will be ignored
+	 * and we won't try to stop listening again.
+	 */
+	atomic_store(&sock->active, false);
 
 	for (int i = 0; i < sock->nchildren; i++) {
 		isc__netievent_udpstop_t *event = NULL;
 
-		if (i == sock->tid) {
+		if (isc_nm_tid() == sock->children[i].tid) {
 			stop_udp_child(&sock->children[i]);
 			continue;
 		}
@@ -358,7 +366,11 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region,
 
 	/*
 	 * Simulate a firewall blocking UDP packets bigger than
-	 * 'maxudp' bytes.
+	 * 'maxudp' bytes, for testing purposes.
+	 *
+	 * The client would ordinarily have unreferenced the handle
+	 * in the callback, but that won't happen in this case, so
+	 * we need to do so here.
 	 */
 	if (maxudp != 0 && region->length > maxudp) {
 		isc_nmhandle_unref(handle);
@@ -371,8 +383,12 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region,
 	} else if (sock->type == isc_nm_udplistener) {
 		psock = sock;
 	} else {
-		isc_nmhandle_unref(handle);
-		return (ISC_R_UNEXPECTED);
+		INSIST(0);
+		ISC_UNREACHABLE();
+	}
+
+	if (!isc__nmsocket_active(sock)) {
+		return (ISC_R_CANCELED);
 	}
 
 	if (isc__nm_in_netthread()) {
@@ -425,7 +441,7 @@ isc__nm_async_udpsend(isc__networker_t *worker, isc__netievent_t *ev0) {
 
 	REQUIRE(worker->id == ievent->sock->tid);
 
-	if (atomic_load(&ievent->sock->active)) {
+	if (isc__nmsocket_active(ievent->sock)) {
 		udp_send_direct(ievent->sock, ievent->req, &ievent->peer);
 	} else {
 		ievent->req->cb.send(ievent->req->handle,

lib/ns/client.c

@@ -381,8 +381,9 @@ ns_client_sendraw(ns_client_t *client, dns_message_t *message) {
 	r.base[1] = client->message->id & 0xff;
 
 	result = client_sendpkg(client, &buffer);
-	if (result == ISC_R_SUCCESS)
+	if (result == ISC_R_SUCCESS) {
 		return;
+	}
 
  done:
 	if (client->tcpbuf != NULL) {
@@ -392,6 +393,7 @@ ns_client_sendraw(ns_client_t *client, dns_message_t *message) {
 	}
 
 	ns_client_drop(client, result);
+	isc_nmhandle_unref(client->handle);
 }
 
 void
@@ -596,6 +598,10 @@ ns_client_send(ns_client_t *client) {
 
 		isc_nmhandle_ref(client->handle);
 		result = client_sendpkg(client, &tcpbuffer);
+		if (result != ISC_R_SUCCESS) {
+			/* We won't get a callback to clean it up */
+			isc_nmhandle_unref(client->handle);
+		}
 
 		switch (isc_sockaddr_pf(&client->peeraddr)) {
 		case AF_INET:
@@ -629,6 +635,10 @@ ns_client_send(ns_client_t *client) {
 
 		isc_nmhandle_ref(client->handle);
 		result = client_sendpkg(client, &buffer);
+		if (result != ISC_R_SUCCESS) {
+			/* We won't get a callback to clean it up */
+			isc_nmhandle_unref(client->handle);
+		}
 
 		switch (isc_sockaddr_pf(&client->peeraddr)) {
 		case AF_INET:
@@ -2333,6 +2343,16 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) {
 		isc_task_detach(&client->task);
 	}
 
+	if (client->manager != NULL) {
+		clientmgr_detach(&client->manager);
+	}
+	if (client->mctx != NULL) {
+		isc_mem_detach(&client->mctx);
+	}
+	if (client->sctx != NULL) {
+		ns_server_detach(&client->sctx);
+	}
+
 	return (result);
 }
 

lib/ns/xfrout.c

@@ -1663,8 +1663,6 @@ xfrout_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 		xfrout_fail(xfr, result, "send");
 	} else if (xfr->end_of_stream == false) {
 		sendstream(xfr);
-		/* Return now so we don't unref the handle */
-		return;
 	} else {
 		/* End of zone transfer stream. */
 		uint64_t msecs, persec;
@@ -1690,9 +1688,9 @@ xfrout_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 			   (unsigned int) persec);
 
 		xfrout_ctx_destroy(&xfr);
+		/* We're done, unreference the handle */
+		isc_nmhandle_unref(handle);
 	}
-
-	isc_nmhandle_unref(handle);
 }
 
 static void