From 3ab3d90de039a3f086bba9c25792e42dd1b4a614 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Sat, 31 Oct 2020 21:08:53 +0100 Subject: [PATCH 1/3] Fix improper closed connection handling in tcpdns. If dnslisten_readcb gets a read callback it needs to verify that the outer socket wasn't closed in the meantime, and issue a CANCELED callback if it was. --- lib/isc/netmgr/tcpdns.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 255c44fe2c..f5df92c105 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -259,6 +259,11 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, REQUIRE(VALID_NMSOCK(dnssock)); REQUIRE(dnssock->tid == isc_nm_tid()); REQUIRE(VALID_NMHANDLE(handle)); + if (eresult == ISC_R_SUCCESS && + (!isc__nmsocket_active(dnssock) || dnssock->outerhandle == NULL)) + { + eresult = ISC_R_CANCELED; + } if (region == NULL || eresult != ISC_R_SUCCESS) { /* Connection closed */ From cea4b4db8f32f98ffb48f2afc08ff3b19bfec602 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 2 Nov 2020 15:00:49 +0100 Subject: [PATCH 2/3] Add CHANGES note for [GL #2227] --- CHANGES | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 891151bf9e..d118b18c63 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +5526. [bug] Fix a race/NULL dereference in TCPDNS read. [GL #2227] + 5525. [placeholder] 5524. [func] Added functionality to the network manager to @@ -9,7 +11,7 @@ not found. Subsequent lookups would succeed. [GL #2236] -5522. [bug] Fix a race/NULL dereference in TCPDNS. [GL #2227] +5522. [bug] Fix a race/NULL dereference in TCPDNS send. [GL #2227] 5521. [func] All use of libltdl was dropped. libuv's shared library handling interface is now used instead. [GL !4278] From c14c1fdd2c43c96a6b31affc719cdf18f0057667 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 2 Nov 2020 15:55:12 +0100 Subject: [PATCH 3/3] Put up additional safe guards to not use inactive/closed tcpdns socket When we are operating on the tcpdns socket, we need to double check whether the socket or its outerhandle or its listener or its mgr is still active and when not, bail out early. --- lib/isc/netmgr/netmgr-int.h | 2 +- lib/isc/netmgr/tcp.c | 3 +- lib/isc/netmgr/tcpdns.c | 137 +++++++++++++++++++++++------------- 3 files changed, 89 insertions(+), 53 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 2583e92855..e99631d151 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -871,7 +871,7 @@ isc__nm_async_tcpdnsstop(isc__networker_t *worker, isc__netievent_t *ev0); void isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0); -isc_result_t +void isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg); void diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 04b1d171bc..ce25a9d920 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -1391,11 +1391,10 @@ isc__nm_async_tcpclose(isc__networker_t *worker, isc__netievent_t *ev0) { isc_nmsocket_t *sock = ievent->sock; REQUIRE(VALID_NMSOCK(sock)); + REQUIRE(sock->tid == isc_nm_tid()); UNUSED(worker); - REQUIRE(sock->tid == isc_nm_tid()); - tcp_close_direct(sock); } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index f5df92c105..d7d4dbb0d3 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -152,6 +152,7 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { dnssock->timer_initialized = true; uv_timer_start(&dnssock->timer, dnstcp_readtimeout, dnssock->read_timeout, 0); + dnssock->timer_running = true; /* * Add a reference to handle to keep it from being freed by @@ -259,26 +260,39 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, REQUIRE(VALID_NMSOCK(dnssock)); REQUIRE(dnssock->tid == isc_nm_tid()); REQUIRE(VALID_NMHANDLE(handle)); - if (eresult == ISC_R_SUCCESS && - (!isc__nmsocket_active(dnssock) || dnssock->outerhandle == NULL)) + + if (!isc__nmsocket_active(dnssock) || atomic_load(&dnssock->closing) || + dnssock->outerhandle == NULL || + (dnssock->listener != NULL && + !isc__nmsocket_active(dnssock->listener)) || + atomic_load(&dnssock->mgr->closing)) { - eresult = ISC_R_CANCELED; + if (eresult == ISC_R_SUCCESS) { + eresult = ISC_R_CANCELED; + } } if (region == NULL || eresult != ISC_R_SUCCESS) { + isc_nm_recv_cb_t cb = dnssock->recv_cb; + void *cbarg = dnssock->recv_cbarg; + /* Connection closed */ atomic_store(&dnssock->result, eresult); - if (atomic_load(&dnssock->client) && dnssock->recv_cb != NULL) { - dnssock->recv_cb(dnssock->statichandle, eresult, NULL, - dnssock->recv_cbarg); + isc__nmsocket_clearcb(dnssock); + if (atomic_load(&dnssock->client) && cb != NULL) { + cb(dnssock->statichandle, eresult, NULL, cbarg); } + if (dnssock->self != NULL) { isc__nmsocket_detach(&dnssock->self); } - isc__nmsocket_clearcb(dnssock); if (dnssock->outerhandle != NULL) { + isc__nmsocket_clearcb(dnssock->outerhandle->sock); isc_nmhandle_detach(&dnssock->outerhandle); } + if (dnssock->listener != NULL) { + isc__nmsocket_detach(&dnssock->listener); + } /* * Server connections will hold two handle references when @@ -487,6 +501,7 @@ resume_processing(void *arg) { if (sock->timer_initialized) { uv_timer_start(&sock->timer, dnstcp_readtimeout, sock->read_timeout, 0); + sock->timer_running = true; } } @@ -551,30 +566,46 @@ tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmhandle_detach(&handle); } +/* + * The socket is closing, outerhandle has been detached, listener is + * inactive, or the netmgr is closing: any operation on it should abort + * with ISC_R_CANCELED. + */ +static bool +inactive(isc_nmsocket_t *sock) { + return (!isc__nmsocket_active(sock) || atomic_load(&sock->closing) || + sock->outerhandle == NULL || + (sock->listener != NULL && + !isc__nmsocket_active(sock->listener)) || + atomic_load(&sock->mgr->closing)); +} + void isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) { isc__netievent_tcpdnssend_t *ievent = (isc__netievent_tcpdnssend_t *)ev0; isc__nm_uvreq_t *req = ievent->req; isc_nmsocket_t *sock = ievent->sock; + isc_nmhandle_t *sendhandle = NULL; + isc_region_t r; + REQUIRE(VALID_NMSOCK(sock)); + REQUIRE(VALID_UVREQ(req)); REQUIRE(worker->id == sock->tid); REQUIRE(sock->tid == isc_nm_tid()); + REQUIRE(sock->type == isc_nm_tcpdnssocket); - if (isc__nmsocket_active(sock) && sock->outerhandle != NULL) { - isc_nmhandle_t *sendhandle = NULL; - isc_region_t r; - - r.base = (unsigned char *)req->uvbuf.base; - r.length = req->uvbuf.len; - isc_nmhandle_attach(sock->outerhandle, &sendhandle); - isc_nm_send(sendhandle, &r, tcpdnssend_cb, req); - } else { + if (inactive(sock)) { req->cb.send(req->handle, ISC_R_CANCELED, req->cbarg); - isc_mem_put(req->sock->mgr->mctx, req->uvbuf.base, - req->uvbuf.len); + isc_mem_put(sock->mgr->mctx, req->uvbuf.base, req->uvbuf.len); isc__nm_uvreq_put(&req, req->handle->sock); + return; } + + r.base = (unsigned char *)req->uvbuf.base; + r.length = req->uvbuf.len; + isc_nmhandle_attach(sock->outerhandle, &sendhandle); + isc_nm_send(sendhandle, &r, tcpdnssend_cb, req); } /* @@ -592,7 +623,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region, REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_tcpdnssocket); - if (!isc__nmsocket_active(sock)) { + if (inactive(sock)) { cb(handle, ISC_R_CANCELED, cbarg); return; } @@ -607,38 +638,25 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region, *(uint16_t *)uvreq->uvbuf.base = htons(region->length); memmove(uvreq->uvbuf.base + 2, region->base, region->length); - if (sock->tid == isc_nm_tid()) { - isc_nmhandle_t *sendhandle = NULL; - isc_region_t r; + isc__netievent_tcpdnssend_t *ievent = NULL; - r.base = (unsigned char *)uvreq->uvbuf.base; - r.length = uvreq->uvbuf.len; - if (sock->outerhandle != NULL) { - isc_nmhandle_attach(sock->outerhandle, &sendhandle); - isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb, - uvreq); - } else { - cb(handle, ISC_R_CANCELED, cbarg); - isc_mem_put(sock->mgr->mctx, uvreq->uvbuf.base, - uvreq->uvbuf.len); - isc__nm_uvreq_put(&uvreq, sock); - } - } else { - isc__netievent_tcpdnssend_t *ievent = NULL; + ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpdnssend); + ievent->req = uvreq; + ievent->sock = sock; - ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpdnssend); - ievent->req = uvreq; - ievent->sock = sock; - - isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid], - (isc__netievent_t *)ievent); - } + isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid], + (isc__netievent_t *)ievent); } static void tcpdns_close_direct(isc_nmsocket_t *sock) { REQUIRE(sock->tid == isc_nm_tid()); + if (sock->timer_running) { + uv_timer_stop(&sock->timer); + sock->timer_running = false; + } + /* We don't need atomics here, it's all in single network thread */ if (sock->self != NULL) { isc__nmsocket_detach(&sock->self); @@ -673,6 +691,11 @@ isc__nm_tcpdns_close(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_tcpdnssocket); + if (!atomic_compare_exchange_strong(&sock->closing, &(bool){ false }, + true)) { + return; + } + if (sock->tid == isc_nm_tid()) { tcpdns_close_direct(sock); } else { @@ -689,8 +712,12 @@ void isc__nm_async_tcpdnsclose(isc__networker_t *worker, isc__netievent_t *ev0) { isc__netievent_tcpdnsclose_t *ievent = (isc__netievent_tcpdnsclose_t *)ev0; + isc_nmsocket_t *sock = ievent->sock; - REQUIRE(worker->id == ievent->sock->tid); + REQUIRE(VALID_NMSOCK(sock)); + REQUIRE(sock->tid == isc_nm_tid()); + + UNUSED(worker); tcpdns_close_direct(ievent->sock); } @@ -739,6 +766,7 @@ tcpdnsconnect_cb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { dnssock->timer_initialized = true; uv_timer_start(&dnssock->timer, dnstcp_readtimeout, dnssock->read_timeout, 0); + dnssock->timer_running = true; /* * The connection is now established; we start reading immediately, @@ -764,7 +792,7 @@ isc_nm_tcpdnsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, timeout, 0)); } -isc_result_t +void isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) { isc_nmsocket_t *sock = handle->sock; isc__netievent_tcpdnsread_t *ievent = NULL; @@ -775,6 +803,11 @@ isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) { REQUIRE(sock->tid == isc_nm_tid()); REQUIRE(atomic_load(&sock->client)); + if (inactive(sock)) { + cb(handle, ISC_R_NOTCONNECTED, NULL, cbarg); + return; + } + /* * This MUST be done asynchronously, no matter which thread we're * in. The callback function for isc_nm_read() often calls @@ -794,7 +827,6 @@ isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) { isc_nmhandle_attach(handle, &eventhandle); isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid], (isc__netievent_t *)ievent); - return (ISC_R_SUCCESS); } void @@ -811,11 +843,15 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) { handle = sock->statichandle; - if (sock->type != isc_nm_tcpdnssocket || sock->outerhandle == NULL) { - if (sock->recv_cb != NULL) { - sock->recv_cb(handle, ISC_R_NOTCONNECTED, NULL, - sock->recv_cbarg); + if (inactive(sock)) { + isc_nm_recv_cb_t cb = sock->recv_cb; + void *cbarg = sock->recv_cbarg; + + isc__nmsocket_clearcb(sock); + if (cb != NULL) { + cb(handle, ISC_R_NOTCONNECTED, NULL, cbarg); } + isc_nmhandle_detach(&handle); return; } @@ -836,6 +872,7 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) { if (sock->timer_initialized) { uv_timer_start(&sock->timer, dnstcp_readtimeout, sock->read_timeout, 0); + sock->timer_running = true; } isc_nm_resumeread(sock->outerhandle); } else {