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] 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 255c44fe2c..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 @@ -260,20 +261,38 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, REQUIRE(dnssock->tid == isc_nm_tid()); REQUIRE(VALID_NMHANDLE(handle)); + 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)) + { + 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 @@ -482,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; } } @@ -546,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); } /* @@ -587,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; } @@ -602,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); @@ -668,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 { @@ -684,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); } @@ -734,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, @@ -759,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; @@ -770,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 @@ -789,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 @@ -806,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; } @@ -831,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 {