From 579270509f68099019a042073b0d9a3721138328 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(). (cherry picked from commit b432d5d3bcccf199141564b6a87d2cdac296ed7e) --- 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 650ee7aa49..55eff3ce24 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -2130,11 +2130,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 cffd2dd9b0..6494e99743 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -2201,39 +2201,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__nm_uverr2result(r); + } else { + atomic_store(&sock->reading, true); + } + + return (result); } void @@ -2295,7 +2298,7 @@ processbuffer(isc_nmsocket_t *sock) { * has been set to sequential mode. 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); @@ -2306,7 +2309,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 @@ -2316,11 +2322,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 @@ -2332,13 +2338,15 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { if (atomic_load(&sock->client) || atomic_load(&sock->sequential)) { 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 b8358f4cbc..040bebc0aa 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -751,18 +751,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 779b42301a..ba6f665699 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -718,6 +718,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); @@ -725,12 +726,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); + } } /* @@ -840,6 +844,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()); @@ -882,7 +887,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) { /* @@ -1034,7 +1042,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 1f9881c10c..8c0f855428 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -303,7 +303,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) { @@ -1043,8 +1047,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()? @@ -1056,7 +1062,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)) { @@ -1078,7 +1087,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; @@ -1630,7 +1642,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 673c04157c..c5449892d0 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -1120,7 +1120,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); @@ -1131,6 +1131,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) { @@ -1139,7 +1141,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 e644acbfaec31e59970ec53c140cafa0c4547ff4 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] (cherry picked from commit 646df5cbbc30fe47ec1787478cea4a4e231776a2) --- CHANGES | 5 +++++ doc/notes/notes-current.rst | 3 +++ 2 files changed, 8 insertions(+) diff --git a/CHANGES b/CHANGES index f9e9f9d2ed..c60eec3ba3 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 f687b1c452..c04c2aed0c 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`