From 4c5b36780b31189c3b937af886e085af4c50348b Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 21 Apr 2021 15:15:27 +0300 Subject: [PATCH 01/11] Fix flawed DoH unit tests logic This commit fixes some logical mistakes in DoH unit tests logic, causing them either to fail or not to do what they are intended to do. --- lib/isc/tests/doh_test.c | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index 759adc97ab..a0a93584d2 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -55,6 +55,7 @@ static uint64_t stop_magic = 0; static uv_buf_t send_msg = { .base = (char *)&send_magic, .len = sizeof(send_magic) }; +static atomic_int_fast64_t active_cconnects = ATOMIC_VAR_INIT(0); static atomic_int_fast64_t nsends = ATOMIC_VAR_INIT(0); static atomic_int_fast64_t ssends = ATOMIC_VAR_INIT(0); static atomic_int_fast64_t sreads = ATOMIC_VAR_INIT(0); @@ -119,10 +120,10 @@ connect_send_cb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { REQUIRE(VALID_NMHANDLE(handle)); + (void)atomic_fetch_sub(&active_cconnects, 1); memmove(&data, arg, sizeof(data)); isc_mem_put(handle->sock->mgr->mctx, arg, sizeof(data)); if (result != ISC_R_SUCCESS) { - atomic_store(&slowdown, true); goto error; } @@ -139,6 +140,11 @@ error: data.reply_cb(handle, result, NULL, data.cb_arg); isc_mem_put(handle->sock->mgr->mctx, data.region.base, data.region.length); + if (result == ISC_R_TOOMANYOPENFILES) { + atomic_store(&slowdown, true); + } else { + atomic_store(&was_error, true); + } } static void @@ -296,6 +302,7 @@ nm_setup(void **state) { atomic_store(&sreads, 0); atomic_store(&ssends, 0); atomic_store(&ctimeouts, 0); + atomic_store(&active_cconnects, 0); atomic_store(&was_error, false); @@ -382,9 +389,8 @@ doh_receive_reply_cb(isc_nmhandle_t *handle, isc_result_t eresult, UNUSED(cbarg); UNUSED(region); - (void)atomic_fetch_sub(&nsends, 1); - if (eresult == ISC_R_SUCCESS) { + (void)atomic_fetch_sub(&nsends, 1); atomic_fetch_add(&csends, 1); atomic_fetch_add(&creads, 1); isc_nm_resumeread(handle); @@ -678,11 +684,13 @@ doh_timeout_recovery_GET(void **state) { static void doh_receive_send_reply_cb(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, void *cbarg) { - int_fast64_t sends = atomic_fetch_sub(&nsends, 1); + isc_nmhandle_t *thandle = NULL; assert_non_null(handle); UNUSED(region); + isc_nmhandle_attach(handle, &thandle); if (eresult == ISC_R_SUCCESS) { + int_fast64_t sends = atomic_fetch_sub(&nsends, 1); atomic_fetch_add(&csends, 1); atomic_fetch_add(&creads, 1); if (sends > 0) { @@ -694,12 +702,16 @@ doh_receive_send_reply_cb(isc_nmhandle_t *handle, isc_result_t eresult, .base = (uint8_t *)send_msg.base, .length = send_msg.len }, doh_receive_send_reply_cb, cbarg); + if (eresult == ISC_R_CANCELED) { + break; + } assert_true(eresult == ISC_R_SUCCESS); } } } else { atomic_store(&was_error, true); } + isc_nmhandle_detach(&thandle); } static isc_threadresult_t @@ -716,8 +728,9 @@ doh_connect_thread(isc_threadarg_t arg) { * We need to back off and slow down if we start getting * errors, to prevent a thundering herd problem. */ - if (atomic_load(&slowdown)) { - isc_test_nap(1000 * workers); + int_fast64_t active = atomic_fetch_add(&active_cconnects, 1); + if (atomic_load(&slowdown) || active > workers) { + isc_test_nap(1000 * (active - workers)); atomic_store(&slowdown, false); } connect_send_request( @@ -1018,6 +1031,8 @@ doh_recv_half_send(void **state) { size_t nthreads = ISC_MAX(ISC_MIN(workers, 32), 1); isc_thread_t threads[32] = { 0 }; + atomic_store(&nsends, (NSENDS * NWRITES) / 2); + result = isc_nm_listenhttp( listen_nm, (isc_nmiface_t *)&tcp_listen_addr, 0, NULL, atomic_load(&use_TLS) ? server_tlsctx : NULL, &listen_sock); @@ -1031,7 +1046,7 @@ doh_recv_half_send(void **state) { isc_thread_create(doh_connect_thread, connect_nm, &threads[i]); } - while (atomic_load(&nsends) >= (NSENDS * NWRITES) / 2) { + while (atomic_load(&nsends) > 0) { isc_thread_yield(); } @@ -1092,6 +1107,8 @@ doh_half_recv_send(void **state) { size_t nthreads = ISC_MAX(ISC_MIN(workers, 32), 1); isc_thread_t threads[32] = { 0 }; + atomic_store(&nsends, (NSENDS * NWRITES) / 2); + result = isc_nm_listenhttp( listen_nm, (isc_nmiface_t *)&tcp_listen_addr, 0, NULL, atomic_load(&use_TLS) ? server_tlsctx : NULL, &listen_sock); @@ -1105,7 +1122,7 @@ doh_half_recv_send(void **state) { isc_thread_create(doh_connect_thread, connect_nm, &threads[i]); } - while (atomic_load(&nsends) >= (NSENDS * NWRITES) / 2) { + while (atomic_load(&nsends) > 0) { isc_thread_yield(); } @@ -1166,6 +1183,8 @@ doh_half_recv_half_send(void **state) { size_t nthreads = ISC_MAX(ISC_MIN(workers, 32), 1); isc_thread_t threads[32] = { 0 }; + atomic_store(&nsends, (NSENDS * NWRITES) / 2); + result = isc_nm_listenhttp( listen_nm, (isc_nmiface_t *)&tcp_listen_addr, 0, NULL, atomic_load(&use_TLS) ? server_tlsctx : NULL, &listen_sock); @@ -1179,7 +1198,7 @@ doh_half_recv_half_send(void **state) { isc_thread_create(doh_connect_thread, connect_nm, &threads[i]); } - while (atomic_load(&nsends) >= (NSENDS * NWRITES) / 2) { + while (atomic_load(&nsends) > 0) { isc_thread_yield(); } From 39448c1581578d36a6955b2531abb65931085f55 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 21 Apr 2021 20:32:35 +0300 Subject: [PATCH 02/11] Finish HTTP session on write failure Not doing so caused client-side code to not free file descriptors as soon as possible, that was causing unit tests to fail. --- lib/isc/netmgr/http.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index a77216000d..53f36853c2 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -925,6 +925,9 @@ http_writecb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { http_do_bio(session, NULL, NULL, NULL); session->sending--; isc_nmhandle_detach(&transphandle); + if (result != ISC_R_SUCCESS && session->sending == 0) { + finish_http_session(session); + } isc__nm_httpsession_detach(&session); } From 134914233359d62a5bdfefe7996b7f99c1fee622 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 23 Apr 2021 17:30:59 +0300 Subject: [PATCH 03/11] Got rid of tlsconnect event and corresponding code We do not need it since we decided to not return values from connect functions. --- lib/isc/netmgr/netmgr-int.h | 3 -- lib/isc/netmgr/netmgr.c | 1 - lib/isc/netmgr/tlsstream.c | 104 +++++++----------------------------- 3 files changed, 18 insertions(+), 90 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 521ed58d54..4384932dca 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -1416,9 +1416,6 @@ isc__nm_async_tlsclose(isc__networker_t *worker, isc__netievent_t *ev0); void isc__nm_async_tlssend(isc__networker_t *worker, isc__netievent_t *ev0); -void -isc__nm_async_tlsconnect(isc__networker_t *worker, isc__netievent_t *ev0); - void isc__nm_async_tlsstartread(isc__networker_t *worker, isc__netievent_t *ev0); diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index e8e3ed992a..0699fe807e 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -869,7 +869,6 @@ process_netievent(isc__networker_t *worker, isc__netievent_t *ievent) { NETIEVENT_CASE(tlsstartread); NETIEVENT_CASE(tlssend); NETIEVENT_CASE(tlsclose); - NETIEVENT_CASE(tlsconnect); NETIEVENT_CASE(tlsdobio); NETIEVENT_CASE(tlscancel); diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index f8e0be0d00..3e0cf4012e 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -86,26 +86,6 @@ inactive(isc_nmsocket_t *sock) { atomic_load(&sock->mgr->closing)); } -static void -update_result(isc_nmsocket_t *sock, const isc_result_t result) { - if (!atomic_load(&sock->tlsstream.result_updated)) { - atomic_store(&sock->tlsstream.result_updated, true); - if (!sock->tlsstream.server) { - LOCK(&sock->lock); - sock->result = result; - SIGNAL(&sock->cond); - while (!atomic_load(&sock->active)) { - WAIT(&sock->scond, &sock->lock); - } - UNLOCK(&sock->lock); - } else { - LOCK(&sock->lock); - sock->result = result; - UNLOCK(&sock->lock); - } - } -} - static void tls_call_connect_cb(isc_nmsocket_t *sock, isc_nmhandle_t *handle, const isc_result_t result) { @@ -562,6 +542,7 @@ initialize_tls(isc_nmsocket_t *sock, bool server) { sock->tlsstream.bio_out); sock->tlsstream.server = server; sock->tlsstream.nsending = 0; + sock->tlsstream.state = TLS_INIT; return (ISC_R_SUCCESS); error: isc_tls_free(&sock->tlsstream.tls); @@ -606,7 +587,6 @@ tlslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { tlssock->peer = handle->sock->peer; tlssock->read_timeout = atomic_load(&handle->sock->mgr->init); tlssock->tid = isc_nm_tid(); - tlssock->tlsstream.state = TLS_INIT; tlssock->tlsstream.ctx = tlslistensock->tlsstream.ctx; @@ -875,12 +855,14 @@ isc__nm_tls_stoplistening(isc_nmsocket_t *sock) { } } +static void +tcp_connected(isc_nmhandle_t *handle, isc_result_t result, void *cbarg); + void isc_nm_tlsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, isc_nm_cb_t cb, void *cbarg, SSL_CTX *ctx, unsigned int timeout, size_t extrahandlesize) { - isc_nmsocket_t *nsock = NULL, *tsock = NULL; - isc__netievent_tlsconnect_t *ievent = NULL; + isc_nmsocket_t *nsock = NULL; #if defined(NETMGR_TRACE) && defined(NETMGR_TRACE_VERBOSE) fprintf(stderr, "TLS: isc_nm_tlsconnect(): in net thread: %s\n", isc__nm_in_netthread() ? "yes" : "no"); @@ -900,33 +882,10 @@ isc_nm_tlsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, nsock->connect_timeout = timeout; nsock->tlsstream.ctx = ctx; - ievent = isc__nm_get_netievent_tlsconnect(mgr, nsock); - ievent->local = local->addr; - ievent->peer = peer->addr; - ievent->ctx = ctx; - - isc__nmsocket_attach(nsock, &tsock); - if (isc__nm_in_netthread()) { - atomic_store(&nsock->active, true); - nsock->tid = isc_nm_tid(); - isc__nm_async_tlsconnect(&mgr->workers[nsock->tid], - (isc__netievent_t *)ievent); - isc__nm_put_netievent_tlsconnect(mgr, ievent); - } else { - nsock->tid = isc_random_uniform(mgr->nworkers); - isc__nm_enqueue_ievent(&mgr->workers[nsock->tid], - (isc__netievent_t *)ievent); - } - - LOCK(&nsock->lock); - while (nsock->result == ISC_R_DEFAULT) { - WAIT(&nsock->cond, &nsock->lock); - } - atomic_store(&nsock->active, true); - BROADCAST(&nsock->scond); - UNLOCK(&nsock->lock); - INSIST(VALID_NMSOCK(nsock)); - isc__nmsocket_detach(&tsock); + isc_nm_tcpconnect(mgr, + (isc_nmiface_t *)&nsock->tlsstream.local_iface.addr, + (isc_nmiface_t *)&peer->addr, tcp_connected, nsock, + nsock->connect_timeout, 0); } static void @@ -937,38 +896,11 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { REQUIRE(VALID_NMSOCK(tlssock)); REQUIRE(VALID_NMHANDLE(handle)); + tlssock->tid = isc_nm_tid(); if (result != ISC_R_SUCCESS) { goto error; } - result = initialize_tls(tlssock, false); - if (result != ISC_R_SUCCESS) { - goto error; - } - - tlssock->peer = isc_nmhandle_peeraddr(handle); - isc_nmhandle_attach(handle, &tlssock->outerhandle); - - tls_do_bio(tlssock, NULL, NULL, false); - return; -error: - tlshandle = isc__nmhandle_get(tlssock, NULL, NULL); - atomic_store(&tlssock->closed, true); - tls_call_connect_cb(tlssock, tlshandle, result); - isc_nmhandle_detach(&tlshandle); - isc__nmsocket_detach(&tlssock); -} - -void -isc__nm_async_tlsconnect(isc__networker_t *worker, isc__netievent_t *ev0) { - isc__netievent_tlsconnect_t *ievent = - (isc__netievent_tlsconnect_t *)ev0; - isc_nmsocket_t *tlssock = ievent->sock; - isc_result_t result; - isc_nmhandle_t *tlshandle = NULL; - - UNUSED(worker); - if (isc__nm_closing(tlssock)) { result = ISC_R_CANCELED; goto error; @@ -983,21 +915,21 @@ isc__nm_async_tlsconnect(isc__networker_t *worker, isc__netievent_t *ev0) { goto error; } - tlssock->tid = isc_nm_tid(); - tlssock->tlsstream.state = TLS_INIT; + result = initialize_tls(tlssock, false); + if (result != ISC_R_SUCCESS) { + goto error; + } - isc_nm_tcpconnect(worker->mgr, (isc_nmiface_t *)&ievent->local, - (isc_nmiface_t *)&ievent->peer, tcp_connected, - tlssock, tlssock->connect_timeout, 0); + tlssock->peer = isc_nmhandle_peeraddr(handle); + isc_nmhandle_attach(handle, &tlssock->outerhandle); + atomic_store(&tlssock->active, true); - update_result(tlssock, ISC_R_SUCCESS); + tls_do_bio(tlssock, NULL, NULL, false); return; - error: tlshandle = isc__nmhandle_get(tlssock, NULL, NULL); atomic_store(&tlssock->closed, true); tls_call_connect_cb(tlssock, tlshandle, result); - update_result(tlssock, result); isc_nmhandle_detach(&tlshandle); isc__nmsocket_detach(&tlssock); } From 8510c5cd59e88d74d697376ec596dcbc54f34d66 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 23 Apr 2021 17:50:04 +0300 Subject: [PATCH 04/11] Always call TCP connect callback from within a worker context This change ensures that a TCP connect callback is called from within the context of a worker thread in case of a low-level error when descriptors cannot be created (e.g. when there are too many open file descriptors). --- lib/isc/netmgr/tcp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 80ce1193c2..60cf9222c5 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -317,9 +317,13 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, if (result != ISC_R_SUCCESS) { if (isc__nm_in_netthread()) { sock->tid = isc_nm_tid(); + isc__nmsocket_clearcb(sock); + isc__nm_connectcb(sock, req, result, false); + } else { + isc__nmsocket_clearcb(sock); + sock->tid = isc_random_uniform(mgr->nworkers); + isc__nm_connectcb(sock, req, result, true); } - isc__nmsocket_clearcb(sock); - isc__nm_connectcb(sock, req, result, false); atomic_store(&sock->closed, true); isc__nmsocket_detach(&sock); return; From 0e8ac61d6ed046c0793c2c610714d5b4c11e8580 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 26 Apr 2021 21:04:06 +0300 Subject: [PATCH 05/11] Avoid creating httpclose netievents in case of low level failures This way we create less load on NM workers by avoiding netievent creation. --- lib/isc/netmgr/http.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 53f36853c2..f82a7ae020 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -2309,6 +2309,7 @@ http_close_direct(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); atomic_store(&sock->closed, true); + atomic_store(&sock->active, false); session = sock->h2.session; if (session != NULL && session->handle) { @@ -2318,6 +2319,7 @@ http_close_direct(isc_nmsocket_t *sock) { void isc__nm_http_close(isc_nmsocket_t *sock) { + bool destroy = false; REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_httpsocket); REQUIRE(!isc__nmsocket_active(sock)); @@ -2327,6 +2329,21 @@ isc__nm_http_close(isc_nmsocket_t *sock) { return; } + if (sock->h2.session != NULL && sock->h2.session->closed && + sock->tid == isc_nm_tid()) + { + isc__nm_httpsession_detach(&sock->h2.session); + destroy = true; + } else if (sock->h2.session == NULL && sock->tid == isc_nm_tid()) { + destroy = true; + } + + if (destroy) { + http_close_direct(sock); + isc__nmsocket_prep_destroy(sock); + return; + } + isc__netievent_httpclose_t *ievent = isc__nm_get_netievent_httpclose(sock->mgr, sock); From 0d3f503dc93f4c85979b203a07281819b944b700 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 26 Apr 2021 22:52:59 +0300 Subject: [PATCH 06/11] Avoid creating connect netievents during low level failures in HTTP This way we create less netievent objects, not bombarding NM with the messages in case of numerous low-level errors (like too many open files) in e.g. unit tests. --- lib/isc/netmgr/http.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index f82a7ae020..c2e403a5d0 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1068,19 +1068,28 @@ http_call_connect_cb(isc_nmsocket_t *sock, isc_nm_http_session_t *session, REQUIRE(sock->connect_cb != NULL); - req = isc__nm_uvreq_get(sock->mgr, sock); - req->cb.connect = sock->connect_cb; - req->cbarg = sock->connect_cbarg; - if (session != NULL) { - session->client_httphandle = httphandle; - req->handle = NULL; - isc_nmhandle_attach(httphandle, &req->handle); - } else { - req->handle = httphandle; - } + if (result == ISC_R_SUCCESS) { + req = isc__nm_uvreq_get(sock->mgr, sock); + req->cb.connect = sock->connect_cb; + req->cbarg = sock->connect_cbarg; + if (session != NULL) { + session->client_httphandle = httphandle; + req->handle = NULL; + isc_nmhandle_attach(httphandle, &req->handle); + } else { + req->handle = httphandle; + } - isc__nmsocket_clearcb(sock); - isc__nm_connectcb(sock, req, result, true); + isc__nmsocket_clearcb(sock); + isc__nm_connectcb(sock, req, result, true); + } else { + void *cbarg = sock->connect_cbarg; + isc_nm_cb_t connect_cb = sock->connect_cb; + + isc__nmsocket_clearcb(sock); + connect_cb(httphandle, result, cbarg); + isc_nmhandle_detach(&httphandle); + } } static void @@ -1168,6 +1177,7 @@ error: isc_mem_free(mctx, http_sock->h2.connect.uri); } + isc__nmsocket_prep_destroy(http_sock); isc__nmsocket_detach(&http_sock); } From 3bf331c4533a4759d1eea89a46c88fda822a98a3 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 27 Apr 2021 13:41:57 +0300 Subject: [PATCH 07/11] Fix crashes in TLS when handling TLS shutdown messages This commit fixes some situations which could appear in TLS code when dealing with shutdown messages and lead to crashes. --- lib/isc/netmgr/tlsstream.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 3e0cf4012e..e738d555fb 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -177,7 +177,8 @@ tls_failed_read_cb(isc_nmsocket_t *sock, const isc_result_t result) { } isc__nm_readcb(sock, req, result); if (result == ISC_R_TIMEDOUT && - isc__nmsocket_timer_running(sock->outerhandle->sock)) + (sock->outerhandle == NULL || + isc__nmsocket_timer_running(sock->outerhandle->sock))) { destroy = false; } @@ -400,7 +401,8 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, /* Decrypt and pass data from network to client */ if (sock->tlsstream.state >= TLS_IO && sock->recv_cb != NULL && - !atomic_load(&sock->readpaused)) + !atomic_load(&sock->readpaused) && + sock->statichandle != NULL) { uint8_t recv_buf[TLS_BUF_SIZE]; INSIST(sock->tlsstream.state > TLS_HANDSHAKE); @@ -803,6 +805,7 @@ tls_close_direct(isc_nmsocket_t *sock) { /* further cleanup performed in isc__nm_tls_cleanup_data() */ atomic_store(&sock->closed, true); + atomic_store(&sock->active, false); sock->tlsstream.state = TLS_CLOSED; } @@ -919,7 +922,6 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { if (result != ISC_R_SUCCESS) { goto error; } - tlssock->peer = isc_nmhandle_peeraddr(handle); isc_nmhandle_attach(handle, &tlssock->outerhandle); atomic_store(&tlssock->active, true); From 22376fc69a3d2b0a475b53a612c8cca754e60390 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 30 Apr 2021 15:55:21 +0300 Subject: [PATCH 08/11] TLS: cancel reading on the underlying TCP socket after (see below) ... the last handle has been detached after calling write callback. That makes it possible to detach from the underlying socket and not to keep the socket object alive for too long. This issue was causing TLS tests with quota to fail because quota might not have been detached on time (because it was still referenced by the underlying TCP socket). One could say that this commit is an ideological continuation of: 513cdb52ecd4e63566672217f7390574f68c4d2d. --- lib/isc/netmgr/tlsstream.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index e738d555fb..264ffcde41 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -113,8 +113,14 @@ tls_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) { send_req->tlssock = NULL; if (send_req->cb != NULL) { + INSIST(VALID_NMHANDLE(tlssock->statichandle)); send_req->cb(send_req->handle, eresult, send_req->cbarg); isc_nmhandle_detach(&send_req->handle); + /* The last handle has been just detached: close the underlying + * socket. */ + if (tlssock->statichandle == NULL) { + finish = true; + } } isc_mem_put(handle->sock->mgr->mctx, send_req->data.base, From cd178043d9f10a159efd8ad4ae839c5b22fa6cc0 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 30 Apr 2021 16:06:09 +0300 Subject: [PATCH 09/11] Make some TLS tests actually use quota A directive to check quota was missing from some of the TLS tests which were supposed to test TLS code with quotas. --- lib/isc/tests/netmgr_test.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/isc/tests/netmgr_test.c b/lib/isc/tests/netmgr_test.c index f167e7433f..d062dbb799 100644 --- a/lib/isc/tests/netmgr_test.c +++ b/lib/isc/tests/netmgr_test.c @@ -2275,6 +2275,7 @@ static void tls_half_recv_send_quota(void **state) { SKIP_IN_CI; stream_use_TLS = true; + atomic_store(&check_listener_quota, true); stream_half_recv_send(state); } @@ -2282,6 +2283,7 @@ static void tls_half_recv_half_send_quota(void **state) { SKIP_IN_CI; stream_use_TLS = true; + atomic_store(&check_listener_quota, true); stream_half_recv_half_send(state); } From a9e97f28b77340d4fb14e22cd5ad3088ecec8022 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 6 May 2021 15:14:04 +0300 Subject: [PATCH 10/11] Fix crash in client side DoH code This commit fixes a situation when a cstream object could get unlinked from the list as a result of a cstream->read_cb call. Thus, unlinking it after the call could crash the program. --- lib/isc/netmgr/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index c2e403a5d0..9dc5e4d5c3 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -528,10 +528,10 @@ call_unlink_cstream_readcb(http_cstream_t *cstream, isc_result_t result) { REQUIRE(VALID_HTTP2_SESSION(session)); REQUIRE(cstream != NULL); + ISC_LIST_UNLINK(session->cstreams, cstream, link); cstream->read_cb(session->handle, result, &(isc_region_t){ cstream->rbuf, cstream->rbufsize }, cstream->read_cbarg); - ISC_LIST_UNLINK(session->cstreams, cstream, link); put_http_cstream(session->mctx, cstream); } From 8c0ea01f34cebd941bb1440bae238ae970395ead Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 6 May 2021 16:44:09 +0300 Subject: [PATCH 11/11] DoH: close active server streams when finishing session Under some circumstances a situation might occur when server-side session gets finished while there are still active HTTP/2 streams. This would lead to isc_nm_httpsocket object leaks. This commit fixes this behaviour as well as refactors failed_read_cb() to allow better code reuse. --- lib/isc/netmgr/http.c | 118 ++++++++++++++++++++++-------------------- 1 file changed, 63 insertions(+), 55 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 9dc5e4d5c3..4482e2023d 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -165,6 +165,12 @@ static void failed_httpstream_read_cb(isc_nmsocket_t *sock, isc_result_t result, isc_nm_http_session_t *session); +static void +client_call_failed_read_cb(isc_result_t result, isc_nm_http_session_t *session); + +static void +server_call_failed_read_cb(isc_result_t result, isc_nm_http_session_t *session); + static void failed_read_cb(isc_result_t result, isc_nm_http_session_t *session); @@ -420,23 +426,10 @@ finish_http_session(isc_nm_http_session_t *session) { isc_nm_cancelread(session->handle); } - if (!ISC_LIST_EMPTY(session->cstreams)) { - http_cstream_t *cstream = - ISC_LIST_HEAD(session->cstreams); - while (cstream != NULL) { - http_cstream_t *next = ISC_LIST_NEXT(cstream, - link); - ISC_LIST_DEQUEUE(session->cstreams, cstream, - link); - cstream->read_cb( - session->client_httphandle, - ISC_R_UNEXPECTED, - &(isc_region_t){ cstream->rbuf, - cstream->rbufsize }, - cstream->read_cbarg); - put_http_cstream(session->mctx, cstream); - cstream = next; - } + if (session->client) { + client_call_failed_read_cb(ISC_R_UNEXPECTED, session); + } else { + server_call_failed_read_cb(ISC_R_UNEXPECTED, session); } isc_nmhandle_detach(&session->handle); } @@ -2401,29 +2394,63 @@ failed_httpstream_read_cb(isc_nmsocket_t *sock, isc_result_t result, } static void -failed_read_cb(isc_result_t result, isc_nm_http_session_t *session) { +client_call_failed_read_cb(isc_result_t result, + isc_nm_http_session_t *session) { + http_cstream_t *cstream = NULL; + REQUIRE(VALID_HTTP2_SESSION(session)); REQUIRE(result != ISC_R_SUCCESS); - if (session->client) { - http_cstream_t *cstream = NULL; - cstream = ISC_LIST_HEAD(session->cstreams); - while (cstream != NULL) { - http_cstream_t *next = ISC_LIST_NEXT(cstream, link); - cstream->read_cb(session->client_httphandle, result, - &(isc_region_t){ cstream->rbuf, - cstream->rbufsize }, - cstream->read_cbarg); - if (result != ISC_R_TIMEDOUT || - !isc__nmsocket_timer_running(session->handle->sock)) - { - ISC_LIST_DEQUEUE(session->cstreams, cstream, - link); - put_http_cstream(session->mctx, cstream); - } - cstream = next; + cstream = ISC_LIST_HEAD(session->cstreams); + while (cstream != NULL) { + http_cstream_t *next = ISC_LIST_NEXT(cstream, link); + cstream->read_cb( + session->client_httphandle, result, + &(isc_region_t){ cstream->rbuf, cstream->rbufsize }, + cstream->read_cbarg); + + if (result != ISC_R_TIMEDOUT || + !isc__nmsocket_timer_running(session->handle->sock)) + { + ISC_LIST_DEQUEUE(session->cstreams, cstream, link); + put_http_cstream(session->mctx, cstream); } + cstream = next; + } +} + +static void +server_call_failed_read_cb(isc_result_t result, + isc_nm_http_session_t *session) { + isc_nmsocket_h2_t *h2data = NULL; /* stream socket */ + + REQUIRE(VALID_HTTP2_SESSION(session)); + REQUIRE(result != ISC_R_SUCCESS); + + for (h2data = ISC_LIST_HEAD(session->sstreams); h2data != NULL; + h2data = ISC_LIST_NEXT(h2data, link)) + { + failed_httpstream_read_cb(h2data->psock, result, session); + } + + h2data = ISC_LIST_HEAD(session->sstreams); + while (h2data != NULL) { + isc_nmsocket_h2_t *next = ISC_LIST_NEXT(h2data, link); + ISC_LIST_DEQUEUE(session->sstreams, h2data, link); + /* Cleanup socket in place */ + atomic_store(&h2data->psock->active, false); + atomic_store(&h2data->psock->closed, true); + isc__nmsocket_detach(&h2data->psock); + + h2data = next; + } +} + +static void +failed_read_cb(isc_result_t result, isc_nm_http_session_t *session) { + if (session->client) { + client_call_failed_read_cb(result, session); /* * If result was ISC_R_TIMEDOUT and the timer was reset, * then we still have active streams and should not close @@ -2433,26 +2460,7 @@ failed_read_cb(isc_result_t result, isc_nm_http_session_t *session) { finish_http_session(session); } } else { - isc_nmsocket_h2_t *h2data = NULL; /* stream socket */ - for (h2data = ISC_LIST_HEAD(session->sstreams); h2data != NULL; - h2data = ISC_LIST_NEXT(h2data, link)) - { - failed_httpstream_read_cb(h2data->psock, result, - session); - } - - h2data = ISC_LIST_HEAD(session->sstreams); - while (h2data != NULL) { - isc_nmsocket_h2_t *next = ISC_LIST_NEXT(h2data, link); - ISC_LIST_DEQUEUE(session->sstreams, h2data, link); - /* Cleanup socket in place */ - atomic_store(&h2data->psock->active, false); - atomic_store(&h2data->psock->closed, true); - isc__nmsocket_detach(&h2data->psock); - - h2data = next; - } - + server_call_failed_read_cb(result, session); /* * All streams are now destroyed; close the session. */