From b432d5d3bcccf199141564b6a87d2cdac296ed7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 14 Jun 2022 09:17:08 +0200 Subject: [PATCH 1/2] Gracefully handle uv_read_start() failures Under specific rare timing circumstances the uv_read_start() could fail with UV_EINVAL when the connection is reset between the connect (or accept) and the uv_read_start() call on the nmworker loop. Handle such situation gracefully by propagating the errors from uv_read_start() into upper layers, so the socket can be internally closed(). --- lib/isc/netmgr/netmgr-int.h | 4 ++-- lib/isc/netmgr/netmgr.c | 32 ++++++++++++++++++++------------ lib/isc/netmgr/tcp.c | 10 ++++++++-- lib/isc/netmgr/tcpdns.c | 25 +++++++++++++++++++------ lib/isc/netmgr/tlsdns.c | 27 +++++++++++++++++++++------ lib/isc/netmgr/udp.c | 5 +++-- 6 files changed, 73 insertions(+), 30 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 1a9114f8ae..f1a6bce08e 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -2098,11 +2098,11 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf); void isc__nm_tlsdns_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf); -void +isc_result_t isc__nm_start_reading(isc_nmsocket_t *sock); void isc__nm_stop_reading(isc_nmsocket_t *sock); -void +isc_result_t isc__nm_process_sock_buffer(isc_nmsocket_t *sock); void isc__nm_resume_processing(void *arg); diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index ad9dc3d57f..6d73d0e288 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -2161,39 +2161,42 @@ isc__nm_alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf) { worker->recvbuf_inuse = true; } -void +isc_result_t isc__nm_start_reading(isc_nmsocket_t *sock) { + isc_result_t result = ISC_R_SUCCESS; int r; if (atomic_load(&sock->reading)) { - return; + return (ISC_R_SUCCESS); } switch (sock->type) { case isc_nm_udpsocket: r = uv_udp_recv_start(&sock->uv_handle.udp, isc__nm_alloc_cb, isc__nm_udp_read_cb); - UV_RUNTIME_CHECK(uv_udp_recv_start, r); break; case isc_nm_tcpsocket: r = uv_read_start(&sock->uv_handle.stream, isc__nm_alloc_cb, isc__nm_tcp_read_cb); - UV_RUNTIME_CHECK(uv_read_start, r); break; case isc_nm_tcpdnssocket: r = uv_read_start(&sock->uv_handle.stream, isc__nm_alloc_cb, isc__nm_tcpdns_read_cb); - UV_RUNTIME_CHECK(uv_read_start, r); break; case isc_nm_tlsdnssocket: r = uv_read_start(&sock->uv_handle.stream, isc__nm_alloc_cb, isc__nm_tlsdns_read_cb); - UV_RUNTIME_CHECK(uv_read_start, r); break; default: UNREACHABLE(); } - atomic_store(&sock->reading, true); + if (r != 0) { + result = isc_uverr2result(r); + } else { + atomic_store(&sock->reading, true); + } + + return (result); } void @@ -2254,7 +2257,7 @@ processbuffer(isc_nmsocket_t *sock) { * Stop reading if this is a client socket. In this case we'll be * called again later by isc__nm_resume_processing(). */ -void +isc_result_t isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { for (;;) { int_fast32_t ah = atomic_load(&sock->ah); @@ -2265,7 +2268,10 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { * Don't reset the timer until we have a * full DNS message. */ - isc__nm_start_reading(sock); + result = isc__nm_start_reading(sock); + if (result != ISC_R_SUCCESS) { + return (result); + } /* * Start the timer only if there are no externally used * active handles, there's always one active handle @@ -2275,11 +2281,11 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { if (ah == 1) { isc__nmsocket_timer_start(sock); } - return; + goto done; case ISC_R_CANCELED: isc__nmsocket_timer_stop(sock); isc__nm_stop_reading(sock); - return; + goto done; case ISC_R_SUCCESS: /* * Stop the timer on the successful message read, this @@ -2290,13 +2296,15 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { if (atomic_load(&sock->client)) { isc__nm_stop_reading(sock); - return; + goto done; } break; default: UNREACHABLE(); } } +done: + return (ISC_R_SUCCESS); } void diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 86e7c85e26..f7af009081 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -747,18 +747,24 @@ isc__nm_async_tcpstartread(isc__networker_t *worker, isc__netievent_t *ev0) { isc__netievent_tcpstartread_t *ievent = (isc__netievent_tcpstartread_t *)ev0; isc_nmsocket_t *sock = ievent->sock; + isc_result_t result; REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); UNUSED(worker); if (isc__nmsocket_closing(sock)) { + result = ISC_R_CANCELED; + } else { + result = isc__nm_start_reading(sock); + } + + if (result != ISC_R_SUCCESS) { atomic_store(&sock->reading, true); - isc__nm_tcp_failed_read_cb(sock, ISC_R_CANCELED); + isc__nm_tcp_failed_read_cb(sock, result); return; } - isc__nm_start_reading(sock); isc__nmsocket_timer_start(sock); } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index e896d4ea4f..e0da11b43f 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -714,6 +714,7 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) { isc__netievent_tcpdnsread_t *ievent = (isc__netievent_tcpdnsread_t *)ev0; isc_nmsocket_t *sock = ievent->sock; + isc_result_t result; UNUSED(worker); @@ -721,12 +722,15 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) { REQUIRE(sock->tid == isc_nm_tid()); if (isc__nmsocket_closing(sock)) { - atomic_store(&sock->reading, true); - isc__nm_failed_read_cb(sock, ISC_R_CANCELED, false); - return; + result = ISC_R_CANCELED; + } else { + result = isc__nm_process_sock_buffer(sock); } - isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + atomic_store(&sock->reading, true); + isc__nm_failed_read_cb(sock, result, false); + } } /* @@ -836,6 +840,7 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread, isc_nmsocket_t *sock = uv_handle_get_data((uv_handle_t *)stream); uint8_t *base = NULL; size_t len; + isc_result_t result; REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); @@ -878,7 +883,10 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread, sock->read_timeout = atomic_load(&sock->mgr->idle); } - isc__nm_process_sock_buffer(sock); + result = isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + isc__nm_failed_read_cb(sock, result, true); + } free: if (nread < 0) { /* @@ -1029,7 +1037,12 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { * prep_destroy()->tcpdns_close_direct(). */ isc_nmhandle_attach(handle, &csock->recv_handle); - isc__nm_process_sock_buffer(csock); + result = isc__nm_process_sock_buffer(csock); + if (result != ISC_R_SUCCESS) { + isc_nmhandle_detach(&csock->recv_handle); + isc_nmhandle_detach(&handle); + goto failure; + } /* * The initial timer has been set, update the read timeout for the next diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index b316449647..74b66ac0a8 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -316,7 +316,11 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) { /* Setting pending req */ sock->tls.pending_req = req; - isc__nm_process_sock_buffer(sock); + result = isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + sock->tls.pending_req = NULL; + goto error; + } result = tls_cycle(sock); if (result != ISC_R_SUCCESS) { @@ -1068,8 +1072,10 @@ tls_cycle_input(isc_nmsocket_t *sock) { /* * Process what's in the buffer so far */ - isc__nm_process_sock_buffer(sock); - + result = isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + goto failure; + } /* * FIXME: Should we call * isc__nm_failed_read_cb()? @@ -1081,7 +1087,10 @@ tls_cycle_input(isc_nmsocket_t *sock) { sock->buf_len += len; - isc__nm_process_sock_buffer(sock); + result = isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + goto failure; + } } } else if (!SSL_is_init_finished(sock->tls.tls)) { if (SSL_is_server(sock->tls.tls)) { @@ -1103,7 +1112,10 @@ tls_cycle_input(isc_nmsocket_t *sock) { if (sock->tls.state == TLS_STATE_NONE && !SSL_is_init_finished(sock->tls.tls)) { sock->tls.state = TLS_STATE_HANDSHAKE; - isc__nm_process_sock_buffer(sock); + result = isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + goto failure; + } } /* else continue reading */ break; @@ -1656,7 +1668,10 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { isc_nmhandle_detach(&handle); - isc__nm_process_sock_buffer(csock); + result = isc__nm_process_sock_buffer(csock); + if (result != ISC_R_SUCCESS) { + goto failure; + } /* * sock is now attached to the handle. diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index 505cd0442b..37a3f0480a 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -1118,7 +1118,7 @@ void isc__nm_async_udpread(isc__networker_t *worker, isc__netievent_t *ev0) { isc__netievent_udpread_t *ievent = (isc__netievent_udpread_t *)ev0; isc_nmsocket_t *sock = ievent->sock; - isc_result_t result = ISC_R_SUCCESS; + isc_result_t result; UNUSED(worker); @@ -1129,6 +1129,8 @@ isc__nm_async_udpread(isc__networker_t *worker, isc__netievent_t *ev0) { result = ISC_R_SHUTTINGDOWN; } else if (isc__nmsocket_closing(sock)) { result = ISC_R_CANCELED; + } else { + result = isc__nm_start_reading(sock); } if (result != ISC_R_SUCCESS) { @@ -1137,7 +1139,6 @@ isc__nm_async_udpread(isc__networker_t *worker, isc__netievent_t *ev0) { return; } - isc__nm_start_reading(sock); isc__nmsocket_timer_start(sock); } From 646df5cbbc30fe47ec1787478cea4a4e231776a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 14 Jun 2022 09:23:32 +0200 Subject: [PATCH 2/2] Add CHANGES and release note for [GL #3400] --- CHANGES | 5 +++++ doc/notes/notes-current.rst | 3 +++ 2 files changed, 8 insertions(+) diff --git a/CHANGES b/CHANGES index 761ecfb031..7c32fb18a7 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +5905. [bug] When the TCP connection would be closed/reset between + the connect/accept and the read, the uv_read_start() + return value would be unexpected and cause an assertion + failure. [GL #3400] + 5904. [func] Changed dnssec-signzone -H default to 0 additional NSEC3 iterations. [GL #3395] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 3b77dcbc67..95a2ff04fe 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -46,3 +46,6 @@ Bug Fixes - It was possible for a catalog zone consumer to process a catalog zone member zone when there was a configured pre-existing forward-only forward zone with the same name. This has been fixed. :gl:`#2506`. + +- Fix the assertion failure caused by TCP connection closing between the + connect (or accept) and the read from the socket. :gl:`#3400`