From 5ea26ee1f11e2e3d294e9e92bc4810f7c525727a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 4 Jun 2020 23:13:54 -0700 Subject: [PATCH] modify reference counting within netmgr - isc__nmhandle_get() now attaches to the sock in the nmhandle object. the caller is responsible for dereferencing the original socket pointer when necessary. - tcpdns listener sockets attach sock->outer to the outer tcp listener socket. tcpdns connected sockets attach sock->outerhandle to the handle for the tcp connected socket. - only listener sockets need to be attached/detached directly. connected sockets should only be accessed and reference-counted via their associated handles. --- lib/isc/netmgr/netmgr-int.h | 4 +++ lib/isc/netmgr/netmgr.c | 20 +++++++---- lib/isc/netmgr/tcp.c | 41 +++++++++++++++++----- lib/isc/netmgr/tcpdns.c | 68 +++++++++++++++++++++++-------------- lib/isc/netmgr/udp.c | 16 +++++++-- 5 files changed, 106 insertions(+), 43 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 981f4aa635..d066198bd0 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -405,6 +405,7 @@ struct isc_nmsocket { int nchildren; isc_nmiface_t *iface; isc_nmhandle_t *tcphandle; + isc_nmhandle_t *outerhandle; /*% Extra data allocated at the end of each isc_nmhandle_t */ size_t extrahandlesize; @@ -589,6 +590,9 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, * * If 'local' is not NULL, set the handle's local address to 'local', * otherwise set it to 'sock->iface->addr'. + * + * 'sock' will be attached to 'handle->sock'. The caller may need + * to detach the socket afterward. */ isc__nm_uvreq_t * diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index dd58978892..1a4a82c831 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -736,9 +736,15 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree) { isc__nm_decstats(sock->mgr, sock->statsindex[STATID_ACTIVE]); } - if (sock->tcphandle != NULL) { - isc_nmhandle_unref(sock->tcphandle); - sock->tcphandle = NULL; + sock->tcphandle = NULL; + + if (sock->outerhandle != NULL) { + isc_nmhandle_unref(sock->outerhandle); + sock->outerhandle = NULL; + } + + if (sock->outer != NULL) { + isc__nmsocket_detach(&sock->outer); } while ((handle = isc_astack_pop(sock->inactivehandles)) != NULL) { @@ -1050,7 +1056,8 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, isc_refcount_increment0(&handle->references); } - handle->sock = sock; + isc__nmsocket_attach(sock, &handle->sock); + if (peer != NULL) { memcpy(&handle->peer, peer, sizeof(isc_sockaddr_t)); } else { @@ -1160,7 +1167,7 @@ nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) { void isc_nmhandle_unref(isc_nmhandle_t *handle) { - isc_nmsocket_t *sock = NULL, *tmp = NULL; + isc_nmsocket_t *sock = NULL; REQUIRE(VALID_NMHANDLE(handle)); @@ -1182,7 +1189,6 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) { * be deleted by another thread while we're deactivating the * handle. */ - isc__nmsocket_attach(sock, &tmp); nmhandle_deactivate(sock, handle); /* @@ -1206,7 +1212,7 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) { } } - isc__nmsocket_detach(&tmp); + isc__nmsocket_detach(&sock); } void * diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 5a6ac45735..d879fceeaf 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -128,14 +128,15 @@ static void tcp_connect_cb(uv_connect_t *uvreq, int status) { isc__nm_uvreq_t *req = (isc__nm_uvreq_t *)uvreq->data; isc_nmsocket_t *sock = NULL; + sock = uv_handle_get_data((uv_handle_t *)uvreq->handle); REQUIRE(VALID_UVREQ(req)); if (status == 0) { isc_result_t result; - isc_nmhandle_t *handle = NULL; struct sockaddr_storage ss; + isc_nmhandle_t *handle = NULL; isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]); uv_tcp_getpeername(&sock->uv_handle.tcp, (struct sockaddr *)&ss, @@ -146,6 +147,19 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) { 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: @@ -154,9 +168,8 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) { isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECTFAIL]); req->cb.connect(NULL, isc__nm_uverr2result(status), req->cbarg); + isc__nm_uvreq_put(&req, sock); } - - isc__nm_uvreq_put(&req, sock); } isc_result_t @@ -170,8 +183,8 @@ isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_cb_t cb, nsock = isc_mem_get(mgr->mctx, sizeof(*nsock)); isc__nmsocket_init(nsock, mgr, isc_nm_tcplistener, iface); - nsock->rcb.accept = cb; - nsock->rcbarg = cbarg; + nsock->accept_cb.accept = cb; + nsock->accept_cbarg = cbarg; nsock->extrahandlesize = extrahandlesize; nsock->backlog = backlog; nsock->result = ISC_R_SUCCESS; @@ -383,10 +396,20 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) { handle = isc__nmhandle_get(csock, NULL, &local); - INSIST(ssock->rcb.accept != NULL); + INSIST(ssock->accept_cb.accept != NULL); csock->read_timeout = ssock->mgr->init; - ssock->rcb.accept(handle, ISC_R_SUCCESS, ssock->rcbarg); + ssock->accept_cb.accept(handle, ISC_R_SUCCESS, ssock->accept_cbarg); + + /* + * csock is now attached to the handle. + */ isc__nmsocket_detach(&csock); + + /* + * If the accept callback wants to hold on to the handle, + * it needs to attach to it. + */ + isc_nmhandle_unref(handle); return; error: @@ -915,7 +938,9 @@ timer_close_cb(uv_handle_t *uvhandle) { REQUIRE(VALID_NMSOCK(sock)); - isc__nmsocket_detach(&sock->server); + if (sock->server != NULL) { + isc__nmsocket_detach(&sock->server); + } uv_close(&sock->uv_handle.handle, tcp_close_cb); } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 0f7819fc23..6ae5c111cf 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -82,7 +82,6 @@ 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)); - isc__nmsocket_detach(&sock); } static void @@ -123,7 +122,10 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { dnssock->extrahandlesize = dnslistensock->extrahandlesize; isc__nmsocket_attach(dnslistensock, &dnssock->listener); - isc__nmsocket_attach(handle->sock, &dnssock->outer); + + dnssock->outerhandle = handle; + isc_nmhandle_ref(dnssock->outerhandle); + dnssock->peer = handle->sock->peer; dnssock->read_timeout = handle->sock->mgr->init; dnssock->tid = isc_nm_tid(); @@ -136,7 +138,11 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { uv_timer_start(&dnssock->timer, dnstcp_readtimeout, dnssock->read_timeout, 0); - isc_nm_read(handle, dnslisten_readcb, dnssock); + isc_nmhandle_ref(handle); + result = isc_nm_read(handle, dnslisten_readcb, dnssock); + if (result != ISC_R_SUCCESS) { + isc_nmhandle_unref(handle); + } } /* @@ -188,6 +194,11 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) { dnssock->buf_len); } + /* + * dnssock is now attached to dnshandle + */ + isc__nmsocket_detach(&dnssock); + *handlep = dnshandle; return (ISC_R_SUCCESS); } @@ -244,7 +255,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) { /* * We have a packet: stop timeout timers */ - atomic_store(&dnssock->outer->processing, true); + atomic_store(&dnssock->outerhandle->sock->processing, true); if (dnssock->timer_initialized) { uv_timer_stop(&dnssock->timer); } @@ -255,7 +266,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) { * one packet, so we're done until the next read * completes. */ - isc_nm_pauseread(dnssock->outer); + isc_nm_pauseread(dnssock->outerhandle->sock); done = true; } else { /* @@ -267,7 +278,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) { */ if (atomic_load(&dnssock->ah) >= TCPDNS_CLIENTS_PER_CONN) { - isc_nm_pauseread(dnssock->outer); + isc_nm_pauseread(dnssock->outerhandle->sock); done = true; } } @@ -326,7 +337,7 @@ isc__nm_tcpdns_stoplistening(isc_nmsocket_t *sock) { sock->rcbarg = NULL; if (sock->outer != NULL) { - isc_nm_stoplistening(sock->outer); + isc__nm_tcp_stoplistening(sock->outer); isc__nmsocket_detach(&sock->outer); } } @@ -336,7 +347,8 @@ isc_nm_tcpdns_sequential(isc_nmhandle_t *handle) { REQUIRE(VALID_NMHANDLE(handle)); if (handle->sock->type != isc_nm_tcpdnssocket || - handle->sock->outer == NULL) { + handle->sock->outerhandle == NULL) + { return; } @@ -348,7 +360,7 @@ isc_nm_tcpdns_sequential(isc_nmhandle_t *handle) { * closehandle_cb callback, called whenever a handle * is released. */ - isc_nm_pauseread(handle->sock->outer); + isc_nm_pauseread(handle->sock->outerhandle->sock); atomic_store(&handle->sock->sequential, true); } @@ -357,12 +369,13 @@ isc_nm_tcpdns_keepalive(isc_nmhandle_t *handle) { REQUIRE(VALID_NMHANDLE(handle)); if (handle->sock->type != isc_nm_tcpdnssocket || - handle->sock->outer == NULL) { + handle->sock->outerhandle == NULL) + { return; } atomic_store(&handle->sock->keepalive, true); - atomic_store(&handle->sock->outer->keepalive, true); + atomic_store(&handle->sock->outerhandle->sock->keepalive, true); } typedef struct tcpsend { @@ -382,13 +395,13 @@ resume_processing(void *arg) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - if (sock->type != isc_nm_tcpdnssocket || sock->outer == NULL) { + if (sock->type != isc_nm_tcpdnssocket || sock->outerhandle == NULL) { return; } if (atomic_load(&sock->ah) == 0) { /* Nothing is active; sockets can timeout now */ - atomic_store(&sock->outer->processing, false); + atomic_store(&sock->outerhandle->sock->processing, false); if (sock->timer_initialized) { uv_timer_start(&sock->timer, dnstcp_readtimeout, sock->read_timeout, 0); @@ -404,13 +417,14 @@ resume_processing(void *arg) { result = processbuffer(sock, &handle); if (result == ISC_R_SUCCESS) { - atomic_store(&sock->outer->processing, true); + atomic_store(&sock->outerhandle->sock->processing, + true); if (sock->timer_initialized) { uv_timer_stop(&sock->timer); } isc_nmhandle_unref(handle); - } else if (sock->outer != NULL) { - isc_nm_resumeread(sock->outer); + } else if (sock->outerhandle != NULL) { + isc_nm_resumeread(sock->outerhandle->sock); } return; @@ -428,8 +442,8 @@ resume_processing(void *arg) { /* * Nothing in the buffer; resume reading. */ - if (sock->outer != NULL) { - isc_nm_resumeread(sock->outer); + if (sock->outerhandle != NULL) { + isc_nm_resumeread(sock->outerhandle->sock); } break; @@ -438,7 +452,7 @@ resume_processing(void *arg) { if (sock->timer_initialized) { uv_timer_stop(&sock->timer); } - atomic_store(&sock->outer->processing, true); + atomic_store(&sock->outerhandle->sock->processing, true); isc_nmhandle_unref(dnshandle); } while (atomic_load(&sock->ah) < TCPDNS_CLIENTS_PER_CONN); } @@ -447,13 +461,13 @@ static void tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { tcpsend_t *ts = (tcpsend_t *)cbarg; - UNUSED(handle); - ts->cb(ts->orighandle, result, ts->cbarg); isc_mem_put(ts->mctx, ts->region.base, ts->region.length); isc_nmhandle_unref(ts->orighandle); isc_mem_putanddetach(&ts->mctx, ts, sizeof(*ts)); + + isc_nmhandle_unref(handle); } /* @@ -471,7 +485,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region, REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_tcpdnssocket); - if (sock->outer == NULL) { + if (sock->outerhandle == NULL) { /* The socket is closed */ return (ISC_R_NOTCONNECTED); } @@ -480,7 +494,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region, *t = (tcpsend_t){ .cb = cb, .cbarg = cbarg, - .handle = handle->sock->outer->tcphandle, + .handle = handle->sock->outerhandle, }; isc_mem_attach(sock->mgr->mctx, &t->mctx); @@ -515,14 +529,16 @@ tcpdns_close_direct(isc_nmsocket_t *sock) { * At this point we're certain that there are no external * references, we can close everything. */ - if (sock->outer != NULL) { - sock->outer->rcb.recv = NULL; - isc__nmsocket_detach(&sock->outer); + if (sock->outerhandle != NULL) { + sock->outerhandle->sock->rcb.recv = NULL; + isc_nmhandle_unref(sock->outerhandle); + sock->outerhandle = NULL; } if (sock->listener != NULL) { isc__nmsocket_detach(&sock->listener); } atomic_store(&sock->closed, true); + isc__nmsocket_prep_destroy(sock); } } diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index bd40fa3811..d37c1c4765 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -317,12 +317,17 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, isc_result_t result; isc_nmhandle_t *nmhandle = NULL; isc_sockaddr_t sockaddr; - isc_nmsocket_t *sock = uv_handle_get_data((uv_handle_t *)handle); + isc_nmsocket_t *sock = NULL; isc_region_t region; uint32_t maxudp; bool free_buf = true; - REQUIRE(VALID_NMSOCK(sock)); + /* + * Even though destruction of the socket can only happen from the + * network thread that we're in, we still attach to the socket here + * to ensure it won't be destroyed by the recv callback. + */ + isc__nmsocket_attach(uv_handle_get_data((uv_handle_t *)handle), &sock); #ifdef UV_UDP_MMSG_CHUNK free_buf = ((flags & UV_UDP_MMSG_CHUNK) == 0); @@ -338,6 +343,7 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, if (free_buf) { isc__nm_free_uvbuf(sock, buf); } + isc__nmsocket_detach(&sock); return; } @@ -347,6 +353,7 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, */ maxudp = atomic_load(&sock->mgr->maxudp); if (maxudp != 0 && (uint32_t)nrecv > maxudp) { + isc__nmsocket_detach(&sock); return; } @@ -362,6 +369,11 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf, isc__nm_free_uvbuf(sock, buf); } + /* + * The sock is now attached to the handle, we can detach our ref. + */ + isc__nmsocket_detach(&sock); + /* * If the recv callback wants to hold on to the handle, * it needs to attach to it.