From 3704c4fff2757ade6dda56865aa87935d0c447b9 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sat, 20 Jun 2020 15:03:05 -0700 Subject: [PATCH] clean up outerhandle when a tcpdns socket is disconnected this prevents a crash when some non-netmgr thread, such as a recursive lookup, times out after the TCP socket is already disconnected. --- lib/isc/netmgr/netmgr-int.h | 14 ++++++-------- lib/isc/netmgr/netmgr.c | 10 ++++++++++ lib/isc/netmgr/tcp.c | 8 +++++++- lib/isc/netmgr/tcpdns.c | 22 +++++++++++++++------- 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index b0b4833e4b..71b2d20c2c 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -98,14 +98,6 @@ struct isc_nmhandle { isc_nmsocket_t *sock; size_t ah_pos; /* Position in the socket's 'active handles' array */ - /* - * The handle is 'inflight' if netmgr is not currently processing - * it in any way - it might mean that e.g. a recursive resolution - * is happening. For an inflight handle we must wait for the - * calling code to finish before we can free it. - */ - atomic_bool inflight; - isc_sockaddr_t peer; isc_sockaddr_t local; isc_nm_opaquecb_t doreset; /* reset extra callback, external */ @@ -650,6 +642,12 @@ isc__nmsocket_active(isc_nmsocket_t *sock); * or, for child sockets, 'sock->parent->active'. */ +void +isc__nmsocket_clearcb(isc_nmsocket_t *sock); +/*%< + * Clear the recv and accept callbacks in 'sock'. + */ + void isc__nm_async_closecb(isc__networker_t *worker, isc__netievent_t *ev0); /*%< diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 7fd029178c..dafe203601 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -987,6 +987,16 @@ isc__nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type, sock->magic = NMSOCK_MAGIC; } +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->accept_cbarg = NULL; +} + void isc__nm_alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf) { isc_nmsocket_t *sock = uv_handle_get_data(handle); diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index f38f31070b..e3e222dead 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -584,6 +584,7 @@ readtimeout_cb(uv_timer_t *handle) { if (sock->rcb.recv != NULL) { sock->rcb.recv(sock->tcphandle, ISC_R_TIMEDOUT, NULL, sock->rcbarg); + isc__nmsocket_clearcb(sock); } } @@ -682,7 +683,9 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) { isc__netievent_startread_t *ievent = NULL; REQUIRE(VALID_NMSOCK(sock)); - REQUIRE(sock->rcb.recv != NULL); + if (sock->rcb.recv == NULL) { + return (ISC_R_CANCELED); + } if (!atomic_load(&sock->readpaused)) { return (ISC_R_SUCCESS); @@ -744,6 +747,7 @@ 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); + isc__nmsocket_clearcb(sock); } /* @@ -1080,6 +1084,7 @@ isc__nm_tcp_shutdown(isc_nmsocket_t *sock) { { sock->rcb.recv(sock->tcphandle, ISC_R_CANCELED, NULL, sock->rcbarg); + isc__nmsocket_clearcb(sock); } } @@ -1092,5 +1097,6 @@ isc__nm_tcp_cancelread(isc_nmsocket_t *sock) { { sock->rcb.recv(sock->tcphandle, ISC_R_CANCELED, NULL, sock->rcbarg); + isc__nmsocket_clearcb(sock); } } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 19a31870f2..a4d3f37dce 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -231,6 +231,11 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t 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; + } return; } @@ -340,8 +345,7 @@ isc__nm_tcpdns_stoplistening(isc_nmsocket_t *sock) { atomic_store(&sock->listening, false); atomic_store(&sock->closed, true); - sock->rcb.recv = NULL; - sock->rcbarg = NULL; + isc__nmsocket_clearcb(sock); if (sock->outer != NULL) { isc__nm_tcp_stoplistening(sock->outer); @@ -431,7 +435,11 @@ resume_processing(void *arg) { } isc_nmhandle_unref(handle); } else if (sock->outerhandle != NULL) { - isc_nm_resumeread(sock->outerhandle->sock); + result = isc_nm_resumeread(sock->outerhandle->sock); + if (result != ISC_R_SUCCESS) { + isc_nmhandle_unref(sock->outerhandle); + sock->outerhandle = NULL; + } } return; @@ -524,7 +532,9 @@ tcpdns_close_direct(isc_nmsocket_t *sock) { REQUIRE(sock->tid == isc_nm_tid()); /* We don't need atomics here, it's all in single network thread */ - if (sock->timer_initialized) { + if (sock->self != NULL) { + isc__nmsocket_detach(&sock->self); + } else if (sock->timer_initialized) { /* * We need to fire the timer callback to clean it up, * it will then call us again (via detach) so that we @@ -533,15 +543,13 @@ tcpdns_close_direct(isc_nmsocket_t *sock) { sock->timer_initialized = false; uv_timer_stop(&sock->timer); uv_close((uv_handle_t *)&sock->timer, timer_close_cb); - } else if (sock->self != NULL) { - isc__nmsocket_detach(&sock->self); } else { /* * At this point we're certain that there are no external * references, we can close everything. */ if (sock->outerhandle != NULL) { - sock->outerhandle->sock->rcb.recv = NULL; + isc__nmsocket_clearcb(sock->outerhandle->sock); isc_nmhandle_unref(sock->outerhandle); sock->outerhandle = NULL; }