From afca2e3b2198eb43a97fe6edb3c5a46915389e09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 20 Oct 2020 08:07:44 +0200 Subject: [PATCH 01/11] Fix the way udp_send_direct() is used There were two problems how udp_send_direct() was used: 1. The udp_send_direct() can return ISC_R_CANCELED (or translated error from uv_udp_send()), but the isc__nm_async_udpsend() wasn't checking the error code and not releasing the uvreq in case of an error. 2. In isc__nm_udp_send(), when the UDP send is already in the right netthread, it uses udp_send_direct() to send the UDP packet right away. When that happened the uvreq was not freed, and the error code was returned to the caller. We need to return ISC_R_SUCCESS and rather use the callback to report an error in such case. --- lib/isc/netmgr/udp.c | 47 ++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index f87ab95a5f..ed49b47274 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -475,9 +475,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 - * data directly + * data directly, but we need to emulate returning the error via + * the callback, not directly to keep the API. */ - return (udp_send_direct(rsock, uvreq, peer)); + isc_result_t result = udp_send_direct(rsock, uvreq, peer); + if (result != ISC_R_SUCCESS) { + uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); + isc__nm_uvreq_put(&uvreq, sock); + } } else { /* * We need to create an event and pass it using async channel @@ -489,8 +494,9 @@ 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); } + + return (ISC_R_SUCCESS); } /* @@ -498,16 +504,19 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, */ void isc__nm_async_udpsend(isc__networker_t *worker, isc__netievent_t *ev0) { + isc_result_t result; isc__netievent_udpsend_t *ievent = (isc__netievent_udpsend_t *)ev0; + isc_nmsocket_t *sock = ievent->sock; + isc__nm_uvreq_t *uvreq = ievent->req; - REQUIRE(worker->id == ievent->sock->tid); + REQUIRE(sock->type == isc_nm_udpsocket); + REQUIRE(worker->id == sock->tid); - if (isc__nmsocket_active(ievent->sock)) { - udp_send_direct(ievent->sock, ievent->req, &ievent->peer); - } else { - ievent->req->cb.send(ievent->req->handle, ISC_R_CANCELED, - ievent->req->cbarg); - isc__nm_uvreq_put(&ievent->req, ievent->req->sock); + result = udp_send_direct(sock, uvreq, &ievent->peer); + if (result != ISC_R_SUCCESS) { + isc__nm_incstats(sock->mgr, sock->statsindex[STATID_SENDFAIL]); + uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); + isc__nm_uvreq_put(&uvreq, sock); } } @@ -515,14 +524,14 @@ static void udp_send_cb(uv_udp_send_t *req, int status) { isc_result_t result = ISC_R_SUCCESS; isc__nm_uvreq_t *uvreq = (isc__nm_uvreq_t *)req->data; + isc_nmsocket_t *sock = uvreq->sock; REQUIRE(VALID_UVREQ(uvreq)); REQUIRE(VALID_NMHANDLE(uvreq->handle)); if (status < 0) { result = isc__nm_uverr2result(status); - isc__nm_incstats(uvreq->sock->mgr, - uvreq->sock->statsindex[STATID_SENDFAIL]); + isc__nm_incstats(sock->mgr, sock->statsindex[STATID_SENDFAIL]); } uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); @@ -537,8 +546,10 @@ static isc_result_t udp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, isc_sockaddr_t *peer) { const struct sockaddr *sa = NULL; - int rv; + int r; + REQUIRE(VALID_NMSOCK(sock)); + REQUIRE(VALID_UVREQ(req)); REQUIRE(sock->tid == isc_nm_tid()); REQUIRE(sock->type == isc_nm_udpsocket); @@ -547,12 +558,10 @@ udp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, } sa = atomic_load(&sock->connected) ? NULL : &peer->type.sa; - rv = uv_udp_send(&req->uv_req.udp_send, &sock->uv_handle.udp, - &req->uvbuf, 1, sa, udp_send_cb); - if (rv < 0) { - isc__nm_incstats(req->sock->mgr, - req->sock->statsindex[STATID_SENDFAIL]); - return (isc__nm_uverr2result(rv)); + r = uv_udp_send(&req->uv_req.udp_send, &sock->uv_handle.udp, + &req->uvbuf, 1, sa, udp_send_cb); + if (r < 0) { + return (isc__nm_uverr2result(r)); } return (ISC_R_SUCCESS); From ae9a6befa802a10fb08fbe100ee96dd03ecb9787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Tue, 20 Oct 2020 12:55:58 +0200 Subject: [PATCH 02/11] Don't crash if isc_uv_export returns an error in accept_connection. isc_uv_export can return an error - e.g. EMFILE (from dup), handle this nicely. --- lib/isc/netmgr/tcp.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 483bd6d12f..7aef685058 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -933,7 +934,9 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { if (r != 0) { result = isc__nm_uverr2result(r); uv_close((uv_handle_t *)uvstream, free_uvtcpt); - isc_quota_detach("a); + if (quota != NULL) { + isc_quota_detach("a); + } return (result); } @@ -942,6 +945,17 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { event = isc__nm_get_ievent(ssock->mgr, netievent_tcpchildaccept); /* Duplicate the server socket */ + r = isc_uv_export((uv_stream_t *)uvstream, &event->streaminfo); + if (r != 0) { + result = isc_errno_toresult(errno); + uv_close((uv_handle_t *)uvstream, free_uvtcpt); + if (quota != NULL) { + isc_quota_detach("a); + } + isc__nm_put_ievent(ssock->mgr, event); + return (result); + } + isc_nmsocket_t *csock = isc_mem_get(ssock->mgr->mctx, sizeof(isc_nmsocket_t)); isc__nmsocket_init(csock, ssock->mgr, isc_nm_tcpsocket, ssock->iface); @@ -954,9 +968,6 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { event->sock = csock; event->quota = quota; - r = isc_uv_export((uv_stream_t *)uvstream, &event->streaminfo); - RUNTIME_CHECK(r == 0); - uv_close((uv_handle_t *)uvstream, free_uvtcpt); if (w == isc_nm_tid()) { From ff0a336d524c249bf144b121b53d1a3fa66a626d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Tue, 20 Oct 2020 16:06:54 +0200 Subject: [PATCH 03/11] Proper handling of socket references in case of TCP conn failure. --- lib/isc/netmgr/tcp.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 7aef685058..6c9bcb47d1 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -135,12 +135,15 @@ isc__nm_async_tcpconnect(isc__networker_t *worker, isc__netievent_t *ev0) { if (r != 0) { /* We need to issue callbacks ourselves */ tcp_connect_cb(&req->uv_req.connect, r); - goto done; + LOCK(&sock->lock); + SIGNAL(&sock->cond); + UNLOCK(&sock->lock); + isc__nmsocket_detach(&sock); + return; } atomic_store(&sock->connected, true); -done: LOCK(&sock->lock); SIGNAL(&sock->cond); UNLOCK(&sock->lock); @@ -161,6 +164,7 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) { if (status != 0) { req->cb.connect(NULL, isc__nm_uverr2result(status), req->cbarg); isc__nm_uvreq_put(&req, sock); + isc__nmsocket_detach(&sock); return; } @@ -243,7 +247,6 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, if (nsock->result != ISC_R_SUCCESS) { result = nsock->result; - isc__nmsocket_detach(&nsock); } isc__nmsocket_detach(&tmp); From 97b33e5bde296feb82c1d8bb703e56c1b224e129 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 20 Oct 2020 20:57:19 +0200 Subject: [PATCH 04/11] Explicitly stop reading before closing the nmtcpsocket When closing the socket that is actively reading from the stream, the read_cb() could be called between uv_close() and close callback when the server socket has been already detached hence using sock->statichandle after it has been already freed. --- lib/isc/netmgr/tcp.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 6c9bcb47d1..b935b94dbd 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -1115,6 +1115,9 @@ tcp_close_direct(isc_nmsocket_t *sock) { if (sock->quota != NULL) { isc_quota_detach(&sock->quota); } + + uv_read_stop((uv_stream_t *)&sock->uv_handle.handle); + if (sock->timer_initialized) { sock->timer_initialized = false; uv_timer_stop(&sock->timer); From d72bc3eb52ca973556077bb65fea3664315cc469 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 20 Oct 2020 21:03:27 +0200 Subject: [PATCH 05/11] Detach the sock->server in uv_close() callback, not before --- lib/isc/netmgr/tcp.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index b935b94dbd..6328893b9c 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -1092,6 +1092,11 @@ tcp_close_cb(uv_handle_t *uvhandle) { isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CLOSE]); atomic_store(&sock->closed, true); atomic_store(&sock->connected, false); + + if (sock->server != NULL) { + isc__nmsocket_detach(&sock->server); + } + isc__nmsocket_prep_destroy(sock); } @@ -1101,9 +1106,6 @@ timer_close_cb(uv_handle_t *uvhandle) { REQUIRE(VALID_NMSOCK(sock)); - if (sock->server != NULL) { - isc__nmsocket_detach(&sock->server); - } uv_close(&sock->uv_handle.handle, tcp_close_cb); } @@ -1123,9 +1125,6 @@ tcp_close_direct(isc_nmsocket_t *sock) { uv_timer_stop(&sock->timer); uv_close((uv_handle_t *)&sock->timer, timer_close_cb); } else { - if (sock->server != NULL) { - isc__nmsocket_detach(&sock->server); - } uv_close(&sock->uv_handle.handle, tcp_close_cb); } } From 6af08d1ca6a105ee8cb6ead64c4fe34b9154d257 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 21 Oct 2020 08:56:21 +0200 Subject: [PATCH 06/11] Fix the way tcp_send_direct() is used There were two problems how tcp_send_direct() was used: 1. The tcp_send_direct() can return ISC_R_CANCELED (or translated error from uv_tcp_send()), but the isc__nm_async_tcpsend() wasn't checking the error code and not releasing the uvreq in case of an error. 2. In isc__nm_tcp_send(), when the TCP send is already in the right netthread, it uses tcp_send_direct() to send the TCP packet right away. When that happened the uvreq was not freed, and the error code was returned to the caller. We need to return ISC_R_SUCCESS and rather use the callback to report an error in such case. --- lib/isc/netmgr/tcp.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 6328893b9c..872c137f0a 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -1006,7 +1006,13 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, * If we're in the same thread as the socket we can send the * data directly */ - return (tcp_send_direct(sock, uvreq)); + isc_result_t result = tcp_send_direct(sock, uvreq); + if (result != ISC_R_SUCCESS) { + isc__nm_incstats(sock->mgr, + sock->statsindex[STATID_SENDFAIL]); + uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); + isc__nm_uvreq_put(&uvreq, sock); + } } else { /* * We need to create an event and pass it using async channel @@ -1014,32 +1020,28 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpsend); ievent->sock = sock; ievent->req = uvreq; + isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid], (isc__netievent_t *)ievent); - return (ISC_R_SUCCESS); } - - return (ISC_R_UNEXPECTED); + return (ISC_R_SUCCESS); } static void tcp_send_cb(uv_write_t *req, int status) { isc_result_t result = ISC_R_SUCCESS; isc__nm_uvreq_t *uvreq = (isc__nm_uvreq_t *)req->data; - isc_nmsocket_t *sock = NULL; + isc_nmsocket_t *sock = uvreq->sock; REQUIRE(VALID_UVREQ(uvreq)); REQUIRE(VALID_NMHANDLE(uvreq->handle)); if (status < 0) { result = isc__nm_uverr2result(status); - isc__nm_incstats(uvreq->sock->mgr, - uvreq->sock->statsindex[STATID_SENDFAIL]); + isc__nm_incstats(sock->mgr, sock->statsindex[STATID_SENDFAIL]); } uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); - - sock = uvreq->handle->sock; isc__nm_uvreq_put(&uvreq, sock); } @@ -1050,18 +1052,17 @@ void isc__nm_async_tcpsend(isc__networker_t *worker, isc__netievent_t *ev0) { isc_result_t result; isc__netievent_tcpsend_t *ievent = (isc__netievent_tcpsend_t *)ev0; + isc_nmsocket_t *sock = ievent->sock; + isc__nm_uvreq_t *uvreq = ievent->req; + REQUIRE(sock->type == isc_nm_tcpsocket); REQUIRE(worker->id == ievent->sock->tid); - if (!atomic_load(&ievent->sock->active)) { - return; - } - - result = tcp_send_direct(ievent->sock, ievent->req); + result = tcp_send_direct(sock, uvreq); if (result != ISC_R_SUCCESS) { - ievent->req->cb.send(ievent->req->handle, result, - ievent->req->cbarg); - isc__nm_uvreq_put(&ievent->req, ievent->req->handle->sock); + isc__nm_incstats(sock->mgr, sock->statsindex[STATID_SENDFAIL]); + uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); + isc__nm_uvreq_put(&uvreq, sock); } } @@ -1069,12 +1070,18 @@ static isc_result_t tcp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { int r; + REQUIRE(VALID_NMSOCK(sock)); + REQUIRE(VALID_UVREQ(req)); REQUIRE(sock->tid == isc_nm_tid()); REQUIRE(sock->type == isc_nm_tcpsocket); + + if (!isc__nmsocket_active(sock)) { + return (ISC_R_CANCELED); + } + r = uv_write(&req->uv_req.write, &sock->uv_handle.stream, &req->uvbuf, 1, tcp_send_cb); if (r < 0) { - isc__nm_incstats(sock->mgr, sock->statsindex[STATID_SENDFAIL]); req->cb.send(NULL, isc__nm_uverr2result(r), req->cbarg); isc__nm_uvreq_put(&req, sock); return (isc__nm_uverr2result(r)); From f7c82e406e83f98d3a1fecedc78725b441552092 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 21 Oct 2020 12:52:09 +0200 Subject: [PATCH 07/11] 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 | 51 +--------- bin/rndc/rndc.c | 20 ++-- lib/isc/httpd.c | 26 ++--- lib/isc/include/isc/netmgr.h | 6 +- lib/isc/netmgr/netmgr-int.h | 10 +- lib/isc/netmgr/netmgr.c | 45 ++++++--- lib/isc/netmgr/tcp.c | 163 +++++++++++++++++--------------- lib/isc/netmgr/tcpdns.c | 47 +++------ lib/isc/netmgr/udp.c | 24 ++--- lib/isccc/ccmsg.c | 13 +-- lib/isccc/include/isccc/ccmsg.h | 7 +- lib/ns/client.c | 49 ++++------ lib/ns/xfrout.c | 4 +- 13 files changed, 203 insertions(+), 262 deletions(-) diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index deb1e7af67..4bfa6af4c1 100644 --- a/bin/named/controlconf.c +++ b/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, - conn); - if (result != ISC_R_SUCCESS) { - conn->reading = false; - isc_nmhandle_detach(&conn->readhandle); - return; - } - + isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn); 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); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&conn->sendhandle); - conn->sending = false; - } + isc_nm_send(conn->sendhandle, &r, control_senddone, conn); 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, - 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); + isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn); } 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); - 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); - } - + newconnection(listener, handle); return (ISC_R_SUCCESS); } diff --git a/bin/rndc/rndc.c b/bin/rndc/rndc.c index 170fd1fff2..a8f7277a01 100644 --- a/bin/rndc/rndc.c +++ b/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); diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index 5ba984d406..ca304e973b 100644 --- a/lib/isc/httpd.c +++ b/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); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&httpd->readhandle); - } - - return (result); + isc_nm_read(handle, httpd_request, httpdmgr); } 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); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&httpd->sendhandle); - - httpd->state = RECV; - isc_nm_resumeread(handle); - } + isc_nm_send(handle, &r, httpd_senddone, httpd); 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); } diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index dc4f977e42..95822d1c78 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/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); /*%< diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index b9a9672783..dd1366970a 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/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); /*%< diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 8358ee4e61..4e945ba082 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/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); diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 872c137f0a..5129e1d4a5 100644 --- a/lib/isc/netmgr/tcp.c +++ b/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); - 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); - } + failed_read_cb(sock, ISC_R_TIMEDOUT); } -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, ®ion, 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 { + /* + * This might happen if the inner socket is closing. It means + * 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_free_uvbuf(sock, buf); - return; + failed_read_cb(sock, ISC_R_EOF); } - isc__nm_free_uvbuf(sock, buf); - - /* - * This might happen if the inner socket is closing. It means that - * it's detached, so the socket will be closed. - */ - if (nread != UV_EOF) { - isc__nm_incstats(sock->mgr, sock->statsindex[STATID_RECVFAIL]); - } - if (cb != NULL) { - isc__nmsocket_clearcb(sock); - cb(sock->statichandle, ISC_R_EOF, NULL, cbarg); - } - - /* - * 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_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 (!isc__nmsocket_active(sock)) { + return; + } + + atomic_store(&sock->active, false); + if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) { - isc_nm_recv_cb_t cb; - 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); - } + 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; - void *cbarg; - - cb = sock->recv_cb; - cbarg = sock->recv_cbarg; - isc__nmsocket_clearcb(sock); - - cb(handle, ISC_R_EOF, NULL, cbarg); + failed_read_cb(sock, ISC_R_EOF); } } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 283f77665e..dd1513f4f5 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/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); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&readhandle); - } + isc_nm_read(readhandle, dnslisten_readcb, dnssock); 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); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&sock->outerhandle); - } + isc_nm_resumeread(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); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&sendhandle); - } - } - - 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_send(sendhandle, &r, tcpdnssend_cb, req); + } else { + 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); } } /* * 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, - uvreq); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&sendhandle); - } - return (result); + isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb, uvreq); } 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 diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index ed49b47274..d11b13bfe6 100644 --- a/lib/isc/netmgr/udp.c +++ b/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 - * data directly, but we need to emulate returning the error via - * the callback, not directly to keep the API. + * If we're in the same thread as the socket we can send + * the data directly, but we still need to return errors + * 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); } /* diff --git a/lib/isccc/ccmsg.c b/lib/isccc/ccmsg.c index 3cb0732f94..649b3f332b 100644 --- a/lib/isccc/ccmsg.c +++ b/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 diff --git a/lib/isccc/include/isccc/ccmsg.h b/lib/isccc/include/isccc/ccmsg.h index a793a96bfc..c5ea35d088 100644 --- a/lib/isccc/include/isccc/ccmsg.h +++ b/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 diff --git a/lib/ns/client.c b/lib/ns/client.c index a2eed9250d..cf9f5dc137 100644 --- a/lib/ns/client.c +++ b/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); - if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&client->sendhandle); - } - return (result); + isc_nm_send(client->handle, &r, client_senddone, client); } 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); - if (result == ISC_R_SUCCESS) { - return; - } + client_sendpkg(client, &buffer); + 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); - if (result == ISC_R_SUCCESS) { + eresult = dns_name_towire(zo, &cctx, &b); + 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; - } + return; -done: +cleanup: if (client->tcpbuf != NULL) { isc_mem_put(client->mctx, client->tcpbuf, NS_CLIENT_TCP_BUFFER_SIZE); diff --git a/lib/ns/xfrout.c b/lib/ns/xfrout.c index 9eb283ff00..0b2d1c4f8c 100644 --- a/lib/ns/xfrout.c +++ b/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, - xfrout_senddone, xfr)); + isc_nm_send(xfr->client->sendhandle, &used, xfrout_senddone, + xfr); xfr->sends++; xfr->cbytes = used.length; } else { From 5ef71c420fc118b506ad5a16bdb4a21fafdcda9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 22 Oct 2020 00:17:03 +0200 Subject: [PATCH 08/11] Ignore and don't log ISC_R_NOTCONNECTED from uv_accept() When client disconnects before the connection can be accepted, the named would log a spurious log message: error: Accepting TCP connection failed: socket is not connected We now ignore the ISC_R_NOTCONNECTED result code and log only other errors --- lib/isc/netmgr/tcp.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 5129e1d4a5..ef575ee502 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -517,9 +517,17 @@ error: if (sock->quota != NULL) { isc_quota_detach(&sock->quota); } - isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR, - ISC_LOG_ERROR, "Accepting TCP connection failed: %s", - isc_result_totext(result)); + + switch (result) { + case ISC_R_NOTCONNECTED: + /* IGNORE: The client disconnected before we could accept */ + break; + default: + isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, + ISC_LOGMODULE_NETMGR, ISC_LOG_ERROR, + "Accepting TCP connection failed: %s", + isc_result_totext(result)); + } /* * Detach the socket properly to make sure uv_close() is called. From 8797e5efd50c3d7ededa152fb8cc6eeb8ace57ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 22 Oct 2020 10:07:56 +0200 Subject: [PATCH 09/11] Fix the data race when read-writing sock->active by using cmpxchg --- lib/isc/include/isc/netmgr.h | 2 +- lib/isc/netmgr/netmgr-int.h | 14 +++++++++++++- lib/isc/netmgr/netmgr.c | 22 ++++++++++++++++++---- lib/isc/netmgr/tcp.c | 27 +++++++++++++-------------- lib/isc/netmgr/tcpdns.c | 2 +- lib/isc/netmgr/udp.c | 10 +++------- 6 files changed, 49 insertions(+), 28 deletions(-) diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index 95822d1c78..8843cbd54f 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -208,7 +208,7 @@ isc_nm_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg); * is data to process. */ -isc_result_t +void isc_nm_pauseread(isc_nmhandle_t *handle); /*%< * Pause reading on this handle's socket, but remember the callback. diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index dd1366970a..99ce506987 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -660,6 +660,18 @@ isc__nmsocket_active(isc_nmsocket_t *sock); * or, for child sockets, 'sock->parent->active'. */ +bool +isc__nmsocket_deactivate(isc_nmsocket_t *sock); +/*%< + * @brief Deactivate active socket + * + * Atomically deactive the socket by setting @p sock->active or, for child + * sockets, @p sock->parent->active to @c false + * + * @param[in] sock - valid nmsocket + * @return @c false if the socket was already inactive, @c true otherwise + */ + void isc__nmsocket_clearcb(isc_nmsocket_t *sock); /*%< @@ -718,7 +730,7 @@ isc__nm_tcp_close(isc_nmsocket_t *sock); /*%< * Close a TCP socket. */ -isc_result_t +void isc__nm_tcp_pauseread(isc_nmsocket_t *sock); /*%< * Pause reading on this socket, while still remembering the callback. diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 4e945ba082..cb6b18911c 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -731,6 +731,19 @@ isc__nmsocket_active(isc_nmsocket_t *sock) { return (atomic_load(&sock->active)); } +bool +isc__nmsocket_deactivate(isc_nmsocket_t *sock) { + REQUIRE(VALID_NMSOCK(sock)); + + if (sock->parent != NULL) { + return (atomic_compare_exchange_strong(&sock->parent->active, + &(bool){ true }, false)); + } + + return (atomic_compare_exchange_strong(&sock->active, &(bool){ true }, + false)); +} + void isc__nmsocket_attach(isc_nmsocket_t *sock, isc_nmsocket_t **target) { REQUIRE(VALID_NMSOCK(sock)); @@ -1380,7 +1393,7 @@ isc__nm_uvreq_get(isc_nm_t *mgr, isc_nmsocket_t *sock) { REQUIRE(VALID_NM(mgr)); REQUIRE(VALID_NMSOCK(sock)); - if (sock != NULL && atomic_load(&sock->active)) { + if (sock != NULL && isc__nmsocket_active(sock)) { /* Try to reuse one */ req = isc_astack_pop(sock->inactivereqs); } @@ -1419,7 +1432,7 @@ isc__nm_uvreq_put(isc__nm_uvreq_t **req0, isc_nmsocket_t *sock) { handle = req->handle; req->handle = NULL; - if (!atomic_load(&sock->active) || + if (!isc__nmsocket_active(sock) || !isc_astack_trypush(sock->inactivereqs, req)) { isc_mempool_put(sock->mgr->reqpool, req); } @@ -1481,7 +1494,7 @@ isc_nm_cancelread(isc_nmhandle_t *handle) { } } -isc_result_t +void isc_nm_pauseread(isc_nmhandle_t *handle) { REQUIRE(VALID_NMHANDLE(handle)); @@ -1489,7 +1502,8 @@ isc_nm_pauseread(isc_nmhandle_t *handle) { switch (sock->type) { case isc_nm_tcpsocket: - return (isc__nm_tcp_pauseread(sock)); + isc__nm_tcp_pauseread(sock); + break; default: INSIST(0); ISC_UNREACHABLE(); diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index ef575ee502..43d3f9fc9f 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -722,17 +722,17 @@ isc__nm_async_tcp_startread(isc__networker_t *worker, isc__netievent_t *ev0) { } } -isc_result_t +void isc__nm_tcp_pauseread(isc_nmsocket_t *sock) { isc__netievent_pauseread_t *ievent = NULL; REQUIRE(VALID_NMSOCK(sock)); - if (atomic_load(&sock->readpaused)) { - return (ISC_R_SUCCESS); + if (!atomic_compare_exchange_strong(&sock->readpaused, &(bool){ false }, + true)) { + return; } - atomic_store(&sock->readpaused, true); ievent = isc__nm_get_ievent(sock->mgr, netievent_tcppauseread); ievent->sock = sock; @@ -745,7 +745,7 @@ isc__nm_tcp_pauseread(isc_nmsocket_t *sock) { (isc__netievent_t *)ievent); } - return (ISC_R_SUCCESS); + return; } void @@ -778,12 +778,11 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) { return; } - if (!atomic_load(&sock->readpaused)) { + if (!atomic_compare_exchange_strong(&sock->readpaused, &(bool){ true }, + false)) { return; } - atomic_store(&sock->readpaused, false); - ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpstartread); ievent->sock = sock; @@ -903,9 +902,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { REQUIRE(VALID_NMSOCK(ssock)); - if (!atomic_load_relaxed(&ssock->active) || - atomic_load_relaxed(&ssock->mgr->closing)) - { + if (!isc__nmsocket_active(ssock) || atomic_load(&ssock->mgr->closing)) { /* We're closing, bail */ if (quota != NULL) { isc_quota_detach("a); @@ -1197,12 +1194,14 @@ isc__nm_tcp_shutdown(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - if (!isc__nmsocket_active(sock)) { + /* + * If the socket is active, mark it inactive and + * continue. If it isn't active, stop now. + */ + if (!isc__nmsocket_deactivate(sock)) { return; } - atomic_store(&sock->active, false); - if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) { failed_read_cb(sock, ISC_R_CANCELED); } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index dd1513f4f5..6a20ea4c7c 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -544,7 +544,7 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) { REQUIRE(worker->id == sock->tid); REQUIRE(sock->tid == isc_nm_tid()); - if (atomic_load(&sock->active) && sock->outerhandle != NULL) { + if (isc__nmsocket_active(sock) && sock->outerhandle != NULL) { isc_nmhandle_t *sendhandle = NULL; isc_region_t r; diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index d11b13bfe6..d97a0b612a 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -262,16 +262,12 @@ isc__nm_udp_stoplistening(isc_nmsocket_t *sock) { REQUIRE(sock->type == isc_nm_udplistener); /* - * Socket is already closing; there's nothing to do. + * If the socket is active, mark it inactive and + * continue. If it isn't active, stop now. */ - if (!isc__nmsocket_active(sock)) { + if (!isc__nmsocket_deactivate(sock)) { return; } - /* - * 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); /* * If the manager is interlocked, re-enqueue this as an asynchronous From 64e56a9704e7dbe7579b7f1dbc71e0bf57028975 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 22 Oct 2020 12:32:18 +0200 Subject: [PATCH 10/11] Postpone the isc_app_shutdown() after rndc response has been sent When `rndc stop` is received, the isc_app_shutdown() was being called before response to the rndc client has been sent; as the isc_app_shutdown() also tears down the netmgr, the message was never sent and rndc would complain about connection being interrupted in the middle of the transaction. We now postpone the shutdown after the rndc response has been sent. --- bin/named/control.c | 6 ++---- bin/named/controlconf.c | 27 +++++++++++++++++++-------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/bin/named/control.c b/bin/named/control.c index 3320a8fe9d..55a469925d 100644 --- a/bin/named/control.c +++ b/bin/named/control.c @@ -183,8 +183,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly, /* Do not flush master files */ named_server_flushonshutdown(named_g_server, false); named_os_shutdownmsg(cmdline, *text); - isc_app_shutdown(); - result = ISC_R_SUCCESS; + result = ISC_R_SHUTTINGDOWN; } else if (command_compare(command, NAMED_COMMAND_STOP)) { /* * "stop" is the same as "halt" except it does @@ -201,8 +200,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly, #endif /* ifdef HAVE_LIBSCF */ named_server_flushonshutdown(named_g_server, true); named_os_shutdownmsg(cmdline, *text); - isc_app_shutdown(); - result = ISC_R_SUCCESS; + result = ISC_R_SHUTTINGDOWN; } else if (command_compare(command, NAMED_COMMAND_ADDZONE) || command_compare(command, NAMED_COMMAND_MODZONE)) { diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index 4bfa6af4c1..871cb459b7 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -225,6 +226,11 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { conn->sending = false; + if (conn->result == ISC_R_SHUTTINGDOWN) { + isc_app_shutdown(); + goto cleanup_sendhandle; + } + if (atomic_load_acquire(&listener->controls->shuttingdown) || result == ISC_R_CANCELED) { @@ -284,12 +290,12 @@ conn_cleanup(controlconnection_t *conn) { } static void -control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) { - controlconnection_t *conn = (controlconnection_t *)arg; +control_respond(isc_nmhandle_t *handle, controlconnection_t *conn) { controllistener_t *listener = conn->listener; isccc_sexpr_t *data = NULL; isc_buffer_t b; isc_region_t r; + isc_result_t result; result = isccc_cc_createresponse(conn->request, conn->now, conn->now + 60, &conn->response); @@ -297,17 +303,22 @@ control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) { goto cleanup; } + if (conn->result == ISC_R_SHUTTINGDOWN) { + result = ISC_R_SUCCESS; + } else { + result = conn->result; + } + data = isccc_alist_lookup(conn->response, "_data"); if (data != NULL) { - if (isccc_cc_defineuint32(data, "result", conn->result) == NULL) - { + if (isccc_cc_defineuint32(data, "result", result) == NULL) { goto cleanup; } } - if (conn->result != ISC_R_SUCCESS) { + if (result != ISC_R_SUCCESS) { if (data != NULL) { - const char *estr = isc_result_totext(conn->result); + const char *estr = isc_result_totext(result); if (isccc_cc_definestring(data, "err", estr) == NULL) { goto cleanup; } @@ -380,7 +391,7 @@ control_command(isc_task_t *task, isc_event_t *event) { conn->result = named_control_docommand(conn->request, listener->readonly, &conn->text); - control_respond(conn->cmdhandle, conn->result, conn); + control_respond(conn->cmdhandle, conn); done: isc_event_free(&event); @@ -521,7 +532,7 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isc_nonce_buf(&conn->nonce, sizeof(conn->nonce)); } conn->result = ISC_R_SUCCESS; - control_respond(handle, result, conn); + control_respond(handle, conn); return; } From 58a0e95976f8299187079e196322a20c658e988a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 22 Oct 2020 13:38:09 -0700 Subject: [PATCH 11/11] CHANGES --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index e280532e3d..a0c7cce12b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5520. [bug] Fixed a number of shutdown races, reference counting + errors, and spurious log messages that could occur + in the network manager. [GL #2221] + 5519. [cleanup] Unused source code was removed: lib/dns/dbtable.c, lib/dns/portlist.c, lib/isc/bufferlist.c, and code related to those files. [GL #2060]