From 7df8c7061c73570a9c75a6835e4312331bee99f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 31 Mar 2021 11:48:41 +0200 Subject: [PATCH] Fix and clean up handling of connect callbacks Serveral problems were discovered and fixed after the change in the connection timeout in the previous commits: * In TLSDNS, the connection callback was not called at all under some circumstances when the TCP connection had been established, but the TLS handshake hadn't been completed yet. Additional checks have been put in place so that tls_cycle() will end early when the nmsocket is invalidated by the isc__nm_tlsdns_shutdown() call. * In TCP, TCPDNS and TLSDNS, new connections would be established even when the network manager was shutting down. The new call isc__nm_closing() has been added and is used to bail out early even before uv_tcp_connect() is attempted. --- lib/isc/netmgr/netmgr-int.h | 11 ++++- lib/isc/netmgr/netmgr.c | 14 ++++-- lib/isc/netmgr/tcp.c | 29 ++++++----- lib/isc/netmgr/tcpdns.c | 24 ++++++---- lib/isc/netmgr/tlsdns.c | 95 +++++++++++++++++++++++++------------ lib/isc/netmgr/udp.c | 8 ++-- 6 files changed, 118 insertions(+), 63 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index af734dd759..a0edef8ef8 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -1134,6 +1134,13 @@ isc___nmsocket_prep_destroy(isc_nmsocket_t *sock FLARG); * if there are no remaining references or active handles. */ +void +isc__nmsocket_shutdown(isc_nmsocket_t *sock); +/*%< + * Initiate the socket shutdown which actively calls the active + * callbacks. + */ + bool isc__nmsocket_active(isc_nmsocket_t *sock); /*%< @@ -1876,7 +1883,9 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock); void isc__nm_resume_processing(void *arg); bool -isc__nm_inactive(isc_nmsocket_t *sock); +isc__nmsocket_closing(isc_nmsocket_t *sock); +bool +isc__nm_closing(isc_nmsocket_t *sock); void isc__nm_alloc_dnsbuf(isc_nmsocket_t *sock, size_t len); diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index ab7b0ac048..815fd74a38 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -1521,7 +1521,7 @@ isc__nmhandle_detach(isc_nmhandle_t **handlep FLARG) { } } -static void +void isc__nmsocket_shutdown(isc_nmsocket_t *sock); static void @@ -1670,6 +1670,7 @@ isc__nm_failed_connect_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, REQUIRE(req->cb.connect != NULL); isc__nmsocket_timer_stop(sock); + uv_handle_set_data((uv_handle_t *)&sock->timer, sock); atomic_store(&sock->connecting, false); @@ -1930,7 +1931,12 @@ isc__nm_stop_reading(isc_nmsocket_t *sock) { } bool -isc__nm_inactive(isc_nmsocket_t *sock) { +isc__nm_closing(isc_nmsocket_t *sock) { + return (atomic_load(&sock->mgr->closing)); +} + +bool +isc__nmsocket_closing(isc_nmsocket_t *sock) { return (!isc__nmsocket_active(sock) || atomic_load(&sock->closing) || atomic_load(&sock->mgr->closing) || (sock->server != NULL && !isc__nmsocket_active(sock->server))); @@ -2017,7 +2023,7 @@ isc__nm_resume_processing(void *arg) { REQUIRE(sock->tid == isc_nm_tid()); REQUIRE(!atomic_load(&sock->client)); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { return; } @@ -2461,7 +2467,7 @@ isc__nm_async_detach(isc__networker_t *worker, isc__netievent_t *ev0) { nmhandle_detach_cb(&ievent->handle FLARG_PASS); } -static void +void isc__nmsocket_shutdown(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); switch (sock->type) { diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 6bfe0d1531..9a3a18592d 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -95,13 +95,6 @@ stop_reading(isc_nmsocket_t *sock); static void tcp_alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf); -static bool -inactive(isc_nmsocket_t *sock) { - return (!isc__nmsocket_active(sock) || atomic_load(&sock->closing) || - atomic_load(&sock->mgr->closing) || - (sock->server != NULL && !isc__nmsocket_active(sock->server))); -} - static void failed_accept_cb(isc_nmsocket_t *sock, isc_result_t eresult) { REQUIRE(sock->accepting); @@ -158,6 +151,12 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { r = uv_timer_init(&worker->loop, &sock->timer); RUNTIME_CHECK(r == 0); + uv_handle_set_data((uv_handle_t *)&sock->timer, sock); + + if (isc__nm_closing(sock)) { + result = ISC_R_CANCELED; + goto error; + } r = uv_tcp_open(&sock->uv_handle.tcp, sock->fd); if (r != 0) { @@ -193,7 +192,7 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { done: result = isc__nm_uverr2result(r); - +error: LOCK(&sock->lock); sock->result = result; SIGNAL(&sock->cond); @@ -263,7 +262,7 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) { */ isc__nm_uvreq_put(&req, sock); return; - } else if (!isc__nmsocket_active(sock)) { + } else if (isc__nmsocket_closing(sock)) { /* Socket was closed midflight by isc__nm_tcp_shutdown() */ result = ISC_R_CANCELED; goto error; @@ -608,7 +607,7 @@ tcp_connection_cb(uv_stream_t *server, int status) { REQUIRE(VALID_NMSOCK(ssock)); REQUIRE(ssock->tid == isc_nm_tid()); - if (inactive(ssock)) { + if (isc__nmsocket_closing(ssock)) { result = ISC_R_CANCELED; goto done; } @@ -826,7 +825,7 @@ isc__nm_async_tcpstartread(isc__networker_t *worker, isc__netievent_t *ev0) { REQUIRE(sock->tid == isc_nm_tid()); UNUSED(worker); - if (inactive(sock)) { + if (isc__nmsocket_closing(sock)) { sock->reading = true; failed_read_cb(sock, ISC_R_CANCELED); return; @@ -914,7 +913,7 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) { REQUIRE(sock->reading); REQUIRE(buf != NULL); - if (inactive(sock)) { + if (isc__nmsocket_closing(sock)) { failed_read_cb(sock, ISC_R_CANCELED); goto free; } @@ -1013,7 +1012,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { REQUIRE(VALID_NMSOCK(ssock)); REQUIRE(ssock->tid == isc_nm_tid()); - if (inactive(ssock)) { + if (isc__nmsocket_closing(ssock)) { if (quota != NULL) { isc_quota_detach("a); } @@ -1187,7 +1186,7 @@ tcp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { int r; - if (inactive(sock)) { + if (isc__nmsocket_closing(sock)) { return (ISC_R_CANCELED); } @@ -1417,7 +1416,7 @@ isc__nm_tcp_shutdown(isc_nmsocket_t *sock) { return; } - if (sock->statichandle) { + if (sock->statichandle != NULL) { failed_read_cb(sock, ISC_R_CANCELED); return; } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 945f05b8f2..dbdb21332d 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -106,6 +106,11 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { RUNTIME_CHECK(r == 0); uv_handle_set_data((uv_handle_t *)&sock->timer, sock); + if (isc__nm_closing(sock)) { + result = ISC_R_CANCELED; + goto error; + } + r = uv_tcp_open(&sock->uv_handle.tcp, sock->fd); if (r != 0) { isc__nm_closesocket(sock->fd); @@ -144,7 +149,7 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { done: result = isc__nm_uverr2result(r); - +error: LOCK(&sock->lock); sock->result = result; SIGNAL(&sock->cond); @@ -212,7 +217,7 @@ tcpdns_connect_cb(uv_connect_t *uvreq, int status) { */ isc__nm_uvreq_put(&req, sock); return; - } else if (!isc__nmsocket_active(sock)) { + } else if (isc__nmsocket_closing(sock)) { /* Socket was closed midflight by isc__nm_tcpdns_shutdown() */ result = ISC_R_CANCELED; goto error; @@ -560,7 +565,7 @@ tcpdns_connection_cb(uv_stream_t *server, int status) { REQUIRE(VALID_NMSOCK(ssock)); REQUIRE(ssock->tid == isc_nm_tid()); - if (isc__nm_inactive(ssock)) { + if (isc__nmsocket_closing(ssock)) { result = ISC_R_CANCELED; goto done; } @@ -714,7 +719,7 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { sock->reading = true; isc__nm_failed_read_cb(sock, ISC_R_CANCELED); return; @@ -741,7 +746,7 @@ isc__nm_tcpdns_processbuffer(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { return (ISC_R_CANCELED); } @@ -818,7 +823,7 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread, REQUIRE(sock->reading); REQUIRE(buf != NULL); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { isc__nm_failed_read_cb(sock, ISC_R_CANCELED); goto free; } @@ -917,7 +922,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { REQUIRE(VALID_NMSOCK(ssock)); REQUIRE(ssock->tid == isc_nm_tid()); - if (isc__nm_inactive(ssock)) { + if (isc__nmsocket_closing(ssock)) { if (quota != NULL) { isc_quota_detach("a); } @@ -1111,7 +1116,7 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) { UNUSED(worker); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { result = ISC_R_CANCELED; goto fail; } @@ -1295,6 +1300,7 @@ tcpdns_close_direct(isc_nmsocket_t *sock) { isc__nmsocket_timer_stop(sock); isc__nm_stop_reading(sock); + uv_handle_set_data((uv_handle_t *)&sock->timer, sock); uv_close((uv_handle_t *)&sock->timer, timer_close_cb); } @@ -1375,7 +1381,7 @@ isc__nm_tcpdns_shutdown(isc_nmsocket_t *sock) { return; } - if (sock->statichandle) { + if (sock->statichandle != NULL) { isc__nm_failed_read_cb(sock, ISC_R_CANCELED); return; } diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index ab99721188..c7aba2cd35 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -110,6 +110,10 @@ tlsdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { atomic_store(&sock->connecting, true); + /* 2 minute timeout */ + result = isc__nm_socket_connectiontimeout(sock->fd, 120 * 1000); + RUNTIME_CHECK(result == ISC_R_SUCCESS); + r = uv_tcp_init(&worker->loop, &sock->uv_handle.tcp); RUNTIME_CHECK(r == 0); uv_handle_set_data(&sock->uv_handle.handle, sock); @@ -118,6 +122,11 @@ tlsdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { RUNTIME_CHECK(r == 0); uv_handle_set_data((uv_handle_t *)&sock->timer, sock); + if (isc__nm_closing(sock)) { + result = ISC_R_CANCELED; + goto error; + } + r = uv_tcp_open(&sock->uv_handle.tcp, sock->fd); if (r != 0) { isc__nm_closesocket(sock->fd); @@ -156,7 +165,7 @@ tlsdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { done: result = isc__nm_uverr2result(r); - +error: LOCK(&sock->lock); sock->result = result; SIGNAL(&sock->cond); @@ -209,9 +218,6 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - isc__nmsocket_timer_stop(sock); - uv_handle_set_data((uv_handle_t *)&sock->timer, sock); - req = uv_handle_get_data((uv_handle_t *)uvreq); REQUIRE(VALID_UVREQ(req)); @@ -224,8 +230,8 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) { */ isc__nm_uvreq_put(&req, sock); return; - } else if (!isc__nmsocket_active(sock)) { - /* Socket was closed midflight by isc__nm_tcpdns_shutdown() */ + } else if (isc__nmsocket_closing(sock)) { + /* Socket was closed midflight by isc__nm_tlsdns_shutdown() */ result = ISC_R_CANCELED; goto error; } else if (status == UV_ETIMEDOUT) { @@ -279,19 +285,20 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) { result = isc_sockaddr_fromsockaddr(&sock->peer, (struct sockaddr *)&ss); RUNTIME_CHECK(result == ISC_R_SUCCESS); + /* Setting pending req */ sock->tls.pending_req = req; isc__nm_process_sock_buffer(sock); result = tls_cycle(sock); if (result != ISC_R_SUCCESS) { + sock->tls.pending_req = NULL; goto error; } return; error: - sock->tls.pending_req = NULL; isc__nm_failed_connect_cb(sock, req, result); } @@ -616,7 +623,7 @@ tlsdns_connection_cb(uv_stream_t *server, int status) { REQUIRE(VALID_NMSOCK(ssock)); REQUIRE(ssock->tid == isc_nm_tid()); - if (isc__nm_inactive(ssock)) { + if (isc__nmsocket_closing(ssock)) { result = ISC_R_CANCELED; goto done; } @@ -854,7 +861,7 @@ isc__nm_async_tlsdnsread(isc__networker_t *worker, isc__netievent_t *ev0) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { sock->reading = true; isc__nm_failed_read_cb(sock, ISC_R_CANCELED); return; @@ -884,7 +891,7 @@ isc__nm_tlsdns_processbuffer(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { return (ISC_R_CANCELED); } @@ -978,16 +985,19 @@ tls_cycle_input(isc_nmsocket_t *sock) { sock->buf + sock->buf_len, sock->buf_size - sock->buf_len, &len); if (rv != 1) { - /* Process what's in the buffer so far + /* + * Process what's in the buffer so far */ isc__nm_process_sock_buffer(sock); - /* FIXME: Should we call - * failed_read_cb()? */ + /* + * FIXME: Should we call + * isc__nm_failed_read_cb()? + */ break; } - REQUIRE((size_t)pending == len); + INSIST((size_t)pending == len); sock->buf_len += len; @@ -1052,6 +1062,9 @@ tls_cycle_input(isc_nmsocket_t *sock) { isc__nm_uvreq_t *req = sock->tls.pending_req; sock->tls.pending_req = NULL; + isc__nmsocket_timer_stop(sock); + uv_handle_set_data((uv_handle_t *)&sock->timer, sock); + atomic_store(&sock->connecting, false); isc__nm_connectcb(sock, req, ISC_R_SUCCESS, true); @@ -1067,7 +1080,13 @@ tls_error(isc_nmsocket_t *sock, isc_result_t result) { switch (sock->tls.state) { case TLS_STATE_HANDSHAKE: case TLS_STATE_IO: - isc__nm_tlsdns_failed_read_cb(sock, result); + if (atomic_load(&sock->connecting)) { + isc__nm_uvreq_t *req = sock->tls.pending_req; + sock->tls.pending_req = NULL; + isc__nm_failed_connect_cb(sock, req, result); + } else { + isc__nm_tlsdns_failed_read_cb(sock, result); + } break; case TLS_STATE_ERROR: return; @@ -1078,7 +1097,7 @@ tls_error(isc_nmsocket_t *sock, isc_result_t result) { sock->tls.state = TLS_STATE_ERROR; sock->tls.pending_error = result; - /* tlsdns_close_direct(sock); */ + isc__nmsocket_shutdown(sock); } static void @@ -1205,6 +1224,10 @@ static isc_result_t tls_cycle(isc_nmsocket_t *sock) { isc_result_t result; + if (isc__nmsocket_closing(sock)) { + return (ISC_R_CANCELED); + } + result = tls_pop_error(sock); if (result != ISC_R_SUCCESS) { goto done; @@ -1235,7 +1258,7 @@ async_tlsdns_cycle(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); /* Socket was closed midflight by isc__nm_tlsdns_shutdown() */ - if (!isc__nmsocket_active(sock)) { + if (isc__nmsocket_closing(sock)) { return; } @@ -1278,7 +1301,7 @@ isc__nm_tlsdns_read_cb(uv_stream_t *stream, ssize_t nread, REQUIRE(sock->reading); REQUIRE(buf != NULL); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { isc__nm_failed_read_cb(sock, ISC_R_CANCELED); goto free; } @@ -1375,7 +1398,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { REQUIRE(VALID_NMSOCK(ssock)); REQUIRE(ssock->tid == isc_nm_tid()); - if (isc__nm_inactive(ssock)) { + if (isc__nmsocket_closing(ssock)) { if (quota != NULL) { isc_quota_detach("a); } @@ -1608,7 +1631,7 @@ tlsdns_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { return (result); } - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { return (ISC_R_CANCELED); } @@ -1813,6 +1836,8 @@ tlsdns_close_direct(isc_nmsocket_t *sock) { isc__nmsocket_timer_stop(sock); isc__nm_stop_reading(sock); + + uv_handle_set_data((uv_handle_t *)&sock->timer, sock); uv_close((uv_handle_t *)&sock->timer, timer_close_cb); } @@ -1883,27 +1908,37 @@ isc__nm_tlsdns_shutdown(isc_nmsocket_t *sock) { return; } + if (sock->tls.tls) { + /* Shutdown any active TLS connections */ + (void)SSL_shutdown(sock->tls.tls); + } + if (sock->accepting) { return; } - if (sock->tls.pending_req != NULL) { - isc__nm_uvreq_t *req = sock->tls.pending_req; - sock->tls.pending_req = NULL; - - isc__nm_failed_connect_cb(sock, req, ISC_R_CANCELED); - - return; - } - + /* TLS handshake hasn't been completed yet */ if (atomic_load(&sock->connecting)) { + /* + * TCP connection has been established, now waiting on + * TLS handshake to complete + */ + if (sock->tls.pending_req != NULL) { + isc__nm_uvreq_t *req = sock->tls.pending_req; + sock->tls.pending_req = NULL; + + isc__nm_failed_connect_cb(sock, req, ISC_R_CANCELED); + return; + } + + /* The TCP connection hasn't been established yet */ isc_nmsocket_t *tsock = NULL; isc__nmsocket_attach(sock, &tsock); uv_close(&sock->uv_handle.handle, tlsdns_close_connect_cb); return; } - if (sock->statichandle) { + if (sock->statichandle != NULL) { isc__nm_failed_read_cb(sock, ISC_R_CANCELED); return; } diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index 0a7496404d..843be0b6ac 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -523,7 +523,7 @@ isc__nm_async_udpsend(isc__networker_t *worker, isc__netievent_t *ev0) { REQUIRE(sock->tid == isc_nm_tid()); UNUSED(worker); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { isc__nm_failed_send_cb(sock, uvreq, ISC_R_CANCELED); return; } @@ -567,7 +567,7 @@ udp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, REQUIRE(sock->tid == isc_nm_tid()); REQUIRE(sock->type == isc_nm_udpsocket); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { return (ISC_R_CANCELED); } @@ -868,7 +868,7 @@ isc__nm_async_udpread(isc__networker_t *worker, isc__netievent_t *ev0) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); - if (isc__nm_inactive(sock)) { + if (isc__nmsocket_closing(sock)) { sock->reading = true; isc__nm_failed_read_cb(sock, ISC_R_CANCELED); return; @@ -1084,7 +1084,7 @@ isc__nm_udp_shutdown(isc_nmsocket_t *sock) { * sock->statichandle would be NULL, in that case, nobody is * interested in the callback. */ - if (sock->statichandle) { + if (sock->statichandle != NULL) { isc__nm_failed_read_cb(sock, ISC_R_CANCELED); return; }