Fix the isc_nm_closedown() to actually close the pending connections

1. The isc__nm_tcp_send() and isc__nm_tcp_read() was not checking
   whether the socket was still alive and scheduling reads/sends on
   closed socket.

2. The isc_nm_read(), isc_nm_send() and isc_nm_resumeread() have been
   changed to always return the error conditions via the callbacks, so
   they always succeed.  This applies to all protocols (UDP, TCP and
   TCPDNS).

bin/named/controlconf.c

@@ -245,14 +245,7 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 
 	isc_nmhandle_detach(&conn->sendhandle);
 
-	result = isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage,
+	isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn);
-					 conn);
-	if (result != ISC_R_SUCCESS) {
-		conn->reading = false;
-		isc_nmhandle_detach(&conn->readhandle);
-		return;
-	}
-
 	return;
 
 cleanup_sendhandle:
@@ -363,11 +356,7 @@ control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 
 	isc_nmhandle_detach(&conn->cmdhandle);
 
-	result = isc_nm_send(conn->sendhandle, &r, control_senddone, conn);
+	isc_nm_send(conn->sendhandle, &r, control_senddone, conn);
-	if (result != ISC_R_SUCCESS) {
-		isc_nmhandle_detach(&conn->sendhandle);
-		conn->sending = false;
-	}
 
 	return;
 
@@ -596,10 +585,9 @@ conn_put(void *arg) {
 	maybe_free_listener(listener);
 }
 
-static isc_result_t
+static void
 newconnection(controllistener_t *listener, isc_nmhandle_t *handle) {
 	controlconnection_t *conn = NULL;
-	isc_result_t result;
 
 	conn = isc_nmhandle_getdata(handle);
 	if (conn == NULL) {
@@ -627,26 +615,7 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) {
 	isc_nmhandle_attach(handle, &conn->readhandle);
 	conn->reading = true;
 
-	result = isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage,
+	isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn);
-					 conn);
-	if (result != ISC_R_SUCCESS) {
-		isc_nmhandle_detach(&conn->readhandle);
-		conn->reading = false;
-		goto cleanup;
-	}
-
-	return (ISC_R_SUCCESS);
-
-cleanup:
-	/*
-	 * conn_reset() handles the cleanup.
-	 */
-#ifdef ENABLE_AFL
-	if (named_g_fuzz_type == isc_fuzz_rndc) {
-		named_fuzz_notify();
-	}
-#endif /* ifdef ENABLE_AFL */
-	return (result);
 }
 
 static isc_result_t
@@ -672,17 +641,7 @@ control_newconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 		return (ISC_R_FAILURE);
 	}
 
-	result = newconnection(listener, handle);
+	newconnection(listener, handle);
-	if (result != ISC_R_SUCCESS) {
-		char socktext[ISC_SOCKADDR_FORMATSIZE];
-		isc_sockaddr_format(&peeraddr, socktext, sizeof(socktext));
-		isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
-			      NAMED_LOGMODULE_CONTROL, ISC_LOG_WARNING,
-			      "dropped command channel from %s: %s", socktext,
-			      isc_result_totext(result));
-		return (result);
-	}
-
 	return (ISC_R_SUCCESS);
 }
 

bin/rndc/rndc.c

@@ -402,13 +402,14 @@ static void
 rndc_recvnonce(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	isccc_ccmsg_t *ccmsg = (isccc_ccmsg_t *)arg;
 	isccc_sexpr_t *response = NULL;
-	isccc_sexpr_t *_ctrl;
+	isc_nmhandle_t *sendhandle = NULL;
+	isccc_sexpr_t *_ctrl = NULL;
 	isccc_region_t source;
 	uint32_t nonce;
 	isccc_sexpr_t *request = NULL;
 	isccc_time_t now;
 	isc_region_t r;
-	isccc_sexpr_t *data;
+	isccc_sexpr_t *data = NULL;
 	isc_buffer_t b;
 
 	REQUIRE(ccmsg != NULL);
@@ -484,13 +485,11 @@ rndc_recvnonce(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 
 	isc_nmhandle_attach(handle, &recvdone_handle);
 	atomic_fetch_add_relaxed(&recvs, 1);
-	DO("schedule recv",
+	isccc_ccmsg_readmessage(ccmsg, rndc_recvdone, ccmsg);
-	   isccc_ccmsg_readmessage(ccmsg, rndc_recvdone, ccmsg));
 
-	isc_nmhandle_t *sendhandle = NULL;
 	isc_nmhandle_attach(handle, &sendhandle);
 	atomic_fetch_add_relaxed(&sends, 1);
-	DO("send message", isc_nm_send(handle, &r, rndc_senddone, sendhandle));
+	isc_nm_send(handle, &r, rndc_senddone, sendhandle);
 
 	REQUIRE(recvnonce_handle == handle);
 	isc_nmhandle_detach(&recvnonce_handle);
@@ -506,11 +505,12 @@ rndc_connected(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	isccc_ccmsg_t *ccmsg = (isccc_ccmsg_t *)arg;
 	char socktext[ISC_SOCKADDR_FORMATSIZE];
 	isccc_sexpr_t *request = NULL;
-	isccc_sexpr_t *data;
+	isccc_sexpr_t *data = NULL;
 	isccc_time_t now;
 	isc_region_t r;
 	isc_buffer_t b;
 	isc_nmhandle_t *connhandle = NULL;
+	isc_nmhandle_t *sendhandle = NULL;
 
 	REQUIRE(ccmsg != NULL);
 
@@ -560,13 +560,11 @@ rndc_connected(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 
 	isc_nmhandle_attach(handle, &recvnonce_handle);
 	atomic_fetch_add_relaxed(&recvs, 1);
-	DO("schedule recv",
+	isccc_ccmsg_readmessage(ccmsg, rndc_recvnonce, ccmsg);
-	   isccc_ccmsg_readmessage(ccmsg, rndc_recvnonce, ccmsg));
 
-	isc_nmhandle_t *sendhandle = NULL;
 	isc_nmhandle_attach(handle, &sendhandle);
 	atomic_fetch_add_relaxed(&sends, 1);
-	DO("send message", isc_nm_send(handle, &r, rndc_senddone, sendhandle));
+	isc_nm_send(handle, &r, rndc_senddone, sendhandle);
 
 	isc_nmhandle_detach(&connhandle);
 	atomic_fetch_sub_release(&connects, 1);

lib/isc/httpd.c

@@ -623,9 +623,8 @@ httpd_put(void *arg) {
 #endif /* ENABLE_AFL */
 }
 
-static isc_result_t
+static void
 new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) {
-	isc_result_t result;
 	isc_httpd_t *httpd = NULL;
 	char *headerdata = NULL;
 
@@ -668,12 +667,7 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) {
 	UNLOCK(&httpdmgr->lock);
 
 	isc_nmhandle_attach(handle, &httpd->readhandle);
-	result = isc_nm_read(handle, httpd_request, httpdmgr);
+	isc_nm_read(handle, httpd_request, httpdmgr);
-	if (result != ISC_R_SUCCESS) {
-		isc_nmhandle_detach(&httpd->readhandle);
-	}
-
-	return (result);
 }
 
 static isc_result_t
@@ -699,7 +693,9 @@ httpd_newconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 		return (ISC_R_FAILURE);
 	}
 
-	return (new_httpd(httpdmgr, handle));
+	new_httpd(httpdmgr, handle);
+
+	return (ISC_R_SUCCESS);
 }
 
 static isc_result_t
@@ -963,13 +959,7 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
 	httpd->state = SEND;
 
 	isc_nmhandle_attach(handle, &httpd->sendhandle);
-	result = isc_nm_send(handle, &r, httpd_senddone, httpd);
+	isc_nm_send(handle, &r, httpd_senddone, httpd);
-	if (result != ISC_R_SUCCESS) {
-		isc_nmhandle_detach(&httpd->sendhandle);
-
-		httpd->state = RECV;
-		isc_nm_resumeread(handle);
-	}
 
 	return;
 cleanup_readhandle:
@@ -1137,12 +1127,12 @@ httpd_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 		}
 	}
 
+	isc_nmhandle_detach(&httpd->sendhandle);
+
 	if (result != ISC_R_SUCCESS) {
 		return;
 	}
 
-	isc_nmhandle_detach(&httpd->sendhandle);
-
 	httpd->state = RECV;
 	isc_nm_resumeread(handle);
 }

lib/isc/include/isc/netmgr.h

@@ -200,7 +200,7 @@ isc_nm_resume(isc_nm_t *mgr);
  * workers to resume.
  */
 
-isc_result_t
+void
 isc_nm_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg);
 /*
  * Begin (or continue) reading on the socket associated with 'handle', and
@@ -228,7 +228,7 @@ isc_nm_cancelread(isc_nmhandle_t *handle);
  * \li	...for which a read/recv callback has been defined.
  */
 
-isc_result_t
+void
 isc_nm_resumeread(isc_nmhandle_t *handle);
 /*%<
  * Resume reading on the handle's socket.
@@ -238,7 +238,7 @@ isc_nm_resumeread(isc_nmhandle_t *handle);
  * \li	...for a socket with a defined read/recv callback.
  */
 
-isc_result_t
+void
 isc_nm_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 	    void *cbarg);
 /*%<

lib/isc/netmgr/netmgr-int.h

@@ -679,7 +679,7 @@ isc__nm_async_shutdown(isc__networker_t *worker, isc__netievent_t *ev0);
  * close on them.
  */
 
-isc_result_t
+void
 isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 		 void *cbarg);
 /*%<
@@ -700,14 +700,14 @@ isc__nm_async_udpsend(isc__networker_t *worker, isc__netievent_t *ev0);
  * Callback handlers for asynchronous UDP events (listen, stoplisten, send).
  */
 
-isc_result_t
+void
 isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 		 void *cbarg);
 /*%<
  * Back-end implementation of isc_nm_send() for TCP handles.
  */
 
-isc_result_t
+void
 isc__nm_tcp_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg);
 /*
  * Back-end implementation of isc_nm_read() for TCP handles.
@@ -724,7 +724,7 @@ isc__nm_tcp_pauseread(isc_nmsocket_t *sock);
  * Pause reading on this socket, while still remembering the callback.
  */
 
-isc_result_t
+void
 isc__nm_tcp_resumeread(isc_nmsocket_t *sock);
 /*%<
  * Resume reading from socket.
@@ -774,7 +774,7 @@ isc__nm_async_tcpclose(isc__networker_t *worker, isc__netievent_t *ev0);
  * stoplisten, send, read, pause, close).
  */
 
-isc_result_t
+void
 isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 		    isc_nm_cb_t cb, void *cbarg);
 /*%<

lib/isc/netmgr/netmgr.c

@@ -429,6 +429,9 @@ isc_nm_destroy(isc_nm_t **mgr0) {
 #endif /* ifdef WIN32 */
 	}
 
+#ifdef NETMGR_TRACE
+	isc__nm_dump_active(mgr);
+#endif
 	INSIST(references == 1);
 
 	/*
@@ -1428,7 +1431,7 @@ isc__nm_uvreq_put(isc__nm_uvreq_t **req0, isc_nmsocket_t *sock) {
 	isc__nmsocket_detach(&sock);
 }
 
-isc_result_t
+void
 isc_nm_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 	    void *cbarg) {
 	REQUIRE(VALID_NMHANDLE(handle));
@@ -1436,24 +1439,28 @@ isc_nm_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 	switch (handle->sock->type) {
 	case isc_nm_udpsocket:
 	case isc_nm_udplistener:
-		return (isc__nm_udp_send(handle, region, cb, cbarg));
+		isc__nm_udp_send(handle, region, cb, cbarg);
+		break;
 	case isc_nm_tcpsocket:
-		return (isc__nm_tcp_send(handle, region, cb, cbarg));
+		isc__nm_tcp_send(handle, region, cb, cbarg);
+		break;
 	case isc_nm_tcpdnssocket:
-		return (isc__nm_tcpdns_send(handle, region, cb, cbarg));
+		isc__nm_tcpdns_send(handle, region, cb, cbarg);
+		break;
 	default:
 		INSIST(0);
 		ISC_UNREACHABLE();
 	}
 }
 
-isc_result_t
+void
 isc_nm_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
 	REQUIRE(VALID_NMHANDLE(handle));
 
 	switch (handle->sock->type) {
 	case isc_nm_tcpsocket:
-		return (isc__nm_tcp_read(handle, cb, cbarg));
+		isc__nm_tcp_read(handle, cb, cbarg);
+		break;
 	default:
 		INSIST(0);
 		ISC_UNREACHABLE();
@@ -1489,7 +1496,7 @@ isc_nm_pauseread(isc_nmhandle_t *handle) {
 	}
 }
 
-isc_result_t
+void
 isc_nm_resumeread(isc_nmhandle_t *handle) {
 	REQUIRE(VALID_NMHANDLE(handle));
 
@@ -1497,7 +1504,8 @@ isc_nm_resumeread(isc_nmhandle_t *handle) {
 
 	switch (sock->type) {
 	case isc_nm_tcpsocket:
-		return (isc__nm_tcp_resumeread(sock));
+		isc__nm_tcp_resumeread(sock);
+		break;
 	default:
 		INSIST(0);
 		ISC_UNREACHABLE();
@@ -1838,7 +1846,8 @@ nmhandle_dump(isc_nmhandle_t *handle) {
 
 static void
 nmsocket_dump(isc_nmsocket_t *sock) {
-	isc_nmhandle_t *handle;
+	isc_nmhandle_t *handle = NULL;
+
 	LOCK(&sock->lock);
 	fprintf(stderr, "\n=================\n");
 	fprintf(stderr, "Active socket %p, type %s, refs %lu\n", sock,
@@ -1850,25 +1859,37 @@ nmsocket_dump(isc_nmsocket_t *sock) {
 	backtrace_symbols_fd(sock->backtrace, sock->backtrace_size,
 			     STDERR_FILENO);
 	fprintf(stderr, "\n");
-	fprintf(stderr, "Active handles:\n");
+
 	for (handle = ISC_LIST_HEAD(sock->active_handles); handle != NULL;
 	     handle = ISC_LIST_NEXT(handle, active_link))
 	{
+		static bool first = true;
+		if (first) {
+			fprintf(stderr, "Active handles:\n");
+			first = false;
+		}
 		nmhandle_dump(handle);
 	}
+
 	fprintf(stderr, "\n");
 	UNLOCK(&sock->lock);
 }
 
 void
 isc__nm_dump_active(isc_nm_t *nm) {
-	isc_nmsocket_t *sock;
+	isc_nmsocket_t *sock = NULL;
+
 	REQUIRE(VALID_NM(nm));
+
 	LOCK(&nm->lock);
-	fprintf(stderr, "Outstanding sockets\n");
 	for (sock = ISC_LIST_HEAD(nm->active_sockets); sock != NULL;
 	     sock = ISC_LIST_NEXT(sock, active_link))
 	{
+		static bool first = true;
+		if (first) {
+			fprintf(stderr, "Outstanding sockets\n");
+			first = false;
+		}
 		nmsocket_dump(sock);
 	}
 	UNLOCK(&nm->lock);

lib/isc/netmgr/tcp.c

@@ -583,11 +583,33 @@ tcp_listenclose_cb(uv_handle_t *handle) {
 	isc__nmsocket_detach(&sock);
 }
 
+static void
+failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) {
+	isc_nm_recv_cb_t cb;
+	void *cbarg = NULL;
+
+	uv_read_stop(&sock->uv_handle.stream);
+
+	if (sock->timer_initialized) {
+		uv_timer_stop(&sock->timer);
+	}
+
+	if (sock->quota) {
+		isc_quota_detach(&sock->quota);
+	}
+
+	cb = sock->recv_cb;
+	cbarg = sock->recv_cbarg;
+	isc__nmsocket_clearcb(sock);
+
+	if (cb != NULL) {
+		cb(sock->statichandle, result, NULL, cbarg);
+	}
+}
+
 static void
 readtimeout_cb(uv_timer_t *handle) {
 	isc_nmsocket_t *sock = uv_handle_get_data((uv_handle_t *)handle);
-	isc_nm_recv_cb_t cb;
-	void *cbarg;
 
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
@@ -604,29 +626,22 @@ readtimeout_cb(uv_timer_t *handle) {
 	/*
 	 * Timeout; stop reading and process whatever we have.
 	 */
-	uv_read_stop(&sock->uv_handle.stream);
+	failed_read_cb(sock, ISC_R_TIMEDOUT);
-	if (sock->quota) {
-		isc_quota_detach(&sock->quota);
-	}
-
-	cb = sock->recv_cb;
-	cbarg = sock->recv_cbarg;
-	isc__nmsocket_clearcb(sock);
-
-	if (cb != NULL) {
-		cb(sock->statichandle, ISC_R_TIMEDOUT, NULL, cbarg);
-	}
 }
 
-isc_result_t
+void
 isc__nm_tcp_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
-	isc_nmsocket_t *sock = NULL;
+	isc_nmsocket_t *sock = handle->sock;
 	isc__netievent_startread_t *ievent = NULL;
 
 	REQUIRE(VALID_NMHANDLE(handle));
 	REQUIRE(VALID_NMSOCK(handle->sock));
 
-	sock = handle->sock;
+	if (!isc__nmsocket_active(sock)) {
+		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_RECVFAIL]);
+		cb(handle, ISC_R_CANCELED, NULL, cbarg);
+		return;
+	}
 
 	REQUIRE(sock->tid == isc_nm_tid());
 	sock->recv_cb = cb;
@@ -644,7 +659,7 @@ isc__nm_tcp_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
 				       (isc__netievent_t *)ievent);
 	}
 
-	return (ISC_R_SUCCESS);
+	return;
 }
 
 /*%<
@@ -678,6 +693,16 @@ isc__nm_async_tcp_startread(isc__networker_t *worker, isc__netievent_t *ev0) {
 	int r;
 
 	REQUIRE(worker->id == isc_nm_tid());
+
+	r = uv_read_start(&sock->uv_handle.stream, tcp_alloc_cb, read_cb);
+	if (r != 0) {
+		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_RECVFAIL]);
+
+		failed_read_cb(sock, ISC_R_CANCELED);
+
+		return;
+	}
+
 	if (sock->read_timeout != 0) {
 		if (!sock->timer_initialized) {
 			uv_timer_init(&worker->loop, &sock->timer);
@@ -687,11 +712,6 @@ isc__nm_async_tcp_startread(isc__networker_t *worker, isc__netievent_t *ev0) {
 		uv_timer_start(&sock->timer, readtimeout_cb, sock->read_timeout,
 			       0);
 	}
-
-	r = uv_read_start(&sock->uv_handle.stream, tcp_alloc_cb, read_cb);
-	if (r != 0) {
-		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_RECVFAIL]);
-	}
 }
 
 isc_result_t
@@ -734,7 +754,7 @@ isc__nm_async_tcp_pauseread(isc__networker_t *worker, isc__netievent_t *ev0) {
 	uv_read_stop(&sock->uv_handle.stream);
 }
 
-isc_result_t
+void
 isc__nm_tcp_resumeread(isc_nmsocket_t *sock) {
 	isc__netievent_startread_t *ievent = NULL;
 
@@ -742,11 +762,16 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) {
 	REQUIRE(sock->tid == isc_nm_tid());
 
 	if (sock->recv_cb == NULL) {
-		return (ISC_R_CANCELED);
+		return;
+	}
+
+	if (!isc__nmsocket_active(sock)) {
+		failed_read_cb(sock, ISC_R_CANCELED);
+		return;
 	}
 
 	if (!atomic_load(&sock->readpaused)) {
-		return (ISC_R_SUCCESS);
+		return;
 	}
 
 	atomic_store(&sock->readpaused, false);
@@ -762,26 +787,21 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) {
 		isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
 				       (isc__netievent_t *)ievent);
 	}
-
-	return (ISC_R_SUCCESS);
 }
 
 static void
 read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
 	isc_nmsocket_t *sock = uv_handle_get_data((uv_handle_t *)stream);
-	isc_nm_recv_cb_t cb;
-	void *cbarg;
 
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
 	REQUIRE(buf != NULL);
 
-	cb = sock->recv_cb;
-	cbarg = sock->recv_cbarg;
-
 	if (nread >= 0) {
 		isc_region_t region = { .base = (unsigned char *)buf->base,
 					.length = nread };
+		isc_nm_recv_cb_t cb = sock->recv_cb;
+		void *cbarg = sock->recv_cbarg;
 
 		if (cb != NULL) {
 			cb(sock->statichandle, ISC_R_SUCCESS, &region, cbarg);
@@ -796,30 +816,19 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
 			uv_timer_start(&sock->timer, readtimeout_cb,
 				       sock->read_timeout, 0);
 		}
-
+	} else {
-		isc__nm_free_uvbuf(sock, buf);
-		return;
-	}
-
-	isc__nm_free_uvbuf(sock, buf);
-
 		/*
-	 * This might happen if the inner socket is closing.  It means that
+		 * This might happen if the inner socket is closing.  It means
-	 * it's detached, so the socket will be closed.
+		 * that it's detached, so the socket will be closed.
 		 */
 		if (nread != UV_EOF) {
-		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_RECVFAIL]);
+			isc__nm_incstats(sock->mgr,
-	}
+					 sock->statsindex[STATID_RECVFAIL]);
-	if (cb != NULL) {
-		isc__nmsocket_clearcb(sock);
-		cb(sock->statichandle, ISC_R_EOF, NULL, cbarg);
 		}
 
-	/*
+		failed_read_cb(sock, ISC_R_EOF);
-	 * We don't need to clean up now; the socket will be closed and
+	}
-	 * resources and quota reclaimed when handle is freed in
+	isc__nm_free_uvbuf(sock, buf);
-	 * isc__nm_tcp_close().
-	 */
 }
 
 static void
@@ -985,7 +994,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
 	return (ISC_R_SUCCESS);
 }
 
-isc_result_t
+void
 isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 		 void *cbarg) {
 	isc_nmsocket_t *sock = handle->sock;
@@ -994,10 +1003,18 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 
 	REQUIRE(sock->type == isc_nm_tcpsocket);
 
+	if (!isc__nmsocket_active(sock)) {
+		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_SENDFAIL]);
+		cb(handle, ISC_R_CANCELED, cbarg);
+		return;
+	}
+
 	uvreq = isc__nm_uvreq_get(sock->mgr, sock);
 	uvreq->uvbuf.base = (char *)region->base;
 	uvreq->uvbuf.len = region->length;
+
 	isc_nmhandle_attach(handle, &uvreq->handle);
+
 	uvreq->cb.send = cb;
 	uvreq->cbarg = cbarg;
 
@@ -1024,7 +1041,7 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 		isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
 				       (isc__netievent_t *)ievent);
 	}
-	return (ISC_R_SUCCESS);
+	return;
 }
 
 static void
@@ -1082,8 +1099,6 @@ tcp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	r = uv_write(&req->uv_req.write, &sock->uv_handle.stream, &req->uvbuf,
 		     1, tcp_send_cb);
 	if (r < 0) {
-		req->cb.send(NULL, isc__nm_uverr2result(r), req->cbarg);
-		isc__nm_uvreq_put(&req, sock);
 		return (isc__nm_uverr2result(r));
 	}
 
@@ -1121,6 +1136,7 @@ tcp_close_direct(isc_nmsocket_t *sock) {
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
 	REQUIRE(sock->type == isc_nm_tcpsocket);
+
 	if (sock->quota != NULL) {
 		isc_quota_detach(&sock->quota);
 	}
@@ -1128,8 +1144,11 @@ tcp_close_direct(isc_nmsocket_t *sock) {
 	uv_read_stop((uv_stream_t *)&sock->uv_handle.handle);
 
 	if (sock->timer_initialized) {
-		sock->timer_initialized = false;
 		uv_timer_stop(&sock->timer);
+	}
+
+	if (sock->timer_initialized) {
+		sock->timer_initialized = false;
 		uv_close((uv_handle_t *)&sock->timer, timer_close_cb);
 	} else {
 		uv_close(&sock->uv_handle.handle, tcp_close_cb);
@@ -1170,17 +1189,14 @@ isc__nm_tcp_shutdown(isc_nmsocket_t *sock) {
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
 
-	if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) {
+	if (!isc__nmsocket_active(sock)) {
-		isc_nm_recv_cb_t cb;
+		return;
-		void *cbarg;
-
-		cb = sock->recv_cb;
-		cbarg = sock->recv_cbarg;
-		isc__nmsocket_clearcb(sock);
-
-		if (cb != NULL) {
-			cb(sock->statichandle, ISC_R_CANCELED, NULL, cbarg);
 	}
+
+	atomic_store(&sock->active, false);
+
+	if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) {
+		failed_read_cb(sock, ISC_R_CANCELED);
 	}
 }
 
@@ -1197,13 +1213,6 @@ isc__nm_tcp_cancelread(isc_nmhandle_t *handle) {
 	REQUIRE(sock->tid == isc_nm_tid());
 
 	if (atomic_load(&sock->client)) {
-		isc_nm_recv_cb_t cb;
+		failed_read_cb(sock, ISC_R_EOF);
-		void *cbarg;
-
-		cb = sock->recv_cb;
-		cbarg = sock->recv_cbarg;
-		isc__nmsocket_clearcb(sock);
-
-		cb(handle, ISC_R_EOF, NULL, cbarg);
 	}
 }

lib/isc/netmgr/tcpdns.c

@@ -159,10 +159,7 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 	 * the connection is closed or there is no more data to be read.
 	 */
 	isc_nmhandle_attach(handle, &readhandle);
-	result = isc_nm_read(readhandle, dnslisten_readcb, dnssock);
+	isc_nm_read(readhandle, dnslisten_readcb, dnssock);
-	if (result != ISC_R_SUCCESS) {
-		isc_nmhandle_detach(&readhandle);
-	}
 	isc__nmsocket_detach(&dnssock);
 
 	return (ISC_R_SUCCESS);
@@ -492,10 +489,7 @@ resume_processing(void *arg) {
 			}
 			isc_nmhandle_detach(&handle);
 		} else if (sock->outerhandle != NULL) {
-			result = isc_nm_resumeread(sock->outerhandle);
+			isc_nm_resumeread(sock->outerhandle);
-			if (result != ISC_R_SUCCESS) {
-				isc_nmhandle_detach(&sock->outerhandle);
-			}
 		}
 
 		return;
@@ -542,7 +536,6 @@ tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 
 void
 isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) {
-	isc_result_t result;
 	isc__netievent_tcpdnssend_t *ievent =
 		(isc__netievent_tcpdnssend_t *)ev0;
 	isc__nm_uvreq_t *req = ievent->req;
@@ -551,7 +544,6 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) {
 	REQUIRE(worker->id == sock->tid);
 	REQUIRE(sock->tid == isc_nm_tid());
 
-	result = ISC_R_NOTCONNECTED;
 	if (atomic_load(&sock->active) && sock->outerhandle != NULL) {
 		isc_nmhandle_t *sendhandle = NULL;
 		isc_region_t r;
@@ -559,23 +551,19 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) {
 		r.base = (unsigned char *)req->uvbuf.base;
 		r.length = req->uvbuf.len;
 		isc_nmhandle_attach(sock->outerhandle, &sendhandle);
-		result = isc_nm_send(sendhandle, &r, tcpdnssend_cb, req);
+		isc_nm_send(sendhandle, &r, tcpdnssend_cb, req);
-		if (result != ISC_R_SUCCESS) {
+	} else {
-			isc_nmhandle_detach(&sendhandle);
+		req->cb.send(req->handle, ISC_R_CANCELED, req->cbarg);
-		}
+		isc_mem_put(req->sock->mgr->mctx, req->uvbuf.base,
-	}
+			    req->uvbuf.len);
-
+		isc__nm_uvreq_put(&req, req->handle->sock);
-	if (result != ISC_R_SUCCESS) {
-		req->cb.send(req->handle, result, req->cbarg);
-		isc_mem_put(sock->mgr->mctx, req->uvbuf.base, req->uvbuf.len);
-		isc__nm_uvreq_put(&req, sock);
 	}
 }
 
 /*
  * isc__nm_tcp_send sends buf to a peer on a socket.
  */
-isc_result_t
+void
 isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 		    isc_nm_cb_t cb, void *cbarg) {
 	isc__nm_uvreq_t *uvreq = NULL;
@@ -587,6 +575,11 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->type == isc_nm_tcpdnssocket);
 
+	if (!isc__nmsocket_active(sock)) {
+		cb(handle, ISC_R_CANCELED, cbarg);
+		return;
+	}
+
 	uvreq = isc__nm_uvreq_get(sock->mgr, sock);
 	isc_nmhandle_attach(handle, &uvreq->handle);
 	uvreq->cb.send = cb;
@@ -598,7 +591,6 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 	memmove(uvreq->uvbuf.base + 2, region->base, region->length);
 
 	if (sock->tid == isc_nm_tid()) {
-		isc_result_t result;
 		isc_nmhandle_t *sendhandle = NULL;
 		isc_region_t r;
 
@@ -606,12 +598,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 		r.length = uvreq->uvbuf.len;
 
 		isc_nmhandle_attach(sock->outerhandle, &sendhandle);
-		result = isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb,
+		isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb, uvreq);
-				     uvreq);
-		if (result != ISC_R_SUCCESS) {
-			isc_nmhandle_detach(&sendhandle);
-		}
-		return (result);
 	} else {
 		isc__netievent_tcpdnssend_t *ievent = NULL;
 
@@ -621,11 +608,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 
 		isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
 				       (isc__netievent_t *)ievent);
-
-		return (ISC_R_SUCCESS);
 	}
-
-	return (ISC_R_UNEXPECTED);
 }
 
 static void

lib/isc/netmgr/udp.c

@@ -407,7 +407,7 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf,
  * a proper sibling/child socket so that we won't have to jump to another
  * thread.
  */
-isc_result_t
+void
 isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 		 void *cbarg) {
 	isc_nmsocket_t *sock = handle->sock;
@@ -418,6 +418,12 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 	uint32_t maxudp = atomic_load(&sock->mgr->maxudp);
 	int ntid;
 
+	if (!isc__nmsocket_active(sock)) {
+		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_SENDFAIL]);
+		cb(handle, ISC_R_CANCELED, cbarg);
+		return;
+	}
+
 	/*
 	 * We're simulating a firewall blocking UDP packets bigger than
 	 * 'maxudp' bytes, for testing purposes.
@@ -428,7 +434,7 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 	 */
 	if (maxudp != 0 && region->length > maxudp) {
 		isc_nmhandle_detach(&handle);
-		return (ISC_R_SUCCESS);
+		return;
 	}
 
 	if (sock->type == isc_nm_udpsocket && !atomic_load(&sock->client)) {
@@ -441,10 +447,6 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 		ISC_UNREACHABLE();
 	}
 
-	if (!isc__nmsocket_active(sock)) {
-		return (ISC_R_CANCELED);
-	}
-
 	/*
 	 * If we're in the network thread, we can send directly.  If the
 	 * handle is associated with a UDP socket, we can reuse its thread
@@ -474,12 +476,14 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 
 	if (isc_nm_tid() == rsock->tid) {
 		/*
-		 * If we're in the same thread as the socket we can send the
+		 * If we're in the same thread as the socket we can send
-		 * data directly, but we need to emulate returning the error via
+		 * the data directly, but we still need to return errors
-		 * the callback, not directly to keep the API.
+		 * via the callback for API consistency.
 		 */
 		isc_result_t result = udp_send_direct(rsock, uvreq, peer);
 		if (result != ISC_R_SUCCESS) {
+			isc__nm_incstats(rsock->mgr,
+					 rsock->statsindex[STATID_SENDFAIL]);
 			uvreq->cb.send(uvreq->handle, result, uvreq->cbarg);
 			isc__nm_uvreq_put(&uvreq, sock);
 		}
@@ -495,8 +499,6 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 		isc__nm_enqueue_ievent(&sock->mgr->workers[rsock->tid],
 				       (isc__netievent_t *)ievent);
 	}
-
-	return (ISC_R_SUCCESS);
 }
 
 /*

lib/isccc/ccmsg.c

@@ -133,10 +133,8 @@ isccc_ccmsg_setmaxsize(isccc_ccmsg_t *ccmsg, unsigned int maxsize) {
 	ccmsg->maxsize = maxsize;
 }
 
-isc_result_t
+void
 isccc_ccmsg_readmessage(isccc_ccmsg_t *ccmsg, isc_nm_cb_t cb, void *cbarg) {
-	isc_result_t result;
-
 	REQUIRE(VALID_CCMSG(ccmsg));
 
 	if (ccmsg->buffer != NULL) {
@@ -149,16 +147,11 @@ isccc_ccmsg_readmessage(isccc_ccmsg_t *ccmsg, isc_nm_cb_t cb, void *cbarg) {
 	ccmsg->length_received = false;
 
 	if (ccmsg->reading) {
-		result = isc_nm_resumeread(ccmsg->handle);
+		isc_nm_resumeread(ccmsg->handle);
 	} else {
-		result = isc_nm_read(ccmsg->handle, recv_data, ccmsg);
+		isc_nm_read(ccmsg->handle, recv_data, ccmsg);
 		ccmsg->reading = true;
 	}
-	if (result != ISC_R_SUCCESS) {
-		ccmsg->reading = false;
-	}
-
-	return (result);
 }
 
 void

lib/isccc/include/isccc/ccmsg.h

@@ -87,7 +87,7 @@ isccc_ccmsg_setmaxsize(isccc_ccmsg_t *ccmsg, unsigned int maxsize);
  *\li	512 <= "maxsize" <= 4294967296
  */
 
-isc_result_t
+void
 isccc_ccmsg_readmessage(isccc_ccmsg_t *ccmsg, isc_nm_cb_t cb, void *cbarg);
 /*%
  * Schedule an event to be delivered when a command channel message is
@@ -97,11 +97,6 @@ isccc_ccmsg_readmessage(isccc_ccmsg_t *ccmsg, isc_nm_cb_t cb, void *cbarg);
  *
  *\li	"ccmsg" be valid.
  *
- * Returns:
- *
- *\li	#ISC_R_SUCCESS		-- no error
- *\li	Anything that the isc_nm_read() call can return.
- *
  * Notes:
  *
  *\li	The event delivered is a fully generic event.  It will contain no

lib/ns/client.c

@@ -323,20 +323,15 @@ client_allocsendbuf(ns_client_t *client, isc_buffer_t *buffer,
 	*datap = data;
 }
 
-static isc_result_t
+static void
 client_sendpkg(ns_client_t *client, isc_buffer_t *buffer) {
-	isc_result_t result;
 	isc_region_t r;
 
 	REQUIRE(client->sendhandle == NULL);
 
 	isc_buffer_usedregion(buffer, &r);
 	isc_nmhandle_attach(client->handle, &client->sendhandle);
-	result = isc_nm_send(client->handle, &r, client_senddone, client);
+	isc_nm_send(client->handle, &r, client_senddone, client);
-	if (result != ISC_R_SUCCESS) {
-		isc_nmhandle_detach(&client->sendhandle);
-	}
-	return (result);
 }
 
 void
@@ -375,11 +370,9 @@ ns_client_sendraw(ns_client_t *client, dns_message_t *message) {
 	r.base[0] = (client->message->id >> 8) & 0xff;
 	r.base[1] = client->message->id & 0xff;
 
-	result = client_sendpkg(client, &buffer);
+	client_sendpkg(client, &buffer);
-	if (result == ISC_R_SUCCESS) {
-		return;
-	}
 
+	return;
 done:
 	if (client->tcpbuf != NULL) {
 		isc_mem_put(client->mctx, client->tcpbuf,
@@ -455,7 +448,7 @@ ns_client_send(ns_client_t *client) {
 		result = ns_client_addopt(client, client->message,
 					  &client->opt);
 		if (result != ISC_R_SUCCESS) {
-			goto done;
+			goto cleanup;
 		}
 	}
 
@@ -463,7 +456,7 @@ ns_client_send(ns_client_t *client) {
 
 	result = dns_compress_init(&cctx, -1, client->mctx);
 	if (result != ISC_R_SUCCESS) {
-		goto done;
+		goto cleanup;
 	}
 	if (client->peeraddr_valid && client->view != NULL) {
 		isc_netaddr_t netaddr;
@@ -489,7 +482,7 @@ ns_client_send(ns_client_t *client) {
 
 	result = dns_message_renderbegin(client->message, &cctx, &buffer);
 	if (result != ISC_R_SUCCESS) {
-		goto done;
+		goto cleanup;
 	}
 
 	if (client->opt != NULL) {
@@ -497,7 +490,7 @@ ns_client_send(ns_client_t *client) {
 		opt_included = true;
 		client->opt = NULL;
 		if (result != ISC_R_SUCCESS) {
-			goto done;
+			goto cleanup;
 		}
 	}
 	result = dns_message_rendersection(client->message,
@@ -507,7 +500,7 @@ ns_client_send(ns_client_t *client) {
 		goto renderend;
 	}
 	if (result != ISC_R_SUCCESS) {
-		goto done;
+		goto cleanup;
 	}
 	/*
 	 * Stop after the question if TC was set for rate limiting.
@@ -523,7 +516,7 @@ ns_client_send(ns_client_t *client) {
 		goto renderend;
 	}
 	if (result != ISC_R_SUCCESS) {
-		goto done;
+		goto cleanup;
 	}
 	result = dns_message_rendersection(
 		client->message, DNS_SECTION_AUTHORITY,
@@ -533,18 +526,18 @@ ns_client_send(ns_client_t *client) {
 		goto renderend;
 	}
 	if (result != ISC_R_SUCCESS) {
-		goto done;
+		goto cleanup;
 	}
 	result = dns_message_rendersection(client->message,
 					   DNS_SECTION_ADDITIONAL,
 					   preferred_glue | render_opts);
 	if (result != ISC_R_SUCCESS && result != ISC_R_NOSPACE) {
-		goto done;
+		goto cleanup;
 	}
 renderend:
 	result = dns_message_renderend(client->message);
 	if (result != ISC_R_SUCCESS) {
-		goto done;
+		goto cleanup;
 	}
 
 #ifdef HAVE_DNSTAP
@@ -552,13 +545,14 @@ renderend:
 	if (((client->message->flags & DNS_MESSAGEFLAG_AA) != 0) &&
 	    (client->query.authzone != NULL))
 	{
+		isc_result_t eresult;
 		isc_buffer_t b;
 		dns_name_t *zo = dns_zone_getorigin(client->query.authzone);
 
 		isc_buffer_init(&b, zone, sizeof(zone));
 		dns_compress_setmethods(&cctx, DNS_COMPRESS_NONE);
-		result = dns_name_towire(zo, &cctx, &b);
+		eresult = dns_name_towire(zo, &cctx, &b);
-		if (result == ISC_R_SUCCESS) {
+		if (eresult == ISC_R_SUCCESS) {
 			isc_buffer_usedregion(&b, &zr);
 		}
 	}
@@ -574,7 +568,6 @@ renderend:
 
 	if (cleanup_cctx) {
 		dns_compress_invalidate(&cctx);
-		cleanup_cctx = false;
 	}
 
 	if (client->sendcb != NULL) {
@@ -591,7 +584,7 @@ renderend:
 
 		respsize = isc_buffer_usedlength(&buffer);
 
-		result = client_sendpkg(client, &buffer);
+		client_sendpkg(client, &buffer);
 
 		switch (isc_sockaddr_pf(&client->peeraddr)) {
 		case AF_INET:
@@ -621,7 +614,7 @@ renderend:
 
 		respsize = isc_buffer_usedlength(&buffer);
 
-		result = client_sendpkg(client, &buffer);
+		client_sendpkg(client, &buffer);
 
 		switch (isc_sockaddr_pf(&client->peeraddr)) {
 		case AF_INET:
@@ -660,11 +653,9 @@ renderend:
 				   ns_statscounter_truncatedresp);
 	}
 
-	if (result == ISC_R_SUCCESS) {
 	return;
-	}
 
-done:
+cleanup:
 	if (client->tcpbuf != NULL) {
 		isc_mem_put(client->mctx, client->tcpbuf,
 			    NS_CLIENT_TCP_BUFFER_SIZE);

lib/ns/xfrout.c

@@ -1567,8 +1567,8 @@ sendstream(xfrout_ctx_t *xfr) {
 
 		isc_nmhandle_attach(xfr->client->handle,
 				    &xfr->client->sendhandle);
-		CHECK(isc_nm_send(xfr->client->sendhandle, &used,
+		isc_nm_send(xfr->client->sendhandle, &used, xfrout_senddone,
-				  xfrout_senddone, xfr));
+			    xfr);
 		xfr->sends++;
 		xfr->cbytes = used.length;
 	} else {