From 7eb4564895037d72c46150acb6a8fc04edf4f8d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Wed, 10 Jun 2020 11:32:39 +0200 Subject: [PATCH 1/8] assorted small netmgr-related changes - rename isc_nmsocket_t->tcphandle to statichandle - cancelread functions now take handles instead of sockets - add a 'client' flag in socket objects, currently unused, to indicate whether it is to be used as a client or server socket --- bin/rndc/rndc.c | 2 +- bin/tests/system/pipelined/tests.sh | 12 ++-- lib/isc/include/isc/netmgr.h | 13 ++-- lib/isc/netmgr/netmgr-int.h | 34 +++++----- lib/isc/netmgr/netmgr.c | 24 +++++--- lib/isc/netmgr/tcp.c | 96 +++++++++++++++-------------- lib/isc/netmgr/tcpdns.c | 62 ++++++++++++++----- lib/isc/netmgr/udp.c | 32 ++++++---- 8 files changed, 168 insertions(+), 107 deletions(-) diff --git a/bin/rndc/rndc.c b/bin/rndc/rndc.c index 53a1cdccd6..de6b1850fa 100644 --- a/bin/rndc/rndc.c +++ b/bin/rndc/rndc.c @@ -1009,7 +1009,7 @@ main(int argc, char **argv) { isc_mem_create(&rndc_mctx); netmgr = isc_nm_start(rndc_mctx, 1); DO("create task manager", - isc_taskmgr_create(rndc_mctx, 1, 0, NULL, &taskmgr)); + isc_taskmgr_create(rndc_mctx, 1, 0, netmgr, &taskmgr)); DO("create task", isc_task_create(taskmgr, 0, &rndc_task)); isc_log_create(rndc_mctx, &log, &logconfig); isc_log_setcontext(log); diff --git a/bin/tests/system/pipelined/tests.sh b/bin/tests/system/pipelined/tests.sh index be2b469a22..77e8b0dec6 100644 --- a/bin/tests/system/pipelined/tests.sh +++ b/bin/tests/system/pipelined/tests.sh @@ -26,12 +26,10 @@ $DIFF ref output > /dev/null && { ret=1 ; echo_i "diff out of order failed"; } if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` -# flush resolver so queries will be from others again -$RNDCCMD 10.53.0.4 flush -sleep 1 - echo_i "check pipelined TCP queries using mdig" ret=0 +$RNDCCMD 10.53.0.4 flush +sleep 1 $MDIG $MDIGOPTS +noall +answer +vc -f input -b 10.53.0.4 @10.53.0.4 > raw.mdig awk '{ print $1 " " $5 }' < raw.mdig > output.mdig sort < output.mdig > output-sorted.mdig @@ -42,6 +40,8 @@ status=`expr $status + $ret` echo_i "check keep-response-order" ret=0 +$RNDCCMD 10.53.0.4 flush +sleep 1 $PIPEQUERIES -p ${PORT} ++ < inputb > rawb || ret=1 awk '{ print $1 " " $5 }' < rawb > outputb $DIFF refb outputb || ret=1 @@ -50,6 +50,8 @@ status=`expr $status + $ret` echo_i "check keep-response-order using mdig" ret=0 +$RNDCCMD 10.53.0.4 flush +sleep 1 $MDIG $MDIGOPTS +noall +answer +vc -f inputb -b 10.53.0.7 @10.53.0.4 > rawb.mdig awk '{ print $1 " " $5 }' < rawb.mdig > outputb.mdig $DIFF refb outputb.mdig || ret=1 @@ -58,6 +60,8 @@ status=`expr $status + $ret` echo_i "check mdig -4 -6" ret=0 +$RNDCCMD 10.53.0.4 flush +sleep 1 $MDIG $MDIGOPTS -4 -6 -f input @10.53.0.4 > output46.mdig 2>&1 && ret=1 grep "only one of -4 and -6 allowed" output46.mdig > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index 3f6d8f6f05..b1c92ce348 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -176,8 +176,8 @@ isc_nm_listenudp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, * as its argument. * * When handles are allocated for the socket, 'extrasize' additional bytes - * will be allocated along with the handle for an associated object - * (typically ns_client). + * can be allocated along with the handle for an associated object, which + * can then be freed automatically when the handle is destroyed. */ void @@ -196,12 +196,17 @@ isc_nm_pause(isc_nm_t *mgr); void isc_nm_resume(isc_nm_t *mgr); /*%< - * Resume paused processing. It will return immediately - * after signalling workers to resume. + * Resume paused processing. It will return immediately after signalling + * workers to resume. */ isc_result_t 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 + * update its recv callback to 'cb', which will be called as soon as there + * is data to process. + */ isc_result_t isc_nm_pauseread(isc_nmhandle_t *handle); diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 6f4d1ff3e7..7c02d1e6da 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -117,12 +117,10 @@ struct isc_nmiface { typedef enum isc__netievent_type { netievent_udpsend, - netievent_udprecv, netievent_udpstop, netievent_tcpconnect, netievent_tcpsend, - netievent_tcprecv, netievent_tcpstartread, netievent_tcppauseread, netievent_tcpchildaccept, @@ -130,8 +128,8 @@ typedef enum isc__netievent_type { netievent_tcpstop, netievent_tcpclose, - netievent_tcpdnsclose, netievent_tcpdnssend, + netievent_tcpdnsclose, netievent_closecb, netievent_shutdown, @@ -146,20 +144,12 @@ typedef enum isc__netievent_type { netievent_tcplisten, } isc__netievent_type; -/* - * We have to split it because we can read and write on a socket - * simultaneously. - */ typedef union { isc_nm_recv_cb_t recv; + isc_nm_cb_t connect; isc_nm_accept_cb_t accept; } isc__nm_readcb_t; -typedef union { - isc_nm_cb_t send; - isc_nm_cb_t connect; -} isc__nm_writecb_t; - typedef union { isc_nm_recv_cb_t recv; isc_nm_accept_cb_t accept; @@ -209,10 +199,10 @@ typedef isc__netievent__socket_t isc__netievent_udplisten_t; typedef isc__netievent__socket_t isc__netievent_udpstop_t; typedef isc__netievent__socket_t isc__netievent_tcpstop_t; typedef isc__netievent__socket_t isc__netievent_tcpclose_t; -typedef isc__netievent__socket_t isc__netievent_tcpdnsclose_t; typedef isc__netievent__socket_t isc__netievent_startread_t; typedef isc__netievent__socket_t isc__netievent_pauseread_t; typedef isc__netievent__socket_t isc__netievent_closecb_t; +typedef isc__netievent__socket_t isc__netievent_tcpdnsclose_t; typedef struct isc__netievent__socket_req { isc__netievent_type type; @@ -333,7 +323,7 @@ typedef enum isc_nmsocket_type { isc_nm_tcpsocket, isc_nm_tcplistener, isc_nm_tcpdnslistener, - isc_nm_tcpdnssocket + isc_nm_tcpdnssocket, } isc_nmsocket_type; /*% @@ -403,7 +393,7 @@ struct isc_nmsocket { isc_nmsocket_t *children; int nchildren; isc_nmiface_t *iface; - isc_nmhandle_t *tcphandle; + isc_nmhandle_t *statichandle; isc_nmhandle_t *outerhandle; /*% Extra data allocated at the end of each isc_nmhandle_t */ @@ -445,7 +435,12 @@ struct isc_nmsocket { isc_refcount_t references; /*% - * TCPDNS socket has been set not to pipeliine. + * Established an outgoing connection, as client not server. + */ + atomic_bool client; + + /*% + * TCPDNS socket has been set not to pipeline. */ atomic_bool sequential; @@ -686,6 +681,9 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, isc_result_t 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. + */ void isc__nm_tcp_close(isc_nmsocket_t *sock); @@ -713,9 +711,9 @@ isc__nm_tcp_shutdown(isc_nmsocket_t *sock); */ void -isc__nm_tcp_cancelread(isc_nmsocket_t *sock); +isc__nm_tcp_cancelread(isc_nmhandle_t *handle); /*%< - * Stop reading on a connected socket. + * Stop reading on a connected TCP handle. */ void diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index bb10557e9c..abd4566669 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -591,6 +591,7 @@ process_queue(isc__networker_t *worker, isc_queue_t *queue) { uv_stop(&worker->loop); isc_mempool_put(worker->mgr->evpool, ievent); return; + case netievent_udplisten: isc__nm_async_udplisten(worker, ievent); break; @@ -600,6 +601,7 @@ process_queue(isc__networker_t *worker, isc_queue_t *queue) { case netievent_udpsend: isc__nm_async_udpsend(worker, ievent); break; + case netievent_tcpconnect: isc__nm_async_tcpconnect(worker, ievent); break; @@ -630,9 +632,11 @@ process_queue(isc__networker_t *worker, isc_queue_t *queue) { case netievent_tcpclose: isc__nm_async_tcpclose(worker, ievent); break; + case netievent_tcpdnsclose: isc__nm_async_tcpdnsclose(worker, ievent); break; + case netievent_closecb: isc__nm_async_closecb(worker, ievent); break; @@ -739,7 +743,7 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree) { isc__nm_decstats(sock->mgr, sock->statsindex[STATID_ACTIVE]); } - sock->tcphandle = NULL; + sock->statichandle = NULL; if (sock->outerhandle != NULL) { isc_nmhandle_unref(sock->outerhandle); @@ -833,7 +837,7 @@ nmsocket_maybe_destroy(isc_nmsocket_t *sock) { } } - if (active_handles == 0 || sock->tcphandle != NULL) { + if (active_handles == 0 || sock->statichandle != NULL) { destroy = true; } @@ -1051,7 +1055,7 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, if (handle == NULL) { handle = alloc_handle(sock); } else { - isc_refcount_increment0(&handle->references); + isc_refcount_init(&handle->references, 1); INSIST(VALID_NMHANDLE(handle)); } @@ -1099,9 +1103,11 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, handle->ah_pos = pos; UNLOCK(&sock->lock); - if (sock->type == isc_nm_tcpsocket) { - INSIST(sock->tcphandle == NULL); - sock->tcphandle = handle; + if (sock->type == isc_nm_tcpsocket || + (sock->type == isc_nm_udpsocket && atomic_load(&sock->client))) + { + INSIST(sock->statichandle == NULL); + sock->statichandle = handle; } return (handle); @@ -1208,6 +1214,10 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) { } } + if (handle == sock->statichandle) { + sock->statichandle = NULL; + } + isc__nmsocket_detach(&sock); } @@ -1353,7 +1363,7 @@ isc_nm_cancelread(isc_nmhandle_t *handle) { switch (handle->sock->type) { case isc_nm_tcpsocket: - isc__nm_tcp_cancelread(handle->sock); + isc__nm_tcp_cancelread(handle); break; default: INSIST(0); diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 73b8b1fa0b..114279710c 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -147,49 +147,46 @@ done: static void tcp_connect_cb(uv_connect_t *uvreq, int status) { + isc_result_t result; isc__nm_uvreq_t *req = (isc__nm_uvreq_t *)uvreq->data; isc_nmsocket_t *sock = NULL; + struct sockaddr_storage ss; + isc_nmhandle_t *handle = NULL; sock = uv_handle_get_data((uv_handle_t *)uvreq->handle); REQUIRE(VALID_UVREQ(req)); - if (status == 0) { - isc_result_t result; - struct sockaddr_storage ss; - isc_nmhandle_t *handle = NULL; - - sock = uv_handle_get_data((uv_handle_t *)uvreq->handle); - isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]); - uv_tcp_getpeername(&sock->uv_handle.tcp, (struct sockaddr *)&ss, - &(int){ sizeof(ss) }); - result = isc_sockaddr_fromsockaddr(&sock->peer, - (struct sockaddr *)&ss); - RUNTIME_CHECK(result == ISC_R_SUCCESS); - - handle = isc__nmhandle_get(sock, NULL, NULL); - req->cb.connect(handle, ISC_R_SUCCESS, req->cbarg); - - isc__nm_uvreq_put(&req, sock); - - /* - * The sock is now attached to the handle. - */ - isc__nmsocket_detach(&sock); - - /* - * If the connect callback wants to hold on to the handle, - * it needs to attach to it. - */ - isc_nmhandle_unref(handle); - } else { - /* - * TODO: - * Handle the connect error properly and free the socket. - */ + if (status != 0) { req->cb.connect(NULL, isc__nm_uverr2result(status), req->cbarg); isc__nm_uvreq_put(&req, sock); + return; } + + sock = uv_handle_get_data((uv_handle_t *)uvreq->handle); + isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]); + uv_tcp_getpeername(&sock->uv_handle.tcp, (struct sockaddr *)&ss, + &(int){ sizeof(ss) }); + result = isc_sockaddr_fromsockaddr(&sock->peer, (struct sockaddr *)&ss); + RUNTIME_CHECK(result == ISC_R_SUCCESS); + + handle = isc__nmhandle_get(sock, NULL, NULL); + req->cb.connect(handle, ISC_R_SUCCESS, req->cbarg); + + isc__nm_uvreq_put(&req, sock); + + atomic_init(&sock->client, true); + + /* + * The sock is now attached to the handle. + */ + isc__nmsocket_detach(&sock); + + /* + * If the connect callback wants to hold on to the handle, + * it needs to attach to it. + */ + isc_nmhandle_unref(handle); } isc_result_t @@ -201,6 +198,8 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, isc_result_t result = ISC_R_SUCCESS; REQUIRE(VALID_NM(mgr)); + REQUIRE(local != NULL); + REQUIRE(peer != NULL); nsock = isc_mem_get(mgr->mctx, sizeof(*nsock)); isc__nmsocket_init(nsock, mgr, isc_nm_tcpsocket, local); @@ -211,6 +210,7 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, req->cb.connect = cb; req->cbarg = cbarg; req->peer = peer->addr; + req->local = local->addr; ievent = isc__nm_get_ievent(mgr, netievent_tcpconnect); ievent->sock = nsock; @@ -604,7 +604,7 @@ readtimeout_cb(uv_timer_t *handle) { isc_quota_detach(&sock->quota); } if (sock->rcb.recv != NULL) { - sock->rcb.recv(sock->tcphandle, ISC_R_TIMEDOUT, NULL, + sock->rcb.recv(sock->statichandle, ISC_R_TIMEDOUT, NULL, sock->rcbarg); isc__nmsocket_clearcb(sock); } @@ -766,8 +766,8 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) { .length = nread }; if (sock->rcb.recv != NULL) { - sock->rcb.recv(sock->tcphandle, ISC_R_SUCCESS, ®ion, - sock->rcbarg); + sock->rcb.recv(sock->statichandle, ISC_R_SUCCESS, + ®ion, sock->rcbarg); } sock->read_timeout = (atomic_load(&sock->keepalive) @@ -792,7 +792,8 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) { */ if (sock->rcb.recv != NULL) { isc__nm_incstats(sock->mgr, sock->statsindex[STATID_RECVFAIL]); - sock->rcb.recv(sock->tcphandle, ISC_R_EOF, NULL, sock->rcbarg); + sock->rcb.recv(sock->statichandle, ISC_R_EOF, NULL, + sock->rcbarg); isc__nmsocket_clearcb(sock); } @@ -1125,24 +1126,27 @@ void isc__nm_tcp_shutdown(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); - if (sock->type == isc_nm_tcpsocket && sock->tcphandle != NULL && + if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL && sock->rcb.recv != NULL) { - sock->rcb.recv(sock->tcphandle, ISC_R_CANCELED, NULL, + sock->rcb.recv(sock->statichandle, ISC_R_CANCELED, NULL, sock->rcbarg); isc__nmsocket_clearcb(sock); } } void -isc__nm_tcp_cancelread(isc_nmsocket_t *sock) { - REQUIRE(VALID_NMSOCK(sock)); +isc__nm_tcp_cancelread(isc_nmhandle_t *handle) { + isc_nmsocket_t *sock = NULL; - if (sock->type == isc_nm_tcpsocket && sock->tcphandle != NULL && - sock->rcb.recv != NULL) - { - sock->rcb.recv(sock->tcphandle, ISC_R_CANCELED, NULL, - sock->rcbarg); + REQUIRE(VALID_NMHANDLE(handle)); + + sock = handle->sock; + + REQUIRE(sock->type == isc_nm_tcpsocket); + + if (atomic_load(&sock->client) && sock->rcb.recv != NULL) { + sock->rcb.recv(handle, ISC_R_EOF, NULL, sock->rcbarg); isc__nmsocket_clearcb(sock); } } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 512f88be03..107cde97b6 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -82,7 +82,9 @@ alloc_dnsbuf(isc_nmsocket_t *sock, size_t len) { static void timer_close_cb(uv_handle_t *handle) { isc_nmsocket_t *sock = (isc_nmsocket_t *)uv_handle_get_data(handle); - INSIST(VALID_NMSOCK(sock)); + + REQUIRE(VALID_NMSOCK(sock)); + atomic_store(&sock->closed, true); tcpdns_close_direct(sock); } @@ -94,7 +96,8 @@ dnstcp_readtimeout(uv_timer_t *timer) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - /* Close the TCP connection, it's closing should fire 'our' closing */ + + /* Close the TCP connection; its closure should fire ours. */ isc_nmhandle_unref(sock->outerhandle); sock->outerhandle = NULL; } @@ -187,8 +190,15 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) { */ len = dnslen(dnssock->buf); if (len <= dnssock->buf_len - 2) { - isc_nmhandle_t *dnshandle = isc__nmhandle_get(dnssock, NULL, - NULL); + isc_nmhandle_t *dnshandle; + if (atomic_load(&dnssock->client) && + dnssock->statichandle != NULL) { + dnshandle = dnssock->statichandle; + isc_nmhandle_ref(dnshandle); + } else { + dnshandle = isc__nmhandle_get(dnssock, NULL, NULL); + } + isc_nmsocket_t *listener = dnssock->listener; if (listener != NULL && listener->rcb.recv != NULL) { @@ -197,6 +207,20 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) { &(isc_region_t){ .base = dnssock->buf + 2, .length = len }, listener->rcbarg); + } else if (dnssock->rcb.recv != NULL) { + isc_nm_recv_cb_t cb = dnssock->rcb.recv; + void *cbarg = dnssock->rcbarg; + + /* + * We need to clear the read callback *before* + * calling it, because it might make another + * call to isc_nm_read() and set up a new callback. + */ + isc__nmsocket_clearcb(dnssock); + cb(dnshandle, ISC_R_SUCCESS, + &(isc_region_t){ .base = dnssock->buf + 2, + .length = len }, + cbarg); } len += 2; @@ -227,11 +251,12 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, REQUIRE(VALID_NMSOCK(dnssock)); REQUIRE(VALID_NMHANDLE(handle)); - REQUIRE(dnssock->tid == isc_nm_tid()); if (region == NULL || eresult != ISC_R_SUCCESS) { /* Connection closed */ - isc_nmhandle_unref(handle); + if (eresult != ISC_R_CANCELED) { + isc_nmhandle_unref(handle); + } dnssock->result = eresult; if (dnssock->self != NULL) { isc__nmsocket_detach(&dnssock->self); @@ -277,11 +302,14 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, uv_timer_stop(&dnssock->timer); } - if (atomic_load(&dnssock->sequential)) { + if (atomic_load(&dnssock->sequential) || + dnssock->rcb.recv == NULL) { /* - * We're in sequential mode and we processed - * one packet, so we're done until the next read - * completes. + * Two reasons we might want to pause here: + * - If we're in sequential mode and we've received + * a whole packet, so we're done until it's been + * processed; + * - If we no longer have a read callback. */ isc_nm_pauseread(dnssock->outerhandle); done = true; @@ -314,7 +342,6 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, void *cbarg, isc_nm_accept_cb_t accept_cb, void *accept_cbarg, size_t extrahandlesize, int backlog, isc_quota_t *quota, isc_nmsocket_t **sockp) { - /* A 'wrapper' socket object with outer set to true TCP socket */ isc_nmsocket_t *dnslistensock = isc_mem_get(mgr->mctx, sizeof(*dnslistensock)); isc_result_t result; @@ -328,7 +355,11 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, dnslistensock->accept_cbarg = accept_cbarg; dnslistensock->extrahandlesize = extrahandlesize; - /* We set dnslistensock->outer to a true listening socket */ + /* + * dnslistensock will be a DNS 'wrapper' around a connected + * stream. We set dnslistensock->outer to a socket listening + * for a TCP connection. + */ result = isc_nm_listentcp(mgr, iface, dnslisten_acceptcb, dnslistensock, extrahandlesize, backlog, quota, &dnslistensock->outer); @@ -495,8 +526,7 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) { r.base = (unsigned char *)req->uvbuf.base; r.length = req->uvbuf.len; - result = isc__nm_tcp_send(sock->outerhandle, &r, tcpdnssend_cb, - req); + result = isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb, req); } if (result != ISC_R_SUCCESS) { @@ -538,8 +568,8 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region, r.base = (unsigned char *)uvreq->uvbuf.base; r.length = uvreq->uvbuf.len; - return (isc__nm_tcp_send(sock->outerhandle, &r, tcpdnssend_cb, - uvreq)); + return (isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb, + uvreq)); } else { isc__netievent_tcpdnssend_t *ievent = NULL; diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index ff981f920f..9a4013f666 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -80,7 +80,7 @@ isc_nm_listenudp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, csock->rcb.recv = cb; csock->rcbarg = cbarg; csock->fd = socket(family, SOCK_DGRAM, 0); - INSIST(csock->fd >= 0); + RUNTIME_CHECK(csock->fd >= 0); /* * This is SO_REUSE**** hell: @@ -223,7 +223,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) { } static void -udp_close_cb(uv_handle_t *handle) { +udp_stop_cb(uv_handle_t *handle) { isc_nmsocket_t *sock = uv_handle_get_data(handle); atomic_store(&sock->closed, true); @@ -236,7 +236,7 @@ stop_udp_child(isc_nmsocket_t *sock) { REQUIRE(sock->tid == isc_nm_tid()); uv_udp_recv_stop(&sock->uv_handle.udp); - uv_close((uv_handle_t *)&sock->uv_handle.udp, udp_close_cb); + uv_close((uv_handle_t *)&sock->uv_handle.udp, udp_stop_cb); isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CLOSE]); @@ -395,7 +395,11 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, result = isc_sockaddr_fromsockaddr(&sockaddr, addr); RUNTIME_CHECK(result == ISC_R_SUCCESS); - nmhandle = isc__nmhandle_get(sock, &sockaddr, NULL); + if (!atomic_load(&sock->connected)) { + nmhandle = isc__nmhandle_get(sock, &sockaddr, NULL); + } else { + nmhandle = sock->statichandle; + } region.base = (unsigned char *)buf->base; region.length = nrecv; @@ -425,13 +429,13 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, isc_result_t isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, void *cbarg) { - isc_nmsocket_t *psock = NULL, *rsock = NULL; isc_nmsocket_t *sock = handle->sock; + isc_nmsocket_t *psock = NULL, *rsock = sock; isc_sockaddr_t *peer = &handle->peer; isc__netievent_udpsend_t *ievent = NULL; isc__nm_uvreq_t *uvreq = NULL; - int ntid; uint32_t maxudp = atomic_load(&sock->mgr->maxudp); + int ntid; /* * We're simulating a firewall blocking UDP packets bigger than @@ -446,12 +450,12 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, return (ISC_R_SUCCESS); } - if (sock->type == isc_nm_udpsocket) { + if (sock->type == isc_nm_udpsocket && !atomic_load(&sock->client)) { INSIST(sock->parent != NULL); psock = sock->parent; } else if (sock->type == isc_nm_udplistener) { psock = sock; - } else { + } else if (!atomic_load(&sock->client)) { INSIST(0); ISC_UNREACHABLE(); } @@ -467,13 +471,16 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, */ if (isc__nm_in_netthread()) { ntid = isc_nm_tid(); - } else if (sock->type == isc_nm_udpsocket) { + } else if (sock->type == isc_nm_udpsocket && + !atomic_load(&sock->client)) { ntid = sock->tid; } else { ntid = (int)isc_random_uniform(sock->nchildren); } - rsock = &psock->children[ntid]; + if (psock != NULL) { + rsock = &psock->children[ntid]; + } uvreq = isc__nm_uvreq_get(sock->mgr, sock); uvreq->uvbuf.base = (char *)region->base; @@ -553,6 +560,7 @@ udp_send_cb(uv_udp_send_t *req, int status) { 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; REQUIRE(sock->tid == isc_nm_tid()); @@ -561,9 +569,11 @@ udp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, if (!isc__nmsocket_active(sock)) { return (ISC_R_CANCELED); } + isc_nmhandle_ref(req->handle); + 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, &peer->type.sa, udp_send_cb); + &req->uvbuf, 1, sa, udp_send_cb); if (rv < 0) { isc__nm_incstats(req->sock->mgr, req->sock->statsindex[STATID_SENDFAIL]); From cfa4ea64bc06685f210a4187dcc05cc0aac84851 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sat, 5 Sep 2020 12:10:35 -0700 Subject: [PATCH 2/8] fix LD_WRAP test in configure The LD_WRAP test in configure was broken, and failed to indicate LD_WRAP support correctly, resulting in some unit tests failing to run. --- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 12170df1bf..9ddad8edf4 100644 --- a/configure.ac +++ b/configure.ac @@ -1320,11 +1320,11 @@ AM_CONDITIONAL([HAVE_CMOCKA], [test "$with_cmocka" = "yes"]) LD_WRAP_TESTS=false AC_MSG_CHECKING([for linker support for --wrap option]) AX_SAVE_FLAGS([wrap]) -LDFLAGS="-Wl,-wrap,exit" +LDFLAGS="-Wl,--wrap,exit" AC_RUN_IFELSE( [AC_LANG_PROGRAM([[#include void __real_exit (int status); - void __wrap_exit (int status) { __real_exit (status); } + void __wrap_exit (int status) { __real_exit (0); } ]], [[exit (1);]])], [LD_WRAP_TESTS=true From 57b4dde9749c88d21d1dc8afd22201224cf83cab Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 3 Sep 2020 13:31:27 -0700 Subject: [PATCH 3/8] change from isc_nmhandle_ref/unref to isc_nmhandle attach/detach Attaching and detaching handle pointers will make it easier to determine where and why reference counting errors have occurred. A handle needs to be referenced more than once when multiple asynchronous operations are in flight, so callers must now maintain multiple handle pointers for each pending operation. For example, ns_client objects now contain: - reqhandle: held while waiting for a request callback (query, notify, update) - sendhandle: held while waiting for a send callback - fetchhandle: held while waiting for a recursive fetch to complete - updatehandle: held while waiting for an update-forwarding task to complete control channel connection objects now contain: - readhandle: held while waiting for a read callback - sendhandle: held while waiting for a send callback - cmdhandle: held while an rndc command is running httpd connections contain: - readhandle: held while waiting for a read callback - sendhandle: held while waiting for a send callback --- bin/named/controlconf.c | 149 +++++++++++++----------- bin/named/server.c | 5 +- bin/rndc/rndc.c | 69 +++++++---- bin/tests/system/rndc/ns4/named.conf.in | 1 + lib/isc/httpd.c | 31 ++--- lib/isc/include/isc/netmgr.h | 4 +- lib/isc/netmgr/netmgr.c | 27 ++++- lib/isc/netmgr/tcp.c | 19 ++- lib/isc/netmgr/tcpdns.c | 81 +++++++------ lib/isc/netmgr/udp.c | 36 +++--- lib/isc/win32/libisc.def.in | 4 +- lib/isccc/ccmsg.c | 8 +- lib/isccc/include/isccc/ccmsg.h | 4 +- lib/ns/client.c | 52 +++------ lib/ns/include/ns/client.h | 21 ++-- lib/ns/include/ns/notify.h | 2 +- lib/ns/include/ns/query.h | 2 +- lib/ns/include/ns/update.h | 3 +- lib/ns/notify.c | 11 +- lib/ns/query.c | 30 +++-- lib/ns/tests/Makefile.am | 5 +- lib/ns/tests/notify_test.c | 8 +- lib/ns/tests/nstest.c | 47 ++++++-- lib/ns/tests/wrap.c | 19 ++- lib/ns/update.c | 35 ++++-- lib/ns/xfrout.c | 52 ++++----- 26 files changed, 416 insertions(+), 309 deletions(-) diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index c69650ace5..6e91d4f782 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -67,9 +67,11 @@ struct controlkey { }; struct controlconnection { - isc_nmhandle_t *handle; + isc_nmhandle_t *readhandle; + isc_nmhandle_t *sendhandle; + isc_nmhandle_t *cmdhandle; isccc_ccmsg_t ccmsg; - bool ccmsg_valid; + bool reading; bool sending; controllistener_t *listener; isccc_sexpr_t *ctrl; @@ -221,10 +223,8 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { conn->sending = false; - isc_nmhandle_unref(handle); - - if (result == ISC_R_CANCELED) { - return; + if (listener->controls->shuttingdown || result == ISC_R_CANCELED) { + goto cleanup_sendhandle; } else if (result != ISC_R_SUCCESS) { char socktext[ISC_SOCKADDR_FORMATSIZE]; @@ -233,16 +233,31 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { NAMED_LOGMODULE_CONTROL, ISC_LOG_WARNING, "error sending command response to %s: %s", socktext, isc_result_totext(result)); - return; + goto cleanup_sendhandle; } + isc_nmhandle_attach(handle, &conn->readhandle); + conn->reading = true; + + isc_nmhandle_detach(&conn->sendhandle); + result = isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn); if (result != ISC_R_SUCCESS) { + isc_nmhandle_detach(&conn->readhandle); + conn->reading = false; + maybe_free_listener(listener); + + return; } listener->listening = true; + + return; + +cleanup_sendhandle: + isc_nmhandle_detach(&conn->sendhandle); } static inline void @@ -257,6 +272,25 @@ log_invalid(isccc_ccmsg_t *ccmsg, isc_result_t result) { isc_result_totext(result)); } +static void +conn_cleanup(controlconnection_t *conn) { + controllistener_t *listener = conn->listener; + + if (conn->response != NULL) { + isccc_sexpr_free(&conn->response); + } + if (conn->request != NULL) { + isccc_sexpr_free(&conn->request); + } + if (conn->secret.rstart != NULL) { + isc_mem_put(listener->mctx, conn->secret.rstart, + REGION_SIZE(conn->secret)); + } + if (conn->text != NULL) { + isc_buffer_free(&conn->text); + } +} + static void control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) { controlconnection_t *conn = (controlconnection_t *)arg; @@ -324,29 +358,21 @@ control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) { r.base = conn->buffer->base; r.length = conn->buffer->used; - isc_nmhandle_ref(handle); + isc_nmhandle_attach(handle, &conn->sendhandle); conn->sending = true; - result = isc_nm_send(handle, &r, control_senddone, conn); + conn_cleanup(conn); + + isc_nmhandle_detach(&conn->cmdhandle); + + result = isc_nm_send(conn->sendhandle, &r, control_senddone, conn); if (result != ISC_R_SUCCESS) { - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&conn->sendhandle); conn->sending = false; } + return; cleanup: - if (conn->response != NULL) { - isccc_sexpr_free(&conn->response); - } - if (conn->request != NULL) { - isccc_sexpr_free(&conn->request); - } - - if (conn->secret.rstart != NULL) { - isc_mem_put(listener->mctx, conn->secret.rstart, - REGION_SIZE(conn->secret)); - } - if (conn->text != NULL) { - isc_buffer_free(&conn->text); - } + conn_cleanup(conn); } static void @@ -356,17 +382,17 @@ control_command(isc_task_t *task, isc_event_t *event) { UNUSED(task); - /* - * An extra ref and two unrefs are needed here to - * ensure the handle isn't cleaned up if we're running - * an "rndc stop" command. - */ - isc_nmhandle_ref(conn->handle); + if (listener->controls->shuttingdown) { + isc_nmhandle_detach(&conn->cmdhandle); + conn_cleanup(conn); + goto done; + } + conn->result = named_control_docommand(conn->request, listener->readonly, &conn->text); - control_respond(conn->handle, conn->result, conn); - isc_nmhandle_unref(conn->handle); - isc_nmhandle_unref(conn->handle); + control_respond(conn->cmdhandle, conn->result, conn); + +done: isc_event_free(&event); } @@ -380,26 +406,21 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isccc_time_t exp; uint32_t nonce; - conn->ccmsg_valid = false; + conn->reading = false; /* Is the server shutting down? */ if (listener->controls->shuttingdown) { - return; + goto cleanup_readhandle; } if (result != ISC_R_SUCCESS) { if (result == ISC_R_CANCELED) { - /* - * Don't bother with any more scheduled command events. - */ listener->controls->shuttingdown = true; - isc_task_purge(named_g_server->task, NULL, - NAMED_EVENT_COMMAND, NULL); } else if (result != ISC_R_EOF) { log_invalid(&conn->ccmsg, result); } - return; + goto cleanup_readhandle; } for (key = ISC_LIST_HEAD(listener->keys); key != NULL; @@ -424,7 +445,7 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) { REGION_SIZE(conn->secret)); if (result != ISCCC_R_BADAUTH) { log_invalid(&conn->ccmsg, result); - return; + goto cleanup; } } @@ -496,7 +517,8 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isc_buffer_allocate(listener->mctx, &conn->text, 2 * 2048); - conn->ccmsg_valid = true; + isc_nmhandle_attach(handle, &conn->cmdhandle); + isc_nmhandle_detach(&conn->readhandle); if (conn->nonce == 0) { /* @@ -513,26 +535,23 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) { /* * Trigger the command. */ - isc_nmhandle_ref(handle); + event = isc_event_allocate(listener->mctx, conn, NAMED_EVENT_COMMAND, control_command, conn, sizeof(isc_event_t)); isc_task_send(named_g_server->task, &event); + return; cleanup: - if (conn->response != NULL) { - isccc_sexpr_free(&conn->response); - } - if (conn->request != NULL) { - isccc_sexpr_free(&conn->request); - } + conn_cleanup(conn); - if (conn->secret.rstart != NULL) { - isc_mem_put(listener->mctx, conn->secret.rstart, - REGION_SIZE(conn->secret)); - } - if (conn->text != NULL) { - isc_buffer_free(&conn->text); +cleanup_readhandle: + /* + * readhandle could be NULL if we're shutting down, + * but if not we need to detach it. + */ + if (conn->readhandle != NULL) { + isc_nmhandle_detach(&conn->readhandle); } } @@ -545,7 +564,7 @@ conn_reset(void *arg) { isc_buffer_free(&conn->buffer); } - if (conn->ccmsg_valid) { + if (conn->reading) { isccc_ccmsg_cancelread(&conn->ccmsg); return; } @@ -583,18 +602,11 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) { isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_CONTROL, ISC_LOG_DEBUG(3), "allocate new control connection"); - *conn = (controlconnection_t){ .handle = NULL }; - } - - if (conn->handle == NULL) { isc_nmhandle_setdata(handle, conn, conn_reset, conn_put); - } else { - INSIST(conn->handle == handle); } - *conn = (controlconnection_t){ .handle = handle, - .listener = listener, - .ccmsg_valid = true, + *conn = (controlconnection_t){ .listener = listener, + .reading = false, .alg = DST_ALG_UNKNOWN }; isccc_ccmsg_init(listener->mctx, handle, &conn->ccmsg); @@ -604,9 +616,14 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) { ISC_LINK_INIT(conn, link); + 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; } diff --git a/bin/named/server.c b/bin/named/server.c index bc6c1e7b3a..af819f3d92 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -14866,14 +14866,15 @@ named_server_zonestatus(named_server_t *server, isc_lex_t *lex, /* Serial number */ result = dns_zone_getserial(mayberaw, &serial); - /* XXXWPK TODO this is to mirror old behavior with dns_zone_getserial */ + + /* This is to mirror old behavior with dns_zone_getserial */ if (result != ISC_R_SUCCESS) { serial = 0; } + snprintf(serbuf, sizeof(serbuf), "%u", serial); if (hasraw) { result = dns_zone_getserial(zone, &signed_serial); - /* XXXWPK TODO ut supra */ if (result != ISC_R_SUCCESS) { serial = 0; } diff --git a/bin/rndc/rndc.c b/bin/rndc/rndc.c index de6b1850fa..a05b1ee96b 100644 --- a/bin/rndc/rndc.c +++ b/bin/rndc/rndc.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -89,6 +90,8 @@ static uint32_t serial; static bool quiet = false; static bool showresult = false; static bool shuttingdown = false; +static isc_nmhandle_t *recvdone_handle = NULL; +static isc_nmhandle_t *recvnonce_handle = NULL; static void rndc_startconnect(isc_sockaddr_t *addr); @@ -288,19 +291,21 @@ get_addresses(const char *host, in_port_t port) { static void rndc_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { - UNUSED(arg); + isc_nmhandle_t *sendhandle = (isc_nmhandle_t *)arg; if (result != ISC_R_SUCCESS) { fatal("send failed: %s", isc_result_totext(result)); } + REQUIRE(sendhandle == handle); + isc_nmhandle_detach(&sendhandle); + if (atomic_fetch_sub_release(&sends, 1) == 1 && atomic_load_acquire(&recvs) == 0) { shuttingdown = true; isc_task_shutdown(rndc_task); isc_app_shutdown(); - isc_nmhandle_unref(handle); } } @@ -315,10 +320,12 @@ rndc_recvdone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { REQUIRE(ccmsg != NULL); - atomic_fetch_sub_release(&recvs, 1); - if (shuttingdown && (result == ISC_R_EOF || result == ISC_R_CANCELED)) { - isc_nmhandle_unref(handle); + atomic_fetch_sub_release(&recvs, 1); + if (handle != NULL) { + REQUIRE(recvdone_handle == handle); + isc_nmhandle_detach(&recvdone_handle); + } return; } else if (result == ISC_R_EOF) { fatal("connection to remote host closed.\n" @@ -376,10 +383,13 @@ rndc_recvdone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isccc_sexpr_free(&response); + REQUIRE(recvdone_handle == handle); + isc_nmhandle_detach(&recvdone_handle); + if (atomic_load_acquire(&sends) == 0 && - atomic_load_acquire(&recvs) == 0) { + atomic_fetch_sub_release(&recvs, 1) == 1) + { shuttingdown = true; - isc_nmhandle_unref(handle); isc_task_shutdown(rndc_task); isc_app_shutdown(); } @@ -400,10 +410,12 @@ rndc_recvnonce(isc_nmhandle_t *handle, isc_result_t result, void *arg) { REQUIRE(ccmsg != NULL); - atomic_fetch_sub_release(&recvs, 1); - if (shuttingdown && result == ISC_R_EOF) { - isc_nmhandle_unref(handle); + atomic_fetch_sub_release(&recvs, 1); + if (handle != NULL) { + REQUIRE(recvnonce_handle == handle); + isc_nmhandle_detach(&recvnonce_handle); + } return; } else if (result == ISC_R_EOF) { fatal("connection to remote host closed.\n" @@ -467,12 +479,19 @@ rndc_recvnonce(isc_nmhandle_t *handle, isc_result_t result, void *arg) { r.base = databuf->base; r.length = databuf->used; + isc_nmhandle_attach(handle, &recvdone_handle); + atomic_fetch_add_relaxed(&recvs, 1); DO("schedule recv", isccc_ccmsg_readmessage(ccmsg, rndc_recvdone, ccmsg)); - atomic_fetch_add_relaxed(&recvs, 1); - DO("send message", isc_nm_send(handle, &r, rndc_senddone, NULL)); + 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)); + + REQUIRE(recvnonce_handle == handle); + isc_nmhandle_detach(&recvnonce_handle); + atomic_fetch_sub_release(&recvs, 1); isccc_sexpr_free(&response); isccc_sexpr_free(&request); @@ -488,12 +507,12 @@ rndc_connected(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isccc_time_t now; isc_region_t r; isc_buffer_t b; + isc_nmhandle_t *connhandle = NULL; REQUIRE(ccmsg != NULL); - atomic_fetch_sub_release(&connects, 1); - if (result != ISC_R_SUCCESS) { + atomic_fetch_sub_release(&connects, 1); isc_sockaddr_format(&serveraddrs[currentaddr], socktext, sizeof(socktext)); if (++currentaddr < nserveraddrs) { @@ -507,6 +526,8 @@ rndc_connected(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isc_result_totext(result)); } + isc_nmhandle_attach(handle, &connhandle); + isc_stdtime_get(&now); DO("create message", isccc_cc_createmessage(1, NULL, NULL, ++serial, now, now + 60, &request)); @@ -534,14 +555,18 @@ rndc_connected(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isccc_ccmsg_init(rndc_mctx, handle, ccmsg); isccc_ccmsg_setmaxsize(ccmsg, 1024 * 1024); - isc_nmhandle_ref(handle); - + isc_nmhandle_attach(handle, &recvnonce_handle); + atomic_fetch_add_relaxed(&recvs, 1); DO("schedule recv", isccc_ccmsg_readmessage(ccmsg, rndc_recvnonce, ccmsg)); - atomic_fetch_add_relaxed(&recvs, 1); - DO("send message", isc_nm_send(handle, &r, rndc_senddone, NULL)); + 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_nmhandle_detach(&connhandle); + atomic_fetch_sub_release(&connects, 1); isccc_sexpr_free(&request); } @@ -573,11 +598,11 @@ rndc_startconnect(isc_sockaddr_t *addr) { ISC_UNREACHABLE(); } + atomic_fetch_add_relaxed(&connects, 1); DO("create connection", isc_nm_tcpconnect(netmgr, (isc_nmiface_t *)local, (isc_nmiface_t *)addr, rndc_connected, &rndc_ccmsg, 0)); - atomic_fetch_add_relaxed(&connects, 1); } static void @@ -1068,16 +1093,18 @@ main(int argc, char **argv) { isc_task_detach(&rndc_task); isc_taskmgr_destroy(&taskmgr); - isc_nm_destroy(&netmgr); + isc_nm_closedown(netmgr); /* * Note: when TCP connections are shut down, there will be a final * call to the isccc callback routine with &rndc_ccmsg as its * argument. We therefore need to delay invalidating it until - * after the netmgr is destroyed. + * after the netmgr is closed down. */ isccc_ccmsg_invalidate(&rndc_ccmsg); + isc_nm_destroy(&netmgr); + isc_log_destroy(&log); isc_log_setcontext(NULL); diff --git a/bin/tests/system/rndc/ns4/named.conf.in b/bin/tests/system/rndc/ns4/named.conf.in index 2466e69559..4f009a7c2d 100644 --- a/bin/tests/system/rndc/ns4/named.conf.in +++ b/bin/tests/system/rndc/ns4/named.conf.in @@ -15,6 +15,7 @@ options { listen-on { 10.53.0.4; }; listen-on-v6 { none; }; recursion yes; + dnssec-validation yes; }; view normal { diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index 688c873daa..1b893fce65 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -75,7 +75,10 @@ struct isc_httpd { isc_httpdmgr_t *mgr; /*%< our parent */ ISC_LINK(isc_httpd_t) link; - isc_nmhandle_t *handle; + isc_nmhandle_t *handle; /* Permanent pointer to handle */ + isc_nmhandle_t *readhandle; /* Waiting for a read callback */ + isc_nmhandle_t *sendhandle; /* Waiting for a send callback */ + state_t state; int flags; @@ -636,6 +639,7 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) { if (httpd->handle == NULL) { isc_nmhandle_setdata(handle, httpd, httpd_reset, httpd_put); + httpd->handle = handle; } else { INSIST(httpd->handle == handle); } @@ -663,10 +667,10 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) { ISC_LIST_APPEND(httpdmgr->running, httpd, link); UNLOCK(&httpdmgr->lock); - isc_nmhandle_ref(handle); + isc_nmhandle_attach(handle, &httpd->readhandle); result = isc_nm_read(handle, httpd_request, httpdmgr); if (result != ISC_R_SUCCESS) { - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&httpd->readhandle); } return (result); @@ -849,7 +853,7 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult, REQUIRE(httpd->state == RECV); if (eresult != ISC_R_SUCCESS) { - goto done; + goto cleanup_readhandle; } result = process_request(httpd, region); @@ -858,9 +862,9 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult, /* don't unref, continue reading */ return; } - goto done; + goto cleanup_readhandle; } else if (result != ISC_R_SUCCESS) { - goto done; + goto cleanup_readhandle; } isc_buffer_initnull(&httpd->bodybuffer); @@ -958,18 +962,18 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_nm_pauseread(handle); httpd->state = SEND; - isc_nmhandle_ref(handle); + isc_nmhandle_attach(handle, &httpd->sendhandle); result = isc_nm_send(handle, &r, httpd_senddone, httpd); if (result != ISC_R_SUCCESS) { - isc_nm_resumeread(handle); - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&httpd->sendhandle); httpd->state = RECV; + isc_nm_resumeread(handle); } return; -done: - isc_nmhandle_unref(handle); +cleanup_readhandle: + isc_nmhandle_detach(&httpd->readhandle); } void @@ -990,7 +994,7 @@ isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { httpd = ISC_LIST_HEAD(httpdmgr->running); while (httpd != NULL) { - isc_nmhandle_unref(httpd->handle); + isc_nm_cancelread(httpd->readhandle); httpd = ISC_LIST_NEXT(httpd, link); } UNLOCK(&httpdmgr->lock); @@ -1137,9 +1141,10 @@ httpd_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { return; } + isc_nmhandle_detach(&httpd->sendhandle); + httpd->state = RECV; isc_nm_resumeread(handle); - isc_nmhandle_unref(handle); } isc_result_t diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index b1c92ce348..991e14028d 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -110,9 +110,9 @@ isc_nmsocket_close(isc_nmsocket_t **sockp); */ void -isc_nmhandle_ref(isc_nmhandle_t *handle); +isc_nmhandle_attach(isc_nmhandle_t *handle, isc_nmhandle_t **dest); void -isc_nmhandle_unref(isc_nmhandle_t *handle); +isc_nmhandle_detach(isc_nmhandle_t **handlep); /*%< * Increment/decrement the reference counter in a netmgr handle, * but (unlike the attach/detach functions) do not change the pointer diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index abd4566669..ab8852d8d0 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -746,8 +746,7 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree) { sock->statichandle = NULL; if (sock->outerhandle != NULL) { - isc_nmhandle_unref(sock->outerhandle); - sock->outerhandle = NULL; + isc_nmhandle_detach(&sock->outerhandle); } if (sock->outer != NULL) { @@ -1107,6 +1106,13 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, (sock->type == isc_nm_udpsocket && atomic_load(&sock->client))) { INSIST(sock->statichandle == NULL); + + /* + * statichandle must be assigned, not attached; + * otherwise, if a handle was detached elsewhere + * it could never reach 0 references, and the + * handle and socket would never be freed. + */ sock->statichandle = handle; } @@ -1114,10 +1120,12 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, } void -isc_nmhandle_ref(isc_nmhandle_t *handle) { +isc_nmhandle_attach(isc_nmhandle_t *handle, isc_nmhandle_t **handlep) { REQUIRE(VALID_NMHANDLE(handle)); + REQUIRE(handlep != NULL && *handlep == NULL); isc_refcount_increment(&handle->references); + *handlep = handle; } bool @@ -1173,14 +1181,20 @@ nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) { } void -isc_nmhandle_unref(isc_nmhandle_t *handle) { +isc_nmhandle_detach(isc_nmhandle_t **handlep) { isc_nmsocket_t *sock = NULL; + isc_nmhandle_t *handle = NULL; - REQUIRE(VALID_NMHANDLE(handle)); + REQUIRE(handlep != NULL); + REQUIRE(VALID_NMHANDLE(*handlep)); + + handle = *handlep; + *handlep = NULL; if (isc_refcount_decrement(&handle->references) > 1) { return; } + /* We need an acquire memory barrier here */ (void)isc_refcount_current(&handle->references); @@ -1215,6 +1229,7 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) { } if (handle == sock->statichandle) { + /* statichandle is assigned, not attached. */ sock->statichandle = NULL; } @@ -1319,7 +1334,7 @@ isc__nm_uvreq_put(isc__nm_uvreq_t **req0, isc_nmsocket_t *sock) { } if (handle != NULL) { - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&handle); } isc__nmsocket_detach(&sock); diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 114279710c..e4dd3fecec 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -183,10 +183,10 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) { isc__nmsocket_detach(&sock); /* - * If the connect callback wants to hold on to the handle, - * it needs to attach to it. + * The connect callback should have attached to the handle. + * If it didn't, the socket will be closed now. */ - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&handle); } isc_result_t @@ -498,10 +498,10 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) { isc__nmsocket_detach(&csock); /* - * If the accept callback wants to hold on to the handle, - * it needs to attach to it. + * The accept callback should have attached to the handle. + * If it didn't, the socket will be closed now. */ - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&handle); return; error: @@ -959,8 +959,7 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, uvreq = isc__nm_uvreq_get(sock->mgr, sock); uvreq->uvbuf.base = (char *)region->base; uvreq->uvbuf.len = region->length; - uvreq->handle = handle; - isc_nmhandle_ref(uvreq->handle); + isc_nmhandle_attach(handle, &uvreq->handle); uvreq->cb.send = cb; uvreq->cbarg = cbarg; @@ -1003,7 +1002,6 @@ tcp_send_cb(uv_write_t *req, int status) { uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); sock = uvreq->handle->sock; - isc_nmhandle_unref(uvreq->handle); isc__nm_uvreq_put(&uvreq, sock); } @@ -1035,8 +1033,6 @@ tcp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { REQUIRE(sock->tid == isc_nm_tid()); REQUIRE(sock->type == isc_nm_tcpsocket); - - isc_nmhandle_ref(req->handle); r = uv_write(&req->uv_req.write, &sock->uv_handle.stream, &req->uvbuf, 1, tcp_send_cb); if (r < 0) { @@ -1143,6 +1139,7 @@ isc__nm_tcp_cancelread(isc_nmhandle_t *handle) { sock = handle->sock; + REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_tcpsocket); if (atomic_load(&sock->client) && sock->rcb.recv != NULL) { diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 107cde97b6..84bf28d206 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -98,8 +98,7 @@ dnstcp_readtimeout(uv_timer_t *timer) { REQUIRE(sock->tid == isc_nm_tid()); /* Close the TCP connection; its closure should fire ours. */ - isc_nmhandle_unref(sock->outerhandle); - sock->outerhandle = NULL; + isc_nmhandle_detach(&sock->outerhandle); } /* @@ -109,6 +108,7 @@ static isc_result_t dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmsocket_t *dnslistensock = (isc_nmsocket_t *)cbarg; isc_nmsocket_t *dnssock = NULL; + isc_nmhandle_t *readhandle = NULL; REQUIRE(VALID_NMSOCK(dnslistensock)); REQUIRE(dnslistensock->type == isc_nm_tcpdnslistener); @@ -135,8 +135,7 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc__nmsocket_attach(dnssock, &dnssock->self); - dnssock->outerhandle = handle; - isc_nmhandle_ref(dnssock->outerhandle); + isc_nmhandle_attach(handle, &dnssock->outerhandle); dnssock->peer = handle->sock->peer; dnssock->read_timeout = handle->sock->mgr->init; @@ -150,10 +149,15 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { uv_timer_start(&dnssock->timer, dnstcp_readtimeout, dnssock->read_timeout, 0); - isc_nmhandle_ref(handle); - result = isc_nm_read(handle, dnslisten_readcb, dnssock); + /* + * Add a reference to handle to keep it from being freed by + * the caller. It will be detached in dnslisted_readcb() when + * 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_unref(handle); + isc_nmhandle_detach(&readhandle); } isc__nmsocket_detach(&dnssock); @@ -190,17 +194,17 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) { */ len = dnslen(dnssock->buf); if (len <= dnssock->buf_len - 2) { - isc_nmhandle_t *dnshandle; + isc_nmhandle_t *dnshandle = NULL; + isc_nmsocket_t *listener = NULL; + if (atomic_load(&dnssock->client) && dnssock->statichandle != NULL) { - dnshandle = dnssock->statichandle; - isc_nmhandle_ref(dnshandle); + isc_nmhandle_attach(dnssock->statichandle, &dnshandle); } else { dnshandle = isc__nmhandle_get(dnssock, NULL, NULL); } - isc_nmsocket_t *listener = dnssock->listener; - + listener = dnssock->listener; if (listener != NULL && listener->rcb.recv != NULL) { listener->rcb.recv( dnshandle, ISC_R_SUCCESS, @@ -238,8 +242,8 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) { } /* - * We've got a read on our underlying socket, need to check if we have - * a complete DNS packet and, if so - call the callback + * We've got a read on our underlying socket. Check whether + * we have a complete DNS packet and, if so, call the callback. */ static void dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, @@ -254,18 +258,15 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, if (region == NULL || eresult != ISC_R_SUCCESS) { /* Connection closed */ - if (eresult != ISC_R_CANCELED) { - isc_nmhandle_unref(handle); - } dnssock->result = eresult; if (dnssock->self != NULL) { isc__nmsocket_detach(&dnssock->self); } isc__nmsocket_clearcb(dnssock); if (dnssock->outerhandle != NULL) { - isc_nmhandle_unref(dnssock->outerhandle); - dnssock->outerhandle = NULL; + isc_nmhandle_detach(&dnssock->outerhandle); } + isc_nmhandle_detach(&handle); return; } @@ -305,11 +306,11 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, if (atomic_load(&dnssock->sequential) || dnssock->rcb.recv == NULL) { /* - * Two reasons we might want to pause here: - * - If we're in sequential mode and we've received + * There are two reasons we might want to pause here: + * - We're in sequential mode and we've received * a whole packet, so we're done until it's been - * processed; - * - If we no longer have a read callback. + * processed; or + * - We no longer have a read callback. */ isc_nm_pauseread(dnssock->outerhandle); done = true; @@ -328,7 +329,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, } } - isc_nmhandle_unref(dnshandle); + isc_nmhandle_detach(&dnshandle); } while (!done); } @@ -460,12 +461,11 @@ resume_processing(void *arg) { if (sock->timer_initialized) { uv_timer_stop(&sock->timer); } - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&handle); } else if (sock->outerhandle != NULL) { result = isc_nm_resumeread(sock->outerhandle); if (result != ISC_R_SUCCESS) { - isc_nmhandle_unref(sock->outerhandle); - sock->outerhandle = NULL; + isc_nmhandle_detach(&sock->outerhandle); } } @@ -495,7 +495,7 @@ resume_processing(void *arg) { uv_timer_stop(&sock->timer); } atomic_store(&sock->outerhandle->sock->processing, true); - isc_nmhandle_unref(dnshandle); + isc_nmhandle_detach(&dnshandle); } while (atomic_load(&sock->ah) < TCPDNS_CLIENTS_PER_CONN); } @@ -508,6 +508,7 @@ tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { req->cb.send(req->handle, result, req->cbarg); isc_mem_put(req->sock->mgr->mctx, req->uvbuf.base, req->uvbuf.len); isc__nm_uvreq_put(&req, req->handle->sock); + isc_nmhandle_detach(&handle); } void @@ -522,11 +523,16 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) { result = ISC_R_NOTCONNECTED; if (atomic_load(&sock->active) && sock->outerhandle != NULL) { + isc_nmhandle_t *sendhandle = NULL; isc_region_t r; r.base = (unsigned char *)req->uvbuf.base; r.length = req->uvbuf.len; - result = isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb, req); + 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) { @@ -552,8 +558,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region, REQUIRE(sock->type == isc_nm_tcpdnssocket); uvreq = isc__nm_uvreq_get(sock->mgr, sock); - uvreq->handle = handle; - isc_nmhandle_ref(uvreq->handle); + isc_nmhandle_attach(handle, &uvreq->handle); uvreq->cb.send = cb; uvreq->cbarg = cbarg; @@ -563,13 +568,20 @@ 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; r.base = (unsigned char *)uvreq->uvbuf.base; r.length = uvreq->uvbuf.len; - return (isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb, - uvreq)); + 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); } else { isc__netievent_tcpdnssend_t *ievent = NULL; @@ -609,8 +621,7 @@ tcpdns_close_direct(isc_nmsocket_t *sock) { */ if (sock->outerhandle != NULL) { isc__nmsocket_clearcb(sock->outerhandle->sock); - isc_nmhandle_unref(sock->outerhandle); - sock->outerhandle = NULL; + isc_nmhandle_detach(&sock->outerhandle); } if (sock->listener != NULL) { isc__nmsocket_detach(&sock->listener); diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index 9a4013f666..d167b91727 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -159,7 +159,7 @@ udp_alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf) { } /* - * handle 'udplisten' async call - start listening on a socket. + * Asynchronous 'udplisten' call handler: start listening on a UDP socket. */ void isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) { @@ -311,7 +311,7 @@ isc__nm_udp_stoplistening(isc_nmsocket_t *sock) { } /* - * handle 'udpstop' async call - stop listening on a socket. + * Asynchronous 'udpstop' call handler: stop listening on a UDP socket. */ void isc__nm_async_udpstop(isc__networker_t *worker, isc__netievent_t *ev0) { @@ -375,9 +375,9 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, #endif /* - * Three reasons to return now without processing: - * - If addr == NULL that's the end of stream - we can - * free the buffer and bail. + * Three possible reasons to return now without processing: + * - If addr == NULL, in which case it's the end of stream; + * we can free the buffer and bail. * - If we're simulating a firewall blocking UDP packets * bigger than 'maxudp' bytes for testing purposes. * - If the socket is no longer active. @@ -395,11 +395,7 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, result = isc_sockaddr_fromsockaddr(&sockaddr, addr); RUNTIME_CHECK(result == ISC_R_SUCCESS); - if (!atomic_load(&sock->connected)) { - nmhandle = isc__nmhandle_get(sock, &sockaddr, NULL); - } else { - nmhandle = sock->statichandle; - } + nmhandle = isc__nmhandle_get(sock, &sockaddr, NULL); region.base = (unsigned char *)buf->base; region.length = nrecv; @@ -418,13 +414,13 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, * If the recv callback wants to hold on to the handle, * it needs to attach to it. */ - isc_nmhandle_unref(nmhandle); + isc_nmhandle_detach(&nmhandle); } /* - * isc__nm_udp_send sends buf to a peer on a socket. - * It tries to find a proper sibling/child socket so that we won't have - * to jump to another thread. + * Send the data in 'region' to a peer via a UDP socket. We try to find + * a proper sibling/child socket so that we won't have to jump to another + * thread. */ isc_result_t isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, @@ -446,7 +442,7 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, * we need to do so here. */ if (maxudp != 0 && region->length > maxudp) { - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&handle); return (ISC_R_SUCCESS); } @@ -486,8 +482,7 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, uvreq->uvbuf.base = (char *)region->base; uvreq->uvbuf.len = region->length; - uvreq->handle = handle; - isc_nmhandle_ref(uvreq->handle); + isc_nmhandle_attach(handle, &uvreq->handle); uvreq->cb.send = cb; uvreq->cbarg = cbarg; @@ -514,7 +509,7 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, } /* - * handle 'udpsend' async event - send a packet on the socket + * Asynchronous 'udpsend' event handler: send a packet on a UDP socket. */ void isc__nm_async_udpsend(isc__networker_t *worker, isc__netievent_t *ev0) { @@ -531,9 +526,6 @@ isc__nm_async_udpsend(isc__networker_t *worker, isc__netievent_t *ev0) { } } -/* - * udp_send_cb - callback - */ static void udp_send_cb(uv_udp_send_t *req, int status) { isc_result_t result = ISC_R_SUCCESS; @@ -549,7 +541,6 @@ udp_send_cb(uv_udp_send_t *req, int status) { } uvreq->cb.send(uvreq->handle, result, uvreq->cbarg); - isc_nmhandle_unref(uvreq->handle); isc__nm_uvreq_put(&uvreq, uvreq->sock); } @@ -570,7 +561,6 @@ udp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, return (ISC_R_CANCELED); } - isc_nmhandle_ref(req->handle); 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); diff --git a/lib/isc/win32/libisc.def.in b/lib/isc/win32/libisc.def.in index bc5de5aa8e..8b87058f7d 100644 --- a/lib/isc/win32/libisc.def.in +++ b/lib/isc/win32/libisc.def.in @@ -436,15 +436,15 @@ isc_netaddr_setzone isc_netaddr_totext isc_netaddr_unspec isc_netscope_pton +isc_nmhandle_attach +isc_nmhandle_detach isc_nmhandle_getdata isc_nmhandle_getextra isc_nmhandle_is_stream isc_nmhandle_netmgr isc_nmhandle_localaddr isc_nmhandle_peeraddr -isc_nmhandle_ref isc_nmhandle_setdata -isc_nmhandle_unref isc_nm_cancelread isc_nm_closedown isc_nm_destroy diff --git a/lib/isccc/ccmsg.c b/lib/isccc/ccmsg.c index 5e80cf82da..98b67622ee 100644 --- a/lib/isccc/ccmsg.c +++ b/lib/isccc/ccmsg.c @@ -108,7 +108,6 @@ recv_data(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, done: isc_nm_pauseread(handle); ccmsg->cb(handle, ccmsg->result, ccmsg->cbarg); - isc_nmhandle_unref(handle); } void @@ -149,18 +148,14 @@ isccc_ccmsg_readmessage(isccc_ccmsg_t *ccmsg, isc_nm_cb_t cb, void *cbarg) { ccmsg->result = ISC_R_UNEXPECTED; /* unknown right now */ ccmsg->length_received = false; - isc_nmhandle_ref(ccmsg->handle); if (ccmsg->reading) { result = isc_nm_resumeread(ccmsg->handle); } else { result = isc_nm_read(ccmsg->handle, recv_data, ccmsg); ccmsg->reading = true; } - if (result == ISC_R_CANCELED) { + if (result != ISC_R_SUCCESS) { ccmsg->reading = false; - } else if (result != ISC_R_SUCCESS) { - ccmsg->reading = false; - isc_nmhandle_unref(ccmsg->handle); } return (result); @@ -172,6 +167,7 @@ isccc_ccmsg_cancelread(isccc_ccmsg_t *ccmsg) { if (ccmsg->reading) { isc_nm_cancelread(ccmsg->handle); + ccmsg->reading = false; } } diff --git a/lib/isccc/include/isccc/ccmsg.h b/lib/isccc/include/isccc/ccmsg.h index baa2c19bb3..31d1490824 100644 --- a/lib/isccc/include/isccc/ccmsg.h +++ b/lib/isccc/include/isccc/ccmsg.h @@ -58,7 +58,9 @@ void isccc_ccmsg_init(isc_mem_t *mctx, isc_nmhandle_t *handle, isccc_ccmsg_t *ccmsg); /*% * Associate a cc message state with a given memory context and - * netmgr handle. + * netmgr handle. (Note that the caller must hold a reference to + * the handle during asynchronous ccmsg operations; the ccmsg code + * does not hold the reference itself.) * * Requires: * diff --git a/lib/ns/client.c b/lib/ns/client.c index 7e1bde9875..db3d40fc4e 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -275,7 +275,7 @@ static void client_senddone(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { ns_client_t *client = cbarg; - REQUIRE(client->handle == handle); + REQUIRE(client->sendhandle == handle); CTRACE("senddone"); if (result != ISC_R_SUCCESS) { @@ -284,7 +284,7 @@ client_senddone(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { "send failed: %s", isc_result_totext(result)); } - isc_nmhandle_unref(handle); + isc_nmhandle_detach(&client->sendhandle); } static void @@ -325,13 +325,18 @@ client_allocsendbuf(ns_client_t *client, isc_buffer_t *buffer, static isc_result_t 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); - - INSIST(client->handle != NULL); - - return (isc_nm_send(client->handle, &r, client_senddone, client)); + 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); } void @@ -383,7 +388,6 @@ done: } ns_client_drop(client, result); - isc_nmhandle_unref(client->handle); } void @@ -405,17 +409,13 @@ ns_client_send(ns_client_t *client) { isc_region_t zr; #endif /* HAVE_DNSTAP */ + REQUIRE(NS_CLIENT_VALID(client)); + /* * XXXWPK TODO * Delay the response according to the -T delay option */ - REQUIRE(NS_CLIENT_VALID(client)); - /* - * We need to do it to make sure the client and handle - * won't disappear from under us with client_senddone. - */ - env = ns_interfacemgr_getaclenv(client->manager->interface->mgr); CTRACE("send"); @@ -591,12 +591,7 @@ renderend: respsize = isc_buffer_usedlength(&buffer); - isc_nmhandle_ref(client->handle); result = client_sendpkg(client, &buffer); - if (result != ISC_R_SUCCESS) { - /* We won't get a callback to clean it up */ - isc_nmhandle_unref(client->handle); - } switch (isc_sockaddr_pf(&client->peeraddr)) { case AF_INET: @@ -626,12 +621,7 @@ renderend: respsize = isc_buffer_usedlength(&buffer); - isc_nmhandle_ref(client->handle); result = client_sendpkg(client, &buffer); - if (result != ISC_R_SUCCESS) { - /* We won't get a callback to clean it up */ - isc_nmhandle_unref(client->handle); - } switch (isc_sockaddr_pf(&client->peeraddr)) { case AF_INET: @@ -1683,6 +1673,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, ns__client_put_cb); client->handle = handle; } + if (isc_nmhandle_is_stream(handle)) { client->attributes |= NS_CLIENTATTR_TCP; } @@ -1697,8 +1688,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_buffer_add(&tbuffer, region->length); buffer = &tbuffer; - client->peeraddr = isc_nmhandle_peeraddr(client->handle); - + client->peeraddr = isc_nmhandle_peeraddr(handle); client->peeraddr_valid = true; reqsize = isc_buffer_usedlength(buffer); @@ -1968,8 +1958,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_netaddr_fromsockaddr(&client->destaddr, &client->manager->interface->addr); } else { - isc_sockaddr_t sockaddr = - isc_nmhandle_localaddr(client->handle); + isc_sockaddr_t sockaddr = isc_nmhandle_localaddr(handle); isc_netaddr_fromsockaddr(&client->destaddr, &sockaddr); } @@ -2174,8 +2163,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, &client->requesttime, NULL, buffer); #endif /* HAVE_DNSTAP */ - isc_nmhandle_ref(client->handle); - ns_query_start(client); + ns_query_start(client, handle); break; case dns_opcode_update: CTRACE("update"); @@ -2185,14 +2173,12 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, &client->requesttime, NULL, buffer); #endif /* HAVE_DNSTAP */ ns_client_settimeout(client, 60); - isc_nmhandle_ref(client->handle); - ns_update_start(client, sigresult); + ns_update_start(client, handle, sigresult); break; case dns_opcode_notify: CTRACE("notify"); ns_client_settimeout(client, 60); - isc_nmhandle_ref(client->handle); - ns_notify_start(client); + ns_notify_start(client, handle); break; case dns_opcode_iquery: CTRACE("iquery"); diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index bf386e869e..539866c0e7 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -183,14 +183,19 @@ struct ns_client { isc_task_t * task; dns_view_t * view; dns_dispatch_t * dispatch; - isc_nmhandle_t * handle; - unsigned char * tcpbuf; - dns_message_t * message; - unsigned char * sendbuf; - dns_rdataset_t * opt; - uint16_t udpsize; - uint16_t extflags; - int16_t ednsversion; /* -1 noedns */ + isc_nmhandle_t * handle; /* Permanent pointer to handle */ + isc_nmhandle_t * sendhandle; /* Waiting for send callback */ + isc_nmhandle_t * reqhandle; /* Waiting for request callback + (query, update, notify) */ + isc_nmhandle_t *fetchhandle; /* Waiting for recursive fetch */ + isc_nmhandle_t *updatehandle; /* Waiting for update callback */ + unsigned char * tcpbuf; + dns_message_t * message; + unsigned char * sendbuf; + dns_rdataset_t *opt; + uint16_t udpsize; + uint16_t extflags; + int16_t ednsversion; /* -1 noedns */ void (*cleanup)(ns_client_t *); void (*shutdown)(void *arg, isc_result_t result); void * shutdown_arg; diff --git a/lib/ns/include/ns/notify.h b/lib/ns/include/ns/notify.h index 555a6f7915..6c39c4344f 100644 --- a/lib/ns/include/ns/notify.h +++ b/lib/ns/include/ns/notify.h @@ -29,7 +29,7 @@ ***/ void -ns_notify_start(ns_client_t *client); +ns_notify_start(ns_client_t *client, isc_nmhandle_t *handle); /*%< * Examines the incoming message to determine appropriate zone. diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index 18e74dec15..a470c0da56 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -209,7 +209,7 @@ void ns_query_free(ns_client_t *client); void -ns_query_start(ns_client_t *client); +ns_query_start(ns_client_t *client, isc_nmhandle_t *handle); void ns_query_cancel(ns_client_t *client); diff --git a/lib/ns/include/ns/update.h b/lib/ns/include/ns/update.h index e3372c9a08..c86915555c 100644 --- a/lib/ns/include/ns/update.h +++ b/lib/ns/include/ns/update.h @@ -37,6 +37,7 @@ ***/ void -ns_update_start(ns_client_t *client, isc_result_t sigresult); +ns_update_start(ns_client_t *client, isc_nmhandle_t *handle, + isc_result_t sigresult); #endif /* NS_UPDATE_H */ diff --git a/lib/ns/notify.c b/lib/ns/notify.c index 3dafc8cdbf..dec74e8f46 100644 --- a/lib/ns/notify.c +++ b/lib/ns/notify.c @@ -54,7 +54,7 @@ respond(ns_client_t *client, isc_result_t result) { } if (msg_result != ISC_R_SUCCESS) { ns_client_drop(client, msg_result); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->reqhandle); return; } message->rcode = rcode; @@ -65,11 +65,11 @@ respond(ns_client_t *client, isc_result_t result) { } ns_client_send(client); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->reqhandle); } void -ns_notify_start(ns_client_t *client) { +ns_notify_start(ns_client_t *client, isc_nmhandle_t *handle) { dns_message_t *request = client->message; isc_result_t result; dns_name_t *zonename; @@ -79,6 +79,11 @@ ns_notify_start(ns_client_t *client) { char tsigbuf[DNS_NAME_FORMATSIZE * 2 + sizeof(": TSIG '' ()")]; dns_tsigkey_t *tsigkey; + /* + * Attach to the request handle + */ + isc_nmhandle_attach(handle, &client->reqhandle); + /* * Interpret the question section. */ diff --git a/lib/ns/query.c b/lib/ns/query.c index 6147ff3705..3a64b0fc4e 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -550,7 +550,7 @@ query_send(ns_client_t *client) { inc_stats(client, counter); ns_client_send(client); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->reqhandle); } static void @@ -577,7 +577,7 @@ query_error(ns_client_t *client, isc_result_t result, int line) { log_queryerror(client, result, line, loglevel); ns_client_error(client, result); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->reqhandle); } static void @@ -590,7 +590,7 @@ query_next(ns_client_t *client, isc_result_t result) { inc_stats(client, ns_statscounter_failure); } ns_client_drop(client, result); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->reqhandle); } static inline void @@ -2475,7 +2475,7 @@ prefetch_done(isc_task_t *task, isc_event_t *event) { } free_devent(client, &event, &devent); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->fetchhandle); } static void @@ -2518,7 +2518,7 @@ query_prefetch(ns_client_t *client, dns_name_t *qname, peeraddr = NULL; } - isc_nmhandle_ref(client->handle); + isc_nmhandle_attach(client->handle, &client->fetchhandle); options = client->query.fetchoptions | DNS_FETCHOPT_PREFETCH; result = dns_resolver_createfetch( client->view->resolver, qname, rdataset->type, NULL, NULL, NULL, @@ -2527,7 +2527,7 @@ query_prefetch(ns_client_t *client, dns_name_t *qname, &client->query.prefetch); if (result != ISC_R_SUCCESS) { ns_client_putrdataset(client, &tmprdataset); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->fetchhandle); } dns_rdataset_clearprefetch(rdataset); @@ -2732,7 +2732,7 @@ query_rpzfetch(ns_client_t *client, dns_name_t *qname, dns_rdatatype_t type) { } options = client->query.fetchoptions; - isc_nmhandle_ref(client->handle); + isc_nmhandle_attach(client->handle, &client->fetchhandle); result = dns_resolver_createfetch( client->view->resolver, qname, type, NULL, NULL, NULL, peeraddr, client->message->id, options, 0, NULL, client->task, @@ -2740,7 +2740,7 @@ query_rpzfetch(ns_client_t *client, dns_name_t *qname, dns_rdatatype_t type) { &client->query.prefetch); if (result != ISC_R_SUCCESS) { ns_client_putrdataset(client, &tmprdataset); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->fetchhandle); } } @@ -5703,6 +5703,8 @@ fetch_callback(isc_task_t *task, isc_event_t *event) { } UNLOCK(&client->manager->reclock); + isc_nmhandle_detach(&client->fetchhandle); + client->query.attributes &= ~NS_QUERYATTR_RECURSING; client->state = NS_CLIENTSTATE_WORKING; @@ -5748,7 +5750,6 @@ fetch_callback(isc_task_t *task, isc_event_t *event) { } dns_resolver_destroyfetch(&fetch); - isc_nmhandle_unref(client->handle); } /*% @@ -5940,14 +5941,14 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, peeraddr = &client->peeraddr; } - isc_nmhandle_ref(client->handle); + isc_nmhandle_attach(client->handle, &client->fetchhandle); result = dns_resolver_createfetch( client->view->resolver, qname, qtype, qdomain, nameservers, NULL, peeraddr, client->message->id, client->query.fetchoptions, 0, NULL, client->task, fetch_callback, client, rdataset, sigrdataset, &client->query.fetch); if (result != ISC_R_SUCCESS) { - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->fetchhandle); ns_client_putrdataset(client, &rdataset); if (sigrdataset != NULL) { ns_client_putrdataset(client, &sigrdataset); @@ -11108,7 +11109,7 @@ log_queryerror(ns_client_t *client, isc_result_t result, int line, int level) { } void -ns_query_start(ns_client_t *client) { +ns_query_start(ns_client_t *client, isc_nmhandle_t *handle) { isc_result_t result; dns_message_t *message; dns_rdataset_t *rdataset; @@ -11118,6 +11119,11 @@ ns_query_start(ns_client_t *client) { REQUIRE(NS_CLIENT_VALID(client)); + /* + * Attach to the request handle + */ + isc_nmhandle_attach(handle, &client->reqhandle); + message = client->message; saved_extflags = client->extflags; saved_flags = client->message->flags; diff --git a/lib/ns/tests/Makefile.am b/lib/ns/tests/Makefile.am index 092360cebd..38903266d5 100644 --- a/lib/ns/tests/Makefile.am +++ b/lib/ns/tests/Makefile.am @@ -32,7 +32,8 @@ notify_test_SOURCES = \ notify_test_LDFLAGS = \ $(LDFLAGS) \ - -Wl,--wrap=isc_nmhandle_unref + -Wl,--wrap=isc_nmhandle_attach \ + -Wl,--wrap=isc_nmhandle_detach query_test_SOURCES = \ query_test.c \ @@ -40,7 +41,7 @@ query_test_SOURCES = \ query_test_LDFLAGS = \ $(LDFLAGS) \ - -Wl,--wrap=isc_nmhandle_unref + -Wl,--wrap=isc_nmhandle_detach endif diff --git a/lib/ns/tests/notify_test.c b/lib/ns/tests/notify_test.c index 4e3c5a63d9..05f1133535 100644 --- a/lib/ns/tests/notify_test.c +++ b/lib/ns/tests/notify_test.c @@ -86,6 +86,7 @@ static void notify_start(void **state) { isc_result_t result; ns_client_t *client = NULL; + isc_nmhandle_t *handle = NULL; dns_message_t *nmsg = NULL; unsigned char ndata[4096]; isc_buffer_t nbuf; @@ -130,13 +131,16 @@ notify_start(void **state) { client->message = nmsg; nmsg = NULL; client->sendcb = check_response; - ns_notify_start(client); + ns_notify_start(client, client->handle); /* * Clean up */ ns_test_cleanup_zone(); - isc_nmhandle_unref(client->handle); + + handle = client->handle; + isc_nmhandle_detach(&client->handle); + isc_nmhandle_detach(&handle); } int diff --git a/lib/ns/tests/nstest.c b/lib/ns/tests/nstest.c index e3a6990fc9..bc421272aa 100644 --- a/lib/ns/tests/nstest.c +++ b/lib/ns/tests/nstest.c @@ -78,10 +78,32 @@ atomic_uint_fast32_t client_refs[32]; atomic_uintptr_t client_addrs[32]; void -__wrap_isc_nmhandle_unref(isc_nmhandle_t *handle); +__wrap_isc_nmhandle_attach(isc_nmhandle_t *source, isc_nmhandle_t **targetp); +void +__wrap_isc_nmhandle_detach(isc_nmhandle_t **handlep); void -__wrap_isc_nmhandle_unref(isc_nmhandle_t *handle) { +__wrap_isc_nmhandle_attach(isc_nmhandle_t *source, isc_nmhandle_t **targetp) { + ns_client_t *client = (ns_client_t *)source; + int i; + + for (i = 0; i < 32; i++) { + if (atomic_load(&client_addrs[i]) == (uintptr_t)client) { + break; + } + } + INSIST(i < 32); + INSIST(atomic_load(&client_refs[i]) > 0); + + atomic_fetch_add(&client_refs[i], 1); + + *targetp = source; + return; +} + +void +__wrap_isc_nmhandle_detach(isc_nmhandle_t **handlep) { + isc_nmhandle_t *handle = *handlep; ns_client_t *client = (ns_client_t *)handle; int i; @@ -90,7 +112,7 @@ __wrap_isc_nmhandle_unref(isc_nmhandle_t *handle) { break; } } - REQUIRE(i < 32); + INSIST(i < 32); if (atomic_fetch_sub(&client_refs[i], 1) == 1) { dns_view_detach(&client->view); @@ -99,6 +121,8 @@ __wrap_isc_nmhandle_unref(isc_nmhandle_t *handle) { ns__client_put_cb(client); isc_mem_put(mctx, client, sizeof(ns_client_t)); } + + *handlep = NULL; return; } @@ -743,11 +767,13 @@ create_qctx_for_client(ns_client_t *client, query_ctx_t **qctxp) { saved_hook_table = ns__hook_table; ns__hook_table = query_hooks; - ns_query_start(client); + ns_query_start(client, client->handle); ns__hook_table = saved_hook_table; ns_hooktable_free(mctx, (void **)&query_hooks); + isc_nmhandle_detach(&client->reqhandle); + if (*qctxp == NULL) { return (ISC_R_NOMEMORY); } else { @@ -760,6 +786,7 @@ ns_test_qctx_create(const ns_test_qctx_create_params_t *params, query_ctx_t **qctxp) { ns_client_t *client = NULL; isc_result_t result; + isc_nmhandle_t *handle = NULL; REQUIRE(params != NULL); REQUIRE(params->qname != NULL); @@ -810,17 +837,19 @@ ns_test_qctx_create(const ns_test_qctx_create_params_t *params, } /* - * Reference count for "client" is now at 2, so decrement it in order - * for it to drop to zero when "qctx" gets destroyed. + * The reference count for "client" is now at 2, so we need to + * decrement it in order for it to drop to zero when "qctx" gets + * destroyed. */ - isc_nmhandle_unref(client->handle); + handle = client->handle; + isc_nmhandle_detach(&handle); return (ISC_R_SUCCESS); destroy_query: dns_message_destroy(&client->message); detach_client: - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->handle); return (result); } @@ -842,7 +871,7 @@ ns_test_qctx_destroy(query_ctx_t **qctxp) { dns_db_detach(&qctx->db); } if (qctx->client != NULL) { - isc_nmhandle_unref(qctx->client->handle); + isc_nmhandle_detach(&qctx->client->handle); } isc_mem_put(mctx, qctx, sizeof(*qctx)); diff --git a/lib/ns/tests/wrap.c b/lib/ns/tests/wrap.c index 41b8449fc6..810a1d5d03 100644 --- a/lib/ns/tests/wrap.c +++ b/lib/ns/tests/wrap.c @@ -26,15 +26,22 @@ #include /* - * This overrides calls to isc_nmhandle_unref(), sending them to - * __wrap_isc_nmhandle_unref(), when libtool is in use and LD_WRAP - * can't be used. + * This overrides calls to isc_nmhandle_attach/detach(), sending them to + * __wrap_isc_nmhandle_attach/detach() instead, when libtool is in use + * and LD_WRAP can't be used. */ +void +__wrap_isc_nmhandle_attach(isc_nmhandle_t *source, isc_nmhandle_t **targetp); extern void -__wrap_isc_nmhandle_unref(isc_nmhandle_t *handle); +__wrap_isc_nmhandle_detach(isc_nmhandle_t **handlep); void -isc_nmhandle_unref(isc_nmhandle_t *handle) { - __wrap_isc_nmhandle_unref(handle); +isc_nmhandle_attach(isc_nmhandle_t *source, isc_nmhandle_t **targetp) { + __wrap_isc_nmhandle_attach(source, targetp); +} + +void +isc_nmhandle_detach(isc_nmhandle_t **handlep) { + __wrap_isc_nmhandle_detach(handlep); } diff --git a/lib/ns/update.c b/lib/ns/update.c index 9b780a62b9..d440955acd 100644 --- a/lib/ns/update.c +++ b/lib/ns/update.c @@ -1567,7 +1567,7 @@ send_update_event(ns_client_t *client, dns_zone_t *zone) { client->nupdates++; event->ev_arg = client; - isc_nmhandle_ref(client->handle); + isc_nmhandle_attach(client->handle, &client->updatehandle); dns_zone_gettask(zone, &zonetask); isc_task_send(zonetask, ISC_EVENT_PTR(&event)); @@ -1585,7 +1585,6 @@ respond(ns_client_t *client, isc_result_t result) { client->message->rcode = dns_result_torcode(result); ns_client_send(client); - isc_nmhandle_unref(client->handle); return; msg_failure: @@ -1594,17 +1593,23 @@ msg_failure: "could not create update response message: %s", isc_result_totext(msg_result)); ns_client_drop(client, msg_result); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->reqhandle); } void -ns_update_start(ns_client_t *client, isc_result_t sigresult) { +ns_update_start(ns_client_t *client, isc_nmhandle_t *handle, + isc_result_t sigresult) { dns_message_t *request = client->message; isc_result_t result; dns_name_t *zonename; dns_rdataset_t *zone_rdataset; dns_zone_t *zone = NULL, *raw = NULL; + /* + * Attach to the request handle + */ + isc_nmhandle_attach(handle, &client->reqhandle); + /* * Interpret the zone section. */ @@ -1673,6 +1678,8 @@ ns_update_start(ns_client_t *client, isc_result_t sigresult) { default: FAILC(DNS_R_NOTAUTH, "not authoritative for update zone"); } + + isc_nmhandle_detach(&client->reqhandle); return; failure: @@ -1690,6 +1697,7 @@ failure: if (zone != NULL) { dns_zone_detach(&zone); } + isc_nmhandle_detach(&client->reqhandle); } /*% @@ -3497,6 +3505,7 @@ common: } uev->ev_type = DNS_EVENT_UPDATEDONE; uev->ev_action = updatedone_action; + isc_task_send(client->task, &event); INSIST(ver == NULL); @@ -3510,8 +3519,9 @@ updatedone_action(isc_task_t *task, isc_event_t *event) { UNUSED(task); - INSIST(event->ev_type == DNS_EVENT_UPDATEDONE); - INSIST(task == client->task); + REQUIRE(event->ev_type == DNS_EVENT_UPDATEDONE); + REQUIRE(task == client->task); + REQUIRE(client->updatehandle == client->handle); INSIST(client->nupdates > 0); switch (uev->result) { @@ -3528,16 +3538,18 @@ updatedone_action(isc_task_t *task, isc_event_t *event) { if (uev->zone != NULL) { dns_zone_detach(&uev->zone); } + client->nupdates--; + respond(client, uev->result); + isc_event_free(&event); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->updatehandle); } /*% * Update forwarding support. */ - static void forward_fail(isc_task_t *task, isc_event_t *event) { ns_client_t *client = (ns_client_t *)event->ev_arg; @@ -3548,7 +3560,7 @@ forward_fail(isc_task_t *task, isc_event_t *event) { client->nupdates--; respond(client, DNS_R_SERVFAIL); isc_event_free(&event); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->updatehandle); } static void @@ -3568,6 +3580,7 @@ forward_callback(void *arg, isc_result_t result, dns_message_t *answer) { uev->answer = answer; inc_stats(client, zone, ns_statscounter_updaterespfwd); } + isc_task_send(client->task, ISC_EVENT_PTR(&uev)); dns_zone_detach(&zone); } @@ -3584,7 +3597,7 @@ forward_done(isc_task_t *task, isc_event_t *event) { ns_client_sendraw(client, uev->answer); dns_message_destroy(&uev->answer); isc_event_free(&event); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->updatehandle); } static void @@ -3636,7 +3649,7 @@ send_forward_event(ns_client_t *client, dns_zone_t *zone) { namebuf, classbuf); dns_zone_gettask(zone, &zonetask); - isc_nmhandle_ref(client->handle); + isc_nmhandle_attach(client->handle, &client->updatehandle); isc_task_send(zonetask, ISC_EVENT_PTR(&event)); if (event != NULL) { diff --git a/lib/ns/xfrout.c b/lib/ns/xfrout.c index b6e46578b5..951720bcea 100644 --- a/lib/ns/xfrout.c +++ b/lib/ns/xfrout.c @@ -16,7 +16,6 @@ #include #include #include -#include #include #include @@ -34,7 +33,6 @@ #include #include #include -#include #include #include #include @@ -1192,7 +1190,7 @@ failure: NS_LOGMODULE_XFER_OUT, ISC_LOG_DEBUG(3), "zone transfer setup failed"); ns_client_error(client, result); - isc_nmhandle_unref(client->handle); + isc_nmhandle_detach(&client->reqhandle); } } @@ -1276,11 +1274,6 @@ xfrout_ctx_create(isc_mem_t *mctx, ns_client_t *client, unsigned int id, xfr->txmem = mem; xfr->txmemlen = len; -#if 0 - CHECK(dns_timer_setidle(xfr->client->timer, - maxtime,idletime,false)); -#endif /* if 0 */ - /* * Register a shutdown callback with the client, so that we * can stop the transfer immediately when the client task @@ -1572,15 +1565,17 @@ sendstream(xfrout_ctx_t *xfr) { xfrout_log(xfr, ISC_LOG_DEBUG(8), "sending TCP message of %d bytes", used.length); - CHECK(isc_nm_send(xfr->client->handle, &used, xfrout_senddone, - xfr)); + isc_nmhandle_attach(xfr->client->handle, + &xfr->client->sendhandle); + CHECK(isc_nm_send(xfr->client->sendhandle, &used, + xfrout_senddone, xfr)); xfr->sends++; xfr->cbytes = used.length; } else { xfrout_log(xfr, ISC_LOG_DEBUG(8), "sending IXFR UDP response"); ns_client_send(xfr->client); xfr->stream->methods->pause(xfr->stream); - isc_nmhandle_unref(xfr->client->handle); + isc_nmhandle_detach(&xfr->client->reqhandle); xfrout_ctx_destroy(&xfr); return; } @@ -1623,6 +1618,10 @@ failure: return; } + if (xfr->client->sendhandle != NULL) { + isc_nmhandle_detach(&xfr->client->sendhandle); + } + xfrout_fail(xfr, result, "sending zone data"); } @@ -1675,6 +1674,8 @@ xfrout_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { xfr->sends--; INSIST(xfr->sends == 0); + isc_nmhandle_detach(&xfr->client->sendhandle); + /* * Update transfer statistics if sending succeeded, accounting for the * two-byte TCP length prefix included in the number of bytes sent. @@ -1684,10 +1685,6 @@ xfrout_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { xfr->stats.nbytes += xfr->cbytes; } -#if 0 - (void)isc_timer_touch(xfr->client->timer); -#endif /* if 0 */ - if (xfr->shuttingdown) { xfrout_maybe_destroy(xfr); } else if (result != ISC_R_SUCCESS) { @@ -1716,9 +1713,12 @@ xfrout_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { (unsigned int)(msecs % 1000), (unsigned int)persec, xfr->end_serial); + /* + * We're done, unreference the handle and destroy the xfr + * context. + */ + isc_nmhandle_detach(&xfr->client->reqhandle); xfrout_ctx_destroy(&xfr); - /* We're done, unreference the handle */ - isc_nmhandle_unref(handle); } } @@ -1732,23 +1732,11 @@ xfrout_fail(xfrout_ctx_t *xfr, isc_result_t result, const char *msg) { static void xfrout_maybe_destroy(xfrout_ctx_t *xfr) { - INSIST(xfr->shuttingdown); -#if 0 - if (xfr->sends > 0) { - /* - * If we are currently sending, cancel it and wait for - * cancel event before destroying the context. - */ - isc_socket_cancel(xfr->client->tcpsocket,xfr->client->task, - ISC_SOCKCANCEL_SEND); - } else { -#endif /* if 0 */ + REQUIRE(xfr->shuttingdown); + ns_client_drop(xfr->client, ISC_R_CANCELED); - isc_nmhandle_unref(xfr->client->handle); + isc_nmhandle_detach(&xfr->client->reqhandle); xfrout_ctx_destroy(&xfr); -#if 0 -} -#endif /* if 0 */ } static void From 89c534d3b9eddba5e2aaaaf54a5ff24bbe38257a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 11 Sep 2020 10:53:31 +0200 Subject: [PATCH 4/8] properly lock the setting/unsetting of callbacks in isc_nmsocket_t changes to socket callback functions were not thread safe. --- lib/isc/netmgr/netmgr-int.h | 13 ++---- lib/isc/netmgr/netmgr.c | 6 +-- lib/isc/netmgr/tcp.c | 86 +++++++++++++++++++++++++++---------- lib/isc/netmgr/tcpdns.c | 50 +++++++++++++-------- lib/isc/netmgr/udp.c | 28 ++++++++---- 5 files changed, 122 insertions(+), 61 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 7c02d1e6da..633e1a0e96 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -146,13 +146,6 @@ typedef enum isc__netievent_type { typedef union { isc_nm_recv_cb_t recv; - isc_nm_cb_t connect; - isc_nm_accept_cb_t accept; -} isc__nm_readcb_t; - -typedef union { - isc_nm_recv_cb_t recv; - isc_nm_accept_cb_t accept; isc_nm_cb_t send; isc_nm_cb_t connect; } isc__nm_cb_t; @@ -526,10 +519,10 @@ struct isc_nmsocket { */ isc_nm_opaquecb_t closehandle_cb; - isc__nm_readcb_t rcb; - void *rcbarg; + isc_nm_recv_cb_t recv_cb; + void *recv_cbarg; - isc__nm_cb_t accept_cb; + isc_nm_accept_cb_t accept_cb; void *accept_cbarg; }; diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index ab8852d8d0..4c6fdde1b2 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -1000,9 +1000,9 @@ void isc__nmsocket_clearcb(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); - sock->rcb.recv = NULL; - sock->rcbarg = NULL; - sock->accept_cb.accept = NULL; + sock->recv_cb = NULL; + sock->recv_cbarg = NULL; + sock->accept_cb = NULL; sock->accept_cbarg = NULL; } diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index e4dd3fecec..822823cb81 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -262,7 +262,7 @@ isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, nsock = isc_mem_get(mgr->mctx, sizeof(*nsock)); isc__nmsocket_init(nsock, mgr, isc_nm_tcplistener, iface); - nsock->accept_cb.accept = accept_cb; + nsock->accept_cb = accept_cb; nsock->accept_cbarg = accept_cbarg; nsock->extrahandlesize = extrahandlesize; nsock->backlog = backlog; @@ -434,6 +434,8 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) { struct sockaddr_storage ss; isc_sockaddr_t local; int r; + isc_nm_accept_cb_t accept_cb; + void *accept_cbarg; REQUIRE(isc__nm_in_netthread()); REQUIRE(ssock->type == isc_nm_tcplistener); @@ -488,9 +490,14 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) { handle = isc__nmhandle_get(csock, NULL, &local); - INSIST(ssock->accept_cb.accept != NULL); + LOCK(&ssock->lock); + INSIST(ssock->accept_cb != NULL); + accept_cb = ssock->accept_cb; + accept_cbarg = ssock->accept_cbarg; + UNLOCK(&ssock->lock); + csock->read_timeout = ssock->mgr->init; - ssock->accept_cb.accept(handle, ISC_R_SUCCESS, ssock->accept_cbarg); + accept_cb(handle, ISC_R_SUCCESS, accept_cbarg); /* * csock is now attached to the handle. @@ -583,6 +590,8 @@ tcp_listenclose_cb(uv_handle_t *handle) { 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()); @@ -603,10 +612,15 @@ readtimeout_cb(uv_timer_t *handle) { if (sock->quota) { isc_quota_detach(&sock->quota); } - if (sock->rcb.recv != NULL) { - sock->rcb.recv(sock->statichandle, ISC_R_TIMEDOUT, NULL, - sock->rcbarg); - isc__nmsocket_clearcb(sock); + + LOCK(&sock->lock); + cb = sock->recv_cb; + cbarg = sock->recv_cbarg; + isc__nmsocket_clearcb(sock); + UNLOCK(&sock->lock); + + if (cb != NULL) { + cb(sock->statichandle, ISC_R_TIMEDOUT, NULL, cbarg); } } @@ -619,8 +633,11 @@ isc__nm_tcp_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) { REQUIRE(VALID_NMSOCK(handle->sock)); sock = handle->sock; - sock->rcb.recv = cb; - sock->rcbarg = cbarg; + + LOCK(&sock->lock); + sock->recv_cb = cb; + sock->recv_cbarg = cbarg; + UNLOCK(&sock->lock); ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpstartread); ievent->sock = sock; @@ -729,9 +746,12 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) { isc__netievent_startread_t *ievent = NULL; REQUIRE(VALID_NMSOCK(sock)); - if (sock->rcb.recv == NULL) { + LOCK(&sock->lock); + if (sock->recv_cb == NULL) { + UNLOCK(&sock->lock); return (ISC_R_CANCELED); } + UNLOCK(&sock->lock); if (!atomic_load(&sock->readpaused)) { return (ISC_R_SUCCESS); @@ -757,17 +777,23 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) { 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(buf != NULL); + LOCK(&sock->lock); + cb = sock->recv_cb; + cbarg = sock->recv_cbarg; + UNLOCK(&sock->lock); + if (nread >= 0) { isc_region_t region = { .base = (unsigned char *)buf->base, .length = nread }; - if (sock->rcb.recv != NULL) { - sock->rcb.recv(sock->statichandle, ISC_R_SUCCESS, - ®ion, sock->rcbarg); + if (cb != NULL) { + cb(sock->statichandle, ISC_R_SUCCESS, ®ion, cbarg); } sock->read_timeout = (atomic_load(&sock->keepalive) @@ -790,11 +816,10 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) { * This might happen if the inner socket is closing. It means that * it's detached, so the socket will be closed. */ - if (sock->rcb.recv != NULL) { + if (cb != NULL) { isc__nm_incstats(sock->mgr, sock->statsindex[STATID_RECVFAIL]); - sock->rcb.recv(sock->statichandle, ISC_R_EOF, NULL, - sock->rcbarg); isc__nmsocket_clearcb(sock); + cb(sock->statichandle, ISC_R_EOF, NULL, cbarg); } /* @@ -1122,12 +1147,19 @@ void isc__nm_tcp_shutdown(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); - if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL && - sock->rcb.recv != NULL) - { - sock->rcb.recv(sock->statichandle, ISC_R_CANCELED, NULL, - sock->rcbarg); + if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) { + isc_nm_recv_cb_t cb; + void *cbarg; + + LOCK(&sock->lock); + cb = sock->recv_cb; + cbarg = sock->recv_cbarg; isc__nmsocket_clearcb(sock); + UNLOCK(&sock->lock); + + if (cb != NULL) { + cb(sock->statichandle, ISC_R_CANCELED, NULL, cbarg); + } } } @@ -1142,8 +1174,16 @@ isc__nm_tcp_cancelread(isc_nmhandle_t *handle) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_tcpsocket); - if (atomic_load(&sock->client) && sock->rcb.recv != NULL) { - sock->rcb.recv(handle, ISC_R_EOF, NULL, sock->rcbarg); + if (atomic_load(&sock->client)) { + isc_nm_recv_cb_t cb; + void *cbarg; + + LOCK(&sock->lock); + cb = sock->recv_cb; + cbarg = sock->recv_cbarg; isc__nmsocket_clearcb(sock); + UNLOCK(&sock->lock); + + cb(handle, ISC_R_EOF, NULL, cbarg); } } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 84bf28d206..e086ed5e79 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -109,6 +109,8 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmsocket_t *dnslistensock = (isc_nmsocket_t *)cbarg; isc_nmsocket_t *dnssock = NULL; isc_nmhandle_t *readhandle = NULL; + isc_nm_accept_cb_t accept_cb; + void *accept_cbarg; REQUIRE(VALID_NMSOCK(dnslistensock)); REQUIRE(dnslistensock->type == isc_nm_tcpdnslistener); @@ -117,9 +119,13 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { return (result); } - if (dnslistensock->accept_cb.accept != NULL) { - result = dnslistensock->accept_cb.accept( - handle, ISC_R_SUCCESS, dnslistensock->accept_cbarg); + LOCK(&dnslistensock->lock); + accept_cb = dnslistensock->accept_cb; + accept_cbarg = dnslistensock->accept_cbarg; + UNLOCK(&dnslistensock->lock); + + if (accept_cb != NULL) { + result = accept_cb(handle, ISC_R_SUCCESS, accept_cbarg); if (result != ISC_R_SUCCESS) { return (result); } @@ -196,6 +202,8 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) { if (len <= dnssock->buf_len - 2) { isc_nmhandle_t *dnshandle = NULL; isc_nmsocket_t *listener = NULL; + isc_nm_recv_cb_t cb = NULL; + void *cbarg = NULL; if (atomic_load(&dnssock->client) && dnssock->statichandle != NULL) { @@ -205,22 +213,25 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) { } listener = dnssock->listener; - if (listener != NULL && listener->rcb.recv != NULL) { - listener->rcb.recv( - dnshandle, ISC_R_SUCCESS, - &(isc_region_t){ .base = dnssock->buf + 2, - .length = len }, - listener->rcbarg); - } else if (dnssock->rcb.recv != NULL) { - isc_nm_recv_cb_t cb = dnssock->rcb.recv; - void *cbarg = dnssock->rcbarg; - + if (listener != NULL) { + LOCK(&listener->lock); + cb = listener->recv_cb; + cbarg = listener->recv_cbarg; + UNLOCK(&listener->lock); + } else if (dnssock->recv_cb != NULL) { + LOCK(&dnssock->lock); + cb = dnssock->recv_cb; + cbarg = dnssock->recv_cbarg; /* * We need to clear the read callback *before* * calling it, because it might make another * call to isc_nm_read() and set up a new callback. */ isc__nmsocket_clearcb(dnssock); + UNLOCK(&dnssock->lock); + } + + if (cb != NULL) { cb(dnshandle, ISC_R_SUCCESS, &(isc_region_t){ .base = dnssock->buf + 2, .length = len }, @@ -303,8 +314,10 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, uv_timer_stop(&dnssock->timer); } + LOCK(&dnssock->lock); if (atomic_load(&dnssock->sequential) || - dnssock->rcb.recv == NULL) { + dnssock->recv_cb == NULL) { + UNLOCK(&dnssock->lock); /* * There are two reasons we might want to pause here: * - We're in sequential mode and we've received @@ -315,6 +328,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, isc_nm_pauseread(dnssock->outerhandle); done = true; } else { + UNLOCK(&dnssock->lock); /* * We're pipelining, so we now resume processing * packets until the clients-per-connection limit @@ -350,10 +364,12 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, REQUIRE(VALID_NM(mgr)); isc__nmsocket_init(dnslistensock, mgr, isc_nm_tcpdnslistener, iface); - dnslistensock->rcb.recv = cb; - dnslistensock->rcbarg = cbarg; - dnslistensock->accept_cb.accept = accept_cb; + LOCK(&dnslistensock->lock); + dnslistensock->recv_cb = cb; + dnslistensock->recv_cbarg = cbarg; + dnslistensock->accept_cb = accept_cb; dnslistensock->accept_cbarg = accept_cbarg; + UNLOCK(&dnslistensock->lock); dnslistensock->extrahandlesize = extrahandlesize; /* diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index d167b91727..e820b24405 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -59,9 +59,9 @@ isc_nm_listenudp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, mgr->nworkers * sizeof(*nsock)); memset(nsock->children, 0, mgr->nworkers * sizeof(*nsock)); - INSIST(nsock->rcb.recv == NULL && nsock->rcbarg == NULL); - nsock->rcb.recv = cb; - nsock->rcbarg = cbarg; + INSIST(nsock->recv_cb == NULL && nsock->recv_cbarg == NULL); + nsock->recv_cb = cb; + nsock->recv_cbarg = cbarg; nsock->extrahandlesize = extrahandlesize; for (size_t i = 0; i < mgr->nworkers; i++) { @@ -76,9 +76,9 @@ isc_nm_listenudp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, csock->tid = i; csock->extrahandlesize = extrahandlesize; - INSIST(csock->rcb.recv == NULL && csock->rcbarg == NULL); - csock->rcb.recv = cb; - csock->rcbarg = cbarg; + INSIST(csock->recv_cb == NULL && csock->recv_cbarg == NULL); + csock->recv_cb = cb; + csock->recv_cbarg = cbarg; csock->fd = socket(family, SOCK_DGRAM, 0); RUNTIME_CHECK(csock->fd >= 0); @@ -360,6 +360,8 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, isc_region_t region; uint32_t maxudp; bool free_buf = true; + isc_nm_recv_cb_t cb; + void *cbarg; /* * Even though destruction of the socket can only happen from the @@ -399,8 +401,18 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, region.base = (unsigned char *)buf->base; region.length = nrecv; - INSIST(sock->rcb.recv != NULL); - sock->rcb.recv(nmhandle, ISC_R_SUCCESS, ®ion, sock->rcbarg); + /* + * In tcp.c and tcpdns.c, this would need to be locked + * by sock->lock because callbacks may be set to NULL + * unexpectedly when the connection drops, but that isn't + * a factor in the UDP case. + */ + INSIST(sock->recv_cb != NULL); + cb = sock->recv_cb; + cbarg = sock->recv_cbarg; + + cb(nmhandle, ISC_R_SUCCESS, ®ion, cbarg); + if (free_buf) { isc__nm_free_uvbuf(sock, buf); } From cc7ceace7dcaade77e7197e9ca2cd916edb7e51a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 16 Jul 2020 15:47:58 -0700 Subject: [PATCH 5/8] add more logging to the shutdown system test the test server running in shutdown/resolver was not logging any debug info, which made it difficult to diagnose test failures. --- bin/tests/system/shutdown/clean.sh | 5 ++--- bin/tests/system/shutdown/resolver/named.conf.in | 8 ++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/bin/tests/system/shutdown/clean.sh b/bin/tests/system/shutdown/clean.sh index f977c772e8..0bb926ec77 100644 --- a/bin/tests/system/shutdown/clean.sh +++ b/bin/tests/system/shutdown/clean.sh @@ -8,11 +8,10 @@ # information regarding copyright ownership. rm -f ns*/*.jnl -rm -f ns*/named.conf rm -f ns*/named.lock rm -f ns*/named.memstats -rm -f ns*/named.run rm -f ns*/rpz*.txt -rm -f resolver/named.conf +rm -f */named.conf +rm -f */named.run rm -rf __pycache__ rm -f *.status diff --git a/bin/tests/system/shutdown/resolver/named.conf.in b/bin/tests/system/shutdown/resolver/named.conf.in index 8ad14f408b..d03539003c 100644 --- a/bin/tests/system/shutdown/resolver/named.conf.in +++ b/bin/tests/system/shutdown/resolver/named.conf.in @@ -1,3 +1,11 @@ +logging { + channel basic { + file "named.run"; + severity debug 999; + print-time yes; + }; + category default { basic; }; +}; key rndc_key { secret "1234abcd8765"; algorithm hmac-sha256; From 2f2d60a9898faeeb8e9c8d7b80061d7e7845687c Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 15 Jul 2020 17:57:58 -0700 Subject: [PATCH 6/8] limit the time we wait for netmgr to be destroyed if more than 10 seconds pass while we wait for netmgr events to finish running on shutdown, something is almost certainly wrong and we should assert and crash. --- lib/isc/netmgr/netmgr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 4c6fdde1b2..ea5a54d0cd 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -393,6 +393,7 @@ isc_nm_closedown(isc_nm_t *mgr) { void isc_nm_destroy(isc_nm_t **mgr0) { isc_nm_t *mgr = NULL; + int counter = 0; REQUIRE(mgr0 != NULL); REQUIRE(VALID_NM(*mgr0)); @@ -407,7 +408,7 @@ isc_nm_destroy(isc_nm_t **mgr0) { /* * Wait for the manager to be dereferenced elsewhere. */ - while (isc_refcount_current(&mgr->references) > 1) { + while (isc_refcount_current(&mgr->references) > 1 && counter++ < 1000) { /* * Sometimes libuv gets stuck, pausing and unpausing * netmgr goes over all events in async queue for all @@ -423,6 +424,8 @@ isc_nm_destroy(isc_nm_t **mgr0) { #endif /* ifdef WIN32 */ } + INSIST(counter <= 1000); + /* * Detach final reference. */ From 00e04a86c8b6828a066573031cc539adab565061 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Wed, 2 Sep 2020 17:57:44 +0200 Subject: [PATCH 7/8] tracing of active sockets and handles If NETMGR_TRACE is defined, we now maintain a list of active sockets in the netmgr object and a list of active handles in each socket object; by walking the list and printing `backtrace` in a debugger we can see where they were created, to assist in in debugging of reference counting errors. On shutdown, if netmgr finds there are still active sockets after waiting, isc__nm_dump_active() will be called to log the list of active sockets and their underlying handles, along with some details about them. --- lib/isc/netmgr/netmgr-int.h | 29 +++++++++ lib/isc/netmgr/netmgr.c | 120 +++++++++++++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 2 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 633e1a0e96..2b93d2169c 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -49,6 +49,20 @@ #define ISC_NETMGR_RECVBUF_SIZE (65536) #endif +/* + * Define NETMGR_TRACE to activate tracing of handles and sockets. + * This will impair performance but enables us to quickly determine, + * if netmgr resources haven't been cleaned up on shutdown, which ones + * are still in use. + */ +#ifdef NETMGR_TRACE +#define TRACE_SIZE 8 + +void +isc__nm_dump_active(isc_nm_t *nm); + +#endif + /* * Single network event loop worker. */ @@ -104,6 +118,11 @@ struct isc_nmhandle { isc_sockaddr_t local; isc_nm_opaquecb_t doreset; /* reset extra callback, external */ isc_nm_opaquecb_t dofree; /* free extra callback, external */ +#ifdef NETMGR_TRACE + void *backtrace[TRACE_SIZE]; + int backtrace_size; + LINK(isc_nmhandle_t) active_link; +#endif void *opaque; char extra[]; }; @@ -308,6 +327,10 @@ struct isc_nm { uint32_t idle; uint32_t keepalive; uint32_t advertised; + +#ifdef NETMGR_TRACE + ISC_LIST(isc_nmsocket_t) active_sockets; +#endif }; typedef enum isc_nmsocket_type { @@ -524,6 +547,12 @@ struct isc_nmsocket { isc_nm_accept_cb_t accept_cb; void *accept_cbarg; +#ifdef NETMGR_TRACE + void *backtrace[TRACE_SIZE]; + int backtrace_size; + LINK(isc_nmsocket_t) active_link; + ISC_LIST(isc_nmhandle_t) active_handles; +#endif }; bool diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index ea5a54d0cd..febbb3c414 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -33,6 +33,10 @@ #include "netmgr-int.h" #include "uv-compat.h" +#ifdef NETMGR_TRACE +#include +#endif + /*% * How many isc_nmhandles and isc_nm_uvreqs will we be * caching for reuse in a socket. @@ -157,6 +161,10 @@ isc_nm_start(isc_mem_t *mctx, uint32_t workers) { atomic_init(&mgr->paused, false); atomic_init(&mgr->interlocked, false); +#ifdef NETMGR_TRACE + ISC_LIST_INIT(mgr->active_sockets); +#endif + /* * Default TCP timeout values. * May be updated by isc_nm_tcptimeouts(). @@ -423,7 +431,12 @@ isc_nm_destroy(isc_nm_t **mgr0) { usleep(10000); #endif /* ifdef WIN32 */ } - +#ifdef NETMGR_TRACE + if (!ISC_LIST_EMPTY(mgr->active_sockets)) { + isc__nm_dump_active(mgr); + INSIST(ISC_LIST_EMPTY(mgr->active_sockets)); + } +#endif INSIST(counter <= 1000); /* @@ -792,7 +805,11 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree) { isc_mem_free(sock->mgr->mctx, sock->ah_handles); isc_mutex_destroy(&sock->lock); isc_condition_destroy(&sock->cond); - +#ifdef NETMGR_TRACE + LOCK(&sock->mgr->lock); + ISC_LIST_UNLINK(sock->mgr->active_sockets, sock, active_link); + UNLOCK(&sock->mgr->lock); +#endif if (dofree) { isc_nm_t *mgr = sock->mgr; isc_mem_put(mgr->mctx, sock, sizeof(*sock)); @@ -950,6 +967,15 @@ isc__nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type, .inactivereqs = isc_astack_new( mgr->mctx, ISC_NM_REQS_STACK_SIZE) }; +#ifdef NETMGR_TRACE + sock->backtrace_size = backtrace(sock->backtrace, TRACE_SIZE); + ISC_LINK_INIT(sock, active_link); + ISC_LIST_INIT(sock->active_handles); + LOCK(&mgr->lock); + ISC_LIST_APPEND(mgr->active_sockets, sock, active_link); + UNLOCK(&mgr->lock); +#endif + isc_nm_attach(mgr, &sock->mgr); sock->uv_handle.handle.data = sock; @@ -1038,6 +1064,9 @@ alloc_handle(isc_nmsocket_t *sock) { sizeof(isc_nmhandle_t) + sock->extrahandlesize); *handle = (isc_nmhandle_t){ .magic = NMHANDLE_MAGIC }; +#ifdef NETMGR_TRACE + ISC_LINK_INIT(handle, active_link); +#endif isc_refcount_init(&handle->references, 1); return (handle); @@ -1063,6 +1092,10 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, isc__nmsocket_attach(sock, &handle->sock); +#ifdef NETMGR_TRACE + handle->backtrace_size = backtrace(handle->backtrace, TRACE_SIZE); +#endif + if (peer != NULL) { memcpy(&handle->peer, peer, sizeof(isc_sockaddr_t)); } else { @@ -1103,6 +1136,9 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, INSIST(sock->ah_handles[pos] == NULL); sock->ah_handles[pos] = handle; handle->ah_pos = pos; +#ifdef NETMGR_TRACE + ISC_LIST_APPEND(sock->active_handles, handle, active_link); +#endif UNLOCK(&sock->lock); if (sock->type == isc_nm_tcpsocket || @@ -1170,6 +1206,10 @@ nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) { INSIST(sock->ah_size > handle->ah_pos); INSIST(atomic_load(&sock->ah) > 0); +#ifdef NETMGR_TRACE + ISC_LIST_UNLINK(sock->active_handles, handle, active_link); +#endif + sock->ah_handles[handle->ah_pos] = NULL; handlenum = atomic_fetch_sub(&sock->ah, 1) - 1; sock->ah_frees[handlenum] = handle->ah_pos; @@ -1581,3 +1621,79 @@ isc__nm_socket_freebind(const uv_handle_t *handle) { #endif return (result); } + +#ifdef NETMGR_TRACE +/* + * Dump all active sockets in netmgr. We output to stderr + * as the logger might be already shut down. + */ + +static const char * +nmsocket_type_totext(isc_nmsocket_type type) { + switch (type) { + case isc_nm_udpsocket: + return ("isc_nm_udpsocket"); + case isc_nm_udplistener: + return ("isc_nm_udplistener"); + case isc_nm_tcpsocket: + return ("isc_nm_tcpsocket"); + case isc_nm_tcplistener: + return ("isc_nm_tcplistener"); + case isc_nm_tcpdnslistener: + return ("isc_nm_tcpdnslistener"); + case isc_nm_tcpdnssocket: + return ("isc_nm_tcpdnssocket"); + default: + INSIST(0); + ISC_UNREACHABLE(); + } +} + +static void +nmhandle_dump(isc_nmhandle_t *handle) { + fprintf(stderr, "Active handle %p, refs %lu\n", handle, + isc_refcount_current(&handle->references)); + fprintf(stderr, "Created by:\n"); + backtrace_symbols_fd(handle->backtrace, handle->backtrace_size, + STDERR_FILENO); + fprintf(stderr, "\n\n"); +} + +static void +nmsocket_dump(isc_nmsocket_t *sock) { + isc_nmhandle_t *handle; + LOCK(&sock->lock); + fprintf(stderr, "\n=================\n"); + fprintf(stderr, "Active socket %p, type %s, refs %lu\n", sock, + nmsocket_type_totext(sock->type), + isc_refcount_current(&sock->references)); + fprintf(stderr, "Parent %p, listener %p\n", sock->parent, + sock->listener); + fprintf(stderr, "Created by:\n"); + 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)) + { + nmhandle_dump(handle); + } + fprintf(stderr, "\n"); + UNLOCK(&sock->lock); +} + +void +isc__nm_dump_active(isc_nm_t *nm) { + isc_nmsocket_t *sock; + 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)) + { + nmsocket_dump(sock); + } + UNLOCK(&nm->lock); +} +#endif From e460e321bd9d1789bdd8b1e79272e7001337a97c Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 3 Sep 2020 15:44:33 -0700 Subject: [PATCH 8/8] CHANGES --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index a803caf456..ba74a88f92 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5503. [bug] Cleaned up reference counting of network manager + handles, now using isc_nmhandle_attach() and _detach() + instead of _ref() and _unref(). [GL #2122] + 5502. [func] 'dig +bufsize=0' no longer disables EDNS. [GL #2054] 5501. [func] Log CDS/CDNSKEY publication. [GL #1748]