From 1f41d59a5e2ef50fca9c82f4195b4ce56bb7f0ae Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 20 Apr 2021 17:13:15 -0700 Subject: [PATCH 01/11] allow client read callback to be assignable allow netmgr client tests to choose the function that will be used as a read callback, without having to write a different connect callback handler. --- lib/isc/tests/netmgr_test.c | 72 ++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 38 deletions(-) diff --git a/lib/isc/tests/netmgr_test.c b/lib/isc/tests/netmgr_test.c index 805f7a6a58..793a6b04f3 100644 --- a/lib/isc/tests/netmgr_test.c +++ b/lib/isc/tests/netmgr_test.c @@ -41,6 +41,12 @@ typedef void (*stream_connect_function)(isc_nm_t *nm); +static void +connect_connect_cb(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg); +static void +connect_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *cbarg); + isc_nm_t *listen_nm = NULL; isc_nm_t *connect_nm = NULL; @@ -89,6 +95,8 @@ static bool skip_long_tests = false; static bool allow_send_back = false; static bool stream_use_TLS = false; +static isc_nm_recv_cb_t connect_readcb = NULL; + #define SKIP_IN_CI \ if (skip_long_tests) { \ skip(); \ @@ -334,6 +342,8 @@ nm_setup(void **state __attribute__((unused))) { isc_quota_init(&listener_quota, 0); atomic_store(&check_listener_quota, false); + connect_readcb = connect_read_cb; + return (0); } @@ -382,15 +392,6 @@ noop_accept_cb(isc_nmhandle_t *handle, unsigned int result, void *cbarg) { return (0); } -static void -noop_connect_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { - UNUSED(handle); - UNUSED(result); - UNUSED(cbarg); - - isc_refcount_decrement(&active_cconnects); -} - static void connect_send_cb(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg); @@ -476,7 +477,7 @@ connect_connect_cb(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) { isc_refcount_decrement(&active_cconnects); - if (eresult != ISC_R_SUCCESS) { + if (eresult != ISC_R_SUCCESS || connect_readcb == NULL) { return; } @@ -484,7 +485,7 @@ connect_connect_cb(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) { isc_refcount_increment0(&active_creads); isc_nmhandle_attach(handle, &readhandle); - isc_nm_read(handle, connect_read_cb, NULL); + isc_nm_read(handle, connect_readcb, NULL); connect_send(handle); } @@ -538,6 +539,7 @@ listen_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, listen_send_cb, cbarg); return; } + /* close the connection on stop_magic */ unref: if (handle == cbarg) { @@ -662,9 +664,10 @@ static void mock_udpconnect_uv_udp_open(void **state __attribute__((unused))) { WILL_RETURN(uv_udp_open, UV_ENOMEM); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, - (isc_nmiface_t *)&udp_listen_addr, noop_connect_cb, + (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); @@ -675,9 +678,10 @@ static void mock_udpconnect_uv_udp_bind(void **state __attribute__((unused))) { WILL_RETURN(uv_udp_bind, UV_ENOMEM); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, - (isc_nmiface_t *)&udp_listen_addr, noop_connect_cb, + (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); @@ -689,9 +693,10 @@ static void mock_udpconnect_uv_udp_connect(void **state __attribute__((unused))) { WILL_RETURN(uv_udp_connect, UV_ENOMEM); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, - (isc_nmiface_t *)&udp_listen_addr, noop_connect_cb, + (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); @@ -703,9 +708,10 @@ static void mock_udpconnect_uv_recv_buffer_size(void **state __attribute__((unused))) { WILL_RETURN(uv_recv_buffer_size, UV_ENOMEM); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, - (isc_nmiface_t *)&udp_listen_addr, noop_connect_cb, + (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); @@ -716,9 +722,10 @@ static void mock_udpconnect_uv_send_buffer_size(void **state __attribute__((unused))) { WILL_RETURN(uv_send_buffer_size, UV_ENOMEM); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, - (isc_nmiface_t *)&udp_listen_addr, noop_connect_cb, + (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); @@ -738,9 +745,10 @@ udp_noop(void **state __attribute__((unused))) { isc_nmsocket_close(&listen_sock); assert_null(listen_sock); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, - (isc_nmiface_t *)&udp_listen_addr, noop_connect_cb, + (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); @@ -1138,8 +1146,9 @@ stream_noop(void **state __attribute__((unused))) { isc_nmsocket_close(&listen_sock); assert_null(listen_sock); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); - stream_connect(noop_connect_cb, NULL, T_CONNECT, 0); + stream_connect(connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); atomic_assert_int_eq(cconnects, 0); @@ -1641,10 +1650,11 @@ tcpdns_noop(void **state __attribute__((unused))) { isc_nmsocket_close(&listen_sock); assert_null(listen_sock); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_tcpdnsconnect(connect_nm, (isc_nmiface_t *)&tcp_connect_addr, - (isc_nmiface_t *)&tcp_listen_addr, noop_connect_cb, - NULL, T_CONNECT, 0); + (isc_nmiface_t *)&tcp_listen_addr, + connect_connect_cb, NULL, T_CONNECT, 0); isc_nm_closedown(connect_nm); atomic_assert_int_eq(cconnects, 0); @@ -2175,10 +2185,12 @@ tlsdns_noop(void **state __attribute__((unused))) { isc_nmsocket_close(&listen_sock); assert_null(listen_sock); + connect_readcb = NULL; isc_refcount_increment0(&active_cconnects); isc_nm_tlsdnsconnect(connect_nm, (isc_nmiface_t *)&tcp_connect_addr, - (isc_nmiface_t *)&tcp_listen_addr, noop_connect_cb, - NULL, T_CONNECT, 0, tcp_connect_tlsctx); + (isc_nmiface_t *)&tcp_listen_addr, + connect_connect_cb, NULL, T_CONNECT, 0, + tcp_connect_tlsctx); isc_nm_closedown(connect_nm); @@ -2564,22 +2576,6 @@ main(void) { nm_setup, nm_teardown), /* TCP */ - /* cmocka_unit_test_setup_teardown(mock_listentcp_uv_tcp_bind, - */ - /* nm_setup, nm_teardown), */ - /* cmocka_unit_test_setup_teardown(mock_listentcp_uv_fileno, */ - /* nm_setup, nm_teardown), */ - /* cmocka_unit_test_setup_teardown( */ - /* mock_listentcp_uv_tcp_getsockname, nm_setup, */ - /* nm_teardown), */ - /* cmocka_unit_test_setup_teardown(mock_listentcp_uv_listen, */ - /* nm_setup, nm_teardown), */ - /* cmocka_unit_test_setup_teardown(mock_tcpconnect_uv_tcp_bind, - */ - /* nm_setup, nm_teardown), */ - /* cmocka_unit_test_setup_teardown(mock_tcpconnect_uv_tcp_connect, - */ - /* nm_setup, nm_teardown), */ cmocka_unit_test_setup_teardown(tcp_noop, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(tcp_noresponse, nm_setup, From 609975ad201908e3abe213f21be3a15e2ffe3bbf Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 20 Apr 2021 21:08:10 -0700 Subject: [PATCH 02/11] add a UDP timeout recovery test this test sets up a server socket that listens for UDP connections but never responds. the client will always time out; it should retry five times before giving up. --- lib/isc/tests/netmgr_test.c | 79 +++++++++++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 7 deletions(-) diff --git a/lib/isc/tests/netmgr_test.c b/lib/isc/tests/netmgr_test.c index 793a6b04f3..55b7d3ef37 100644 --- a/lib/isc/tests/netmgr_test.c +++ b/lib/isc/tests/netmgr_test.c @@ -73,13 +73,14 @@ static unsigned int workers = 0; static atomic_int_fast64_t nsends; static int_fast64_t esends; /* expected sends */ -static atomic_int_fast64_t ssends; -static atomic_int_fast64_t sreads; -static atomic_int_fast64_t saccepts; +static atomic_int_fast64_t ssends = ATOMIC_VAR_INIT(0); +static atomic_int_fast64_t sreads = ATOMIC_VAR_INIT(0); +static atomic_int_fast64_t saccepts = ATOMIC_VAR_INIT(0); -static atomic_int_fast64_t cconnects; -static atomic_int_fast64_t csends; -static atomic_int_fast64_t creads; +static atomic_int_fast64_t cconnects = ATOMIC_VAR_INIT(0); +static atomic_int_fast64_t csends = ATOMIC_VAR_INIT(0); +static atomic_int_fast64_t creads = ATOMIC_VAR_INIT(0); +static atomic_int_fast64_t ctimeouts = ATOMIC_VAR_INIT(0); static isc_refcount_t active_cconnects; static isc_refcount_t active_csends; @@ -314,6 +315,7 @@ nm_setup(void **state __attribute__((unused))) { atomic_store(&cconnects, 0); atomic_store(&csends, 0); atomic_store(&creads, 0); + atomic_store(&ctimeouts, 0); allow_send_back = false; stream_use_TLS = false; @@ -463,7 +465,7 @@ connect_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, } unref: - atomic_fetch_sub(&active_creads, 1); + isc_refcount_decrement(&active_creads); isc_nmhandle_detach(&handle); } @@ -794,6 +796,67 @@ udp_noresponse(void **state __attribute__((unused))) { atomic_assert_int_eq(ssends, 0); } +static void +timeout_retry_cb(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *cbarg) { + UNUSED(region); + UNUSED(cbarg); + + assert_non_null(handle); + + F(); + + if (eresult == ISC_R_TIMEDOUT && atomic_load(&csends) < 5) { + isc_nmhandle_settimeout(handle, 50); + connect_send(handle); + return; + } + + atomic_fetch_add(&ctimeouts, 1); + + isc_refcount_decrement(&active_creads); + isc_nmhandle_detach(&handle); +} + +static void +udp_timeout_recovery(void **state __attribute__((unused))) { + isc_result_t result = ISC_R_SUCCESS; + isc_nmsocket_t *listen_sock = NULL; + + SKIP_IN_CI; + + /* + * Listen using the noop callback so that client reads will time out. + */ + result = isc_nm_listenudp(listen_nm, (isc_nmiface_t *)&udp_listen_addr, + noop_recv_cb, NULL, 0, &listen_sock); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Connect with client timeout set to 0.05 seconds, then sleep for at + * least a second for each 'tick'. timeout_retry_cb() will give up + * after five timeouts. + */ + connect_readcb = timeout_retry_cb; + isc_refcount_increment0(&active_cconnects); + isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, + (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, + NULL, 50, 0); + + WAIT_FOR_EQ(cconnects, 1); + WAIT_FOR_GE(csends, 1); + WAIT_FOR_GE(csends, 2); + WAIT_FOR_GE(csends, 3); + WAIT_FOR_GE(csends, 4); + WAIT_FOR_EQ(csends, 5); + WAIT_FOR_EQ(ctimeouts, 1); + + isc_nm_stoplistening(listen_sock); + isc_nmsocket_close(&listen_sock); + assert_null(listen_sock); + isc_nm_closedown(connect_nm); +} + static void udp_recv_one(void **state __attribute__((unused))) { isc_result_t result = ISC_R_SUCCESS; @@ -2562,6 +2625,8 @@ main(void) { nm_teardown), cmocka_unit_test_setup_teardown(udp_noresponse, nm_setup, nm_teardown), + cmocka_unit_test_setup_teardown(udp_timeout_recovery, nm_setup, + nm_teardown), cmocka_unit_test_setup_teardown(udp_recv_one, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(udp_recv_two, nm_setup, From bcf5b2a67523bf0f6c1394d241c3d92128137373 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 20 Apr 2021 23:27:51 -0700 Subject: [PATCH 03/11] run read callbacks synchronously on timeout when running read callbacks, if the event result is not ISC_R_SUCCESS, the callback is always run asynchronously. this is a problem on timeout, because there's no chance to reset the timer before the socket has already been destroyed. this commit allows read callbacks to run synchronously for both ISC_R_SUCCESS and ISC_R_TIMEDOUT result codes. --- lib/isc/netmgr/netmgr.c | 2 +- lib/isc/netmgr/tcp.c | 8 +++++--- lib/isc/netmgr/tcpdns.c | 8 +++++--- lib/isc/netmgr/tlsdns.c | 8 +++++--- lib/isc/netmgr/udp.c | 1 + 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index c2891f1a63..5052bd00f6 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -2550,7 +2550,7 @@ isc__nm_readcb(isc_nmsocket_t *sock, isc__nm_uvreq_t *uvreq, REQUIRE(VALID_UVREQ(uvreq)); REQUIRE(VALID_NMHANDLE(uvreq->handle)); - if (eresult == ISC_R_SUCCESS) { + if (eresult == ISC_R_SUCCESS || eresult == ISC_R_TIMEDOUT) { isc__netievent_readcb_t ievent = { .sock = sock, .req = uvreq, .result = eresult }; diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index d86e1866e5..f950ba2646 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -676,9 +676,11 @@ failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) { destroy: isc__nmsocket_prep_destroy(sock); - /* We need to detach from quota after the read callback function had a - * chance to be executed. */ - if (sock->quota) { + /* + * We need to detach from quota after the read callback function had a + * chance to be executed. + */ + if (sock->quota != NULL) { isc_quota_detach(&sock->quota); } } diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index f8005bb4d9..9d2601b906 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -651,9 +651,11 @@ isc__nm_tcpdns_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) { destroy: isc__nmsocket_prep_destroy(sock); - /* We need to detach from quota after the read callback function had a - * chance to be executed. */ - if (sock->quota) { + /* + * We need to detach from quota after the read callback function had a + * chance to be executed. + */ + if (sock->quota != NULL) { isc_quota_detach(&sock->quota); } } diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index ff83fa4624..bb7ea76a3c 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -802,9 +802,11 @@ isc__nm_tlsdns_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result, destroy: isc__nmsocket_prep_destroy(sock); - /* We need to detach from quota after the read callback function - * had a chance to be executed. */ - if (sock->quota) { + /* + * We need to detach from quota after the read callback function + * had a chance to be executed. + */ + if (sock->quota != NULL) { isc_quota_detach(&sock->quota); } } diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index eb57c954be..ef404d93da 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -815,6 +815,7 @@ isc__nm_udp_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) { REQUIRE(result != ISC_R_SUCCESS); if (atomic_load(&sock->client)) { + isc__nmsocket_timer_stop(sock); isc__nm_stop_reading(sock); if (!sock->recv_read) { From 52f256f9aebdc3b3f9c0581b08c76349cc052ea2 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 21 Apr 2021 01:09:30 -0700 Subject: [PATCH 04/11] add TCPDNS and TLSDNS timeout recovery tests this is similar in structure to the UDP timeout recovery test. this commit adds a new mechanism to the netmgr test allowing the listen socket to accept incoming TCP connections but never send a response. this forces the client to time out on read. --- lib/isc/tests/netmgr_test.c | 100 +++++++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 1 deletion(-) diff --git a/lib/isc/tests/netmgr_test.c b/lib/isc/tests/netmgr_test.c index 55b7d3ef37..d95a42ded5 100644 --- a/lib/isc/tests/netmgr_test.c +++ b/lib/isc/tests/netmgr_test.c @@ -94,6 +94,7 @@ static atomic_bool check_listener_quota; static bool skip_long_tests = false; static bool allow_send_back = false; +static bool noanswer = false; static bool stream_use_TLS = false; static isc_nm_recv_cb_t connect_readcb = NULL; @@ -345,6 +346,7 @@ nm_setup(void **state __attribute__((unused))) { atomic_store(&check_listener_quota, false); connect_readcb = connect_read_cb; + noanswer = false; return (0); } @@ -533,7 +535,7 @@ listen_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, memmove(&magic, region->base, sizeof(magic)); assert_true(magic == stop_magic || magic == send_magic); - if (magic == send_magic) { + if (magic == send_magic && !noanswer) { isc_nmhandle_t *sendhandle = NULL; isc_nmhandle_attach(handle, &sendhandle); isc_refcount_increment0(&active_ssends); @@ -1767,6 +1769,49 @@ tcpdns_noresponse(void **state __attribute__((unused))) { atomic_assert_int_eq(ssends, 0); } +static void +tcpdns_timeout_recovery(void **state __attribute__((unused))) { + isc_result_t result = ISC_R_SUCCESS; + isc_nmsocket_t *listen_sock = NULL; + + SKIP_IN_CI; + + /* + * Accept connections but don't send responses, forcing client + * reads to time out. + */ + noanswer = true; + result = isc_nm_listentcpdns( + listen_nm, (isc_nmiface_t *)&tcp_listen_addr, listen_read_cb, + NULL, listen_accept_cb, NULL, 0, 0, NULL, &listen_sock); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Shorten all the TCP client timeouts to 0.05 seconds, connect, + * then sleep for at least a second for each 'tick'. + * timeout_retry_cb() will give up after five timeouts. + */ + connect_readcb = timeout_retry_cb; + isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + isc_refcount_increment0(&active_cconnects); + isc_nm_tcpdnsconnect(connect_nm, (isc_nmiface_t *)&tcp_connect_addr, + (isc_nmiface_t *)&tcp_listen_addr, + connect_connect_cb, NULL, 50, 0); + + WAIT_FOR_EQ(cconnects, 1); + WAIT_FOR_GE(csends, 1); + WAIT_FOR_GE(csends, 2); + WAIT_FOR_GE(csends, 3); + WAIT_FOR_GE(csends, 4); + WAIT_FOR_EQ(csends, 5); + WAIT_FOR_EQ(ctimeouts, 1); + + isc_nm_stoplistening(listen_sock); + isc_nmsocket_close(&listen_sock); + assert_null(listen_sock); + isc_nm_closedown(connect_nm); +} + static void tcpdns_recv_one(void **state __attribute__((unused))) { isc_result_t result = ISC_R_SUCCESS; @@ -2306,6 +2351,55 @@ tlsdns_noresponse(void **state __attribute__((unused))) { atomic_assert_int_eq(ssends, 0); } +static void +tlsdns_timeout_recovery(void **state __attribute__((unused))) { + isc_result_t result = ISC_R_SUCCESS; + isc_nmsocket_t *listen_sock = NULL; + isc_sockaddr_t connect_addr; + + SKIP_IN_CI; + + connect_addr = (isc_sockaddr_t){ .length = 0 }; + isc_sockaddr_fromin6(&connect_addr, &in6addr_loopback, 0); + + /* + * Accept connections but don't send responses, forcing client + * reads to time out. + */ + noanswer = true; + result = isc_nm_listentlsdns( + listen_nm, (isc_nmiface_t *)&tcp_listen_addr, listen_read_cb, + NULL, listen_accept_cb, NULL, 0, 0, NULL, tcp_listen_tlsctx, + &listen_sock); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Shorten all the TCP client timeouts to 0.05 seconds, connect, + * then sleep for at least a second for each 'tick'. + * timeout_retry_cb() will give up after five timeouts. + */ + connect_readcb = timeout_retry_cb; + isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + isc_refcount_increment0(&active_cconnects); + isc_nm_tlsdnsconnect(connect_nm, (isc_nmiface_t *)&tcp_connect_addr, + (isc_nmiface_t *)&tcp_listen_addr, + connect_connect_cb, NULL, 50, 0, + tcp_connect_tlsctx); + + WAIT_FOR_EQ(cconnects, 1); + WAIT_FOR_GE(csends, 1); + WAIT_FOR_GE(csends, 2); + WAIT_FOR_GE(csends, 3); + WAIT_FOR_GE(csends, 4); + WAIT_FOR_EQ(csends, 5); + WAIT_FOR_EQ(ctimeouts, 1); + + isc_nm_stoplistening(listen_sock); + isc_nmsocket_close(&listen_sock); + assert_null(listen_sock); + isc_nm_closedown(connect_nm); +} + static void tlsdns_recv_one(void **state __attribute__((unused))) { isc_result_t result = ISC_R_SUCCESS; @@ -2701,6 +2795,8 @@ main(void) { nm_teardown), cmocka_unit_test_setup_teardown(tcpdns_noresponse, nm_setup, nm_teardown), + cmocka_unit_test_setup_teardown(tcpdns_timeout_recovery, + nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(tcpdns_recv_send, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(tcpdns_recv_half_send, nm_setup, @@ -2771,6 +2867,8 @@ main(void) { nm_teardown), cmocka_unit_test_setup_teardown(tlsdns_noresponse, nm_setup, nm_teardown), + cmocka_unit_test_setup_teardown(tlsdns_timeout_recovery, + nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(tlsdns_recv_send, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(tlsdns_recv_half_send, nm_setup, From 25ef0547a957f1d3a95cd99ffd75caef7c19fdd3 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 21 Apr 2021 10:27:09 -0700 Subject: [PATCH 05/11] add TCP and TLS timeout recovery tests NOTE: currently these tests fail --- lib/isc/tests/netmgr_test.c | 82 ++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 14 deletions(-) diff --git a/lib/isc/tests/netmgr_test.c b/lib/isc/tests/netmgr_test.c index d95a42ded5..f0c27714cd 100644 --- a/lib/isc/tests/netmgr_test.c +++ b/lib/isc/tests/netmgr_test.c @@ -453,7 +453,7 @@ connect_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, goto unref; } - assert_int_equal(region->length, sizeof(magic)); + assert_true(region->length >= sizeof(magic)); atomic_fetch_add(&creads, 1); @@ -530,21 +530,22 @@ listen_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, atomic_fetch_add(&sreads, 1); - assert_int_equal(region->length, sizeof(magic)); + assert_true(region->length >= sizeof(magic)); memmove(&magic, region->base, sizeof(magic)); assert_true(magic == stop_magic || magic == send_magic); - if (magic == send_magic && !noanswer) { - isc_nmhandle_t *sendhandle = NULL; - isc_nmhandle_attach(handle, &sendhandle); - isc_refcount_increment0(&active_ssends); - isc_nm_send(sendhandle, (isc_region_t *)&send_msg, - listen_send_cb, cbarg); + if (magic == send_magic) { + if (!noanswer) { + isc_nmhandle_t *sendhandle = NULL; + isc_nmhandle_attach(handle, &sendhandle); + isc_refcount_increment0(&active_ssends); + isc_nm_send(sendhandle, (isc_region_t *)&send_msg, + listen_send_cb, cbarg); + } return; } - /* close the connection on stop_magic */ unref: if (handle == cbarg) { isc_refcount_decrement(&active_sreads); @@ -1191,12 +1192,12 @@ stream_connect(isc_nm_cb_t cb, void *cbarg, unsigned int timeout, (isc_nmiface_t *)&tcp_connect_addr, (isc_nmiface_t *)&tcp_listen_addr, cb, cbarg, tcp_connect_tlsctx, timeout, extrahandlesize); - return; + } else { + isc_nm_tcpconnect(connect_nm, + (isc_nmiface_t *)&tcp_connect_addr, + (isc_nmiface_t *)&tcp_listen_addr, cb, cbarg, + timeout, extrahandlesize); } - - isc_nm_tcpconnect(connect_nm, (isc_nmiface_t *)&tcp_connect_addr, - (isc_nmiface_t *)&tcp_listen_addr, cb, cbarg, timeout, - extrahandlesize); } static void @@ -1255,6 +1256,44 @@ stream_noresponse(void **state __attribute__((unused))) { atomic_assert_int_eq(ssends, 0); } +static void +stream_timeout_recovery(void **state __attribute__((unused))) { + isc_result_t result = ISC_R_SUCCESS; + isc_nmsocket_t *listen_sock = NULL; + + SKIP_IN_CI; + + /* + * Accept connections but don't send responses, forcing client + * reads to time out. + */ + noanswer = true; + result = stream_listen(stream_accept_cb, NULL, 0, 0, NULL, + &listen_sock); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Shorten all the client timeouts to 0.05 seconds. + */ + isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + connect_readcb = timeout_retry_cb; + isc_refcount_increment0(&active_cconnects); + stream_connect(connect_connect_cb, NULL, 50, 0); + + WAIT_FOR_EQ(cconnects, 1); + WAIT_FOR_GE(csends, 1); + WAIT_FOR_GE(csends, 2); + WAIT_FOR_GE(csends, 3); + WAIT_FOR_GE(csends, 4); + WAIT_FOR_EQ(csends, 5); + WAIT_FOR_EQ(ctimeouts, 1); + + isc_nm_stoplistening(listen_sock); + isc_nmsocket_close(&listen_sock); + assert_null(listen_sock); + isc_nm_closedown(connect_nm); +} + static void stream_recv_one(void **state __attribute__((unused))) { isc_result_t result = ISC_R_SUCCESS; @@ -1560,6 +1599,11 @@ tcp_noresponse(void **state) { stream_noresponse(state); } +static void +tcp_timeout_recovery(void **state) { + stream_timeout_recovery(state); +} + static void tcp_recv_one(void **state) { stream_recv_one(state); @@ -2114,6 +2158,12 @@ tls_noresponse(void **state) { stream_noresponse(state); } +static void +tls_timeout_recovery(void **state) { + stream_use_TLS = true; + stream_timeout_recovery(state); +} + static void tls_recv_one(void **state) { stream_use_TLS = true; @@ -2739,6 +2789,8 @@ main(void) { nm_teardown), cmocka_unit_test_setup_teardown(tcp_noresponse, nm_setup, nm_teardown), + cmocka_unit_test_setup_teardown(tcp_timeout_recovery, nm_setup, + nm_teardown), cmocka_unit_test_setup_teardown(tcp_recv_one, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(tcp_recv_two, nm_setup, @@ -2811,6 +2863,8 @@ main(void) { nm_teardown), cmocka_unit_test_setup_teardown(tls_noresponse, nm_setup, nm_teardown), + cmocka_unit_test_setup_teardown(tls_timeout_recovery, nm_setup, + nm_teardown), cmocka_unit_test_setup_teardown(tls_recv_one, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(tls_recv_two, nm_setup, From c90da99180b2c62cd1b31271b62b2d31a7e70fd9 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 21 Apr 2021 10:27:46 -0700 Subject: [PATCH 06/11] fix TCP timeout recovery removed an unnecessary assert in the failed_read_cb() function. also renamed to isc__nm_tcp_failed_read_cb() to match the practice in other modules. --- lib/isc/netmgr/tcp.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index f950ba2646..80ce1193c2 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -654,10 +654,10 @@ isc__nm_async_tcpstop(isc__networker_t *worker, isc__netievent_t *ev0) { stop_tcp_parent(sock); } -static void -failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) { +void +isc__nm_tcp_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) { REQUIRE(VALID_NMSOCK(sock)); - REQUIRE(sock->statichandle != NULL); + REQUIRE(result != ISC_R_SUCCESS); isc__nmsocket_timer_stop(sock); isc__nm_stop_reading(sock); @@ -685,11 +685,6 @@ destroy: } } -void -isc__nm_tcp_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) { - failed_read_cb(sock, result); -} - static void failed_send_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, isc_result_t eresult) { @@ -752,7 +747,7 @@ isc__nm_async_tcpstartread(isc__networker_t *worker, isc__netievent_t *ev0) { if (isc__nmsocket_closing(sock)) { sock->reading = true; - failed_read_cb(sock, ISC_R_CANCELED); + isc__nm_tcp_failed_read_cb(sock, ISC_R_CANCELED); return; } @@ -762,11 +757,12 @@ isc__nm_async_tcpstartread(isc__networker_t *worker, isc__netievent_t *ev0) { void isc__nm_tcp_pauseread(isc_nmhandle_t *handle) { - REQUIRE(VALID_NMHANDLE(handle)); - REQUIRE(VALID_NMSOCK(handle->sock)); - isc__netievent_tcppauseread_t *ievent = NULL; - isc_nmsocket_t *sock = handle->sock; + isc_nmsocket_t *sock = NULL; + + REQUIRE(VALID_NMHANDLE(handle)); + + sock = handle->sock; REQUIRE(VALID_NMSOCK(sock)); @@ -814,7 +810,7 @@ isc__nm_tcp_resumeread(isc_nmhandle_t *handle) { if (!isc__nmsocket_active(sock)) { sock->reading = true; - failed_read_cb(sock, ISC_R_CANCELED); + isc__nm_tcp_failed_read_cb(sock, ISC_R_CANCELED); return; } @@ -840,7 +836,7 @@ isc__nm_tcp_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) { REQUIRE(buf != NULL); if (isc__nmsocket_closing(sock)) { - failed_read_cb(sock, ISC_R_CANCELED); + isc__nm_tcp_failed_read_cb(sock, ISC_R_CANCELED); goto free; } @@ -850,7 +846,7 @@ isc__nm_tcp_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) { sock->statsindex[STATID_RECVFAIL]); } - failed_read_cb(sock, isc__nm_uverr2result(nread)); + isc__nm_tcp_failed_read_cb(sock, isc__nm_uverr2result(nread)); goto free; } @@ -1339,7 +1335,7 @@ isc__nm_tcp_shutdown(isc_nmsocket_t *sock) { } if (sock->statichandle != NULL) { - failed_read_cb(sock, ISC_R_CANCELED); + isc__nm_tcp_failed_read_cb(sock, ISC_R_CANCELED); return; } @@ -1379,7 +1375,7 @@ isc__nm_async_tcpcancel(isc__networker_t *worker, isc__netievent_t *ev0) { uv_timer_stop(&sock->timer); - failed_read_cb(sock, ISC_R_EOF); + isc__nm_tcp_failed_read_cb(sock, ISC_R_EOF); } int_fast32_t From 23ec0112989861e675a46aaa4f9ad3e05e1f8217 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 21 Apr 2021 10:28:26 -0700 Subject: [PATCH 07/11] fix TLS timeout recovery the destruction of the socket in tls_failed_read_cb() needs to be conditional; if reached due to a timeout on a timer that has subsequently been reset, the socket must not be destroyed. --- lib/isc/netmgr/tlsstream.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index a68838a775..07f3d26427 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -168,7 +168,10 @@ tls_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) { static void tls_failed_read_cb(isc_nmsocket_t *sock, const isc_result_t result) { + bool destroy = true; + REQUIRE(VALID_NMSOCK(sock)); + REQUIRE(result != ISC_R_SUCCESS); if (!sock->tlsstream.server && (sock->tlsstream.state == TLS_INIT || @@ -188,14 +191,22 @@ tls_failed_read_cb(isc_nmsocket_t *sock, const isc_result_t result) { req = isc__nm_uvreq_get(sock->mgr, sock); req->cb.recv = sock->recv_cb; req->cbarg = sock->recv_cbarg; - req->handle = NULL; isc_nmhandle_attach(sock->statichandle, &req->handle); - isc__nmsocket_clearcb(sock); + if (result != ISC_R_TIMEDOUT) { + isc__nmsocket_clearcb(sock); + } isc__nm_readcb(sock, req, result); + if (result == ISC_R_TIMEDOUT && + isc__nmsocket_timer_running(sock->outerhandle->sock)) + { + destroy = false; + } } - isc__nmsocket_prep_destroy(sock); - isc__nmsocket_detach(&sock); + if (destroy) { + isc__nmsocket_prep_destroy(sock); + isc__nmsocket_detach(&sock); + } } static void @@ -520,6 +531,7 @@ tls_readcb(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, tls_failed_read_cb(tlssock, result); return; } + tls_do_bio(tlssock, region, NULL, false); } From b258df8562987f58db1363bcdedeae050c279338 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 21 Apr 2021 11:55:40 -0700 Subject: [PATCH 08/11] add HTTP timeout recovery test NOTE: this test currently fails --- lib/isc/tests/doh_test.c | 160 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 150 insertions(+), 10 deletions(-) diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index d27b1f7b9b..d5f4c83e34 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -55,19 +55,19 @@ 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 nsends; - -static atomic_uint_fast64_t ssends; -static atomic_uint_fast64_t sreads; - -static atomic_uint_fast64_t csends; -static atomic_uint_fast64_t creads; +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); +static atomic_int_fast64_t csends = ATOMIC_VAR_INIT(0); +static atomic_int_fast64_t creads = ATOMIC_VAR_INIT(0); +static atomic_int_fast64_t ctimeouts = ATOMIC_VAR_INIT(0); static atomic_bool was_error; static unsigned int workers = 0; static bool reuse_supported = true; +static bool noanswer = false; static atomic_bool POST = ATOMIC_VAR_INIT(true); @@ -292,12 +292,15 @@ nm_setup(void **state) { atomic_store(&creads, 0); atomic_store(&sreads, 0); atomic_store(&ssends, 0); + atomic_store(&ctimeouts, 0); atomic_store(&was_error, false); atomic_store(&POST, false); atomic_store(&use_TLS, false); + noanswer = false; + isc_nonce_buf(&send_magic, sizeof(send_magic)); isc_nonce_buf(&stop_magic, sizeof(stop_magic)); if (send_magic == stop_magic) { @@ -428,11 +431,16 @@ doh_receive_request_cb(isc_nmhandle_t *handle, isc_result_t eresult, tcp_buffer_length); if (magic == send_magic) { - isc_nm_send(handle, region, doh_reply_sent_cb, NULL); + if (!noanswer) { + isc_nm_send(handle, region, doh_reply_sent_cb, + NULL); + } return; } else if (magic == stop_magic) { - /* We are done, so we don't send anything back */ - /* There should be no more packets in the buffer */ + /* + * We are done, so we don't send anything back. + * There should be no more packets in the buffer. + */ assert_int_equal(tcp_buffer_length, 0); } } @@ -543,6 +551,134 @@ doh_noresponse_GET(void **state) { doh_noresponse(state); } +static void +timeout_query_sent_cb(isc_nmhandle_t *handle, isc_result_t eresult, + void *cbarg) { + UNUSED(eresult); + UNUSED(cbarg); + + assert_non_null(handle); + + if (eresult == ISC_R_SUCCESS) { + atomic_fetch_add(&csends, 1); + } + + isc_nmhandle_detach(&handle); +} + +static void +timeout_retry_cb(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *arg) { + UNUSED(region); + UNUSED(arg); + + assert_non_null(handle); + + atomic_fetch_add(&ctimeouts, 1); + + if (eresult == ISC_R_TIMEDOUT && atomic_load(&ctimeouts) < 5) { + isc_nmhandle_settimeout(handle, 50); + return; + } + + /* + * XXX: We should be attaching to a readhandle in + * timeout_request_cb() and detaching it here, but we + * don't do this because the handle passed to this function + * is for the tcpsocket, not the httpsocket. This is a + * bug. + */ + /* isc_nmhandle_detach(&handle); */ +} + +static void +timeout_request_cb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { + isc_nmhandle_t *sendhandle = NULL; + /* isc_nmhandle_t *readhandle = NULL; */ + + REQUIRE(VALID_NMHANDLE(handle)); + + if (result != ISC_R_SUCCESS) { + goto error; + } + + isc_nmhandle_attach(handle, &sendhandle); + isc_nm_send(handle, + &(isc_region_t){ .base = (uint8_t *)send_msg.base, + .length = send_msg.len }, + timeout_query_sent_cb, arg); + + /* isc_nmhandle_attach(handle, &readhandle); */ + isc_nm_read(handle, timeout_retry_cb, NULL); + return; + +error: + atomic_store(&was_error, true); +} + +static void +doh_timeout_recovery(void **state) { + isc_nm_t **nm = (isc_nm_t **)*state; + isc_nm_t *listen_nm = nm[0]; + isc_nm_t *connect_nm = nm[1]; + isc_result_t result = ISC_R_SUCCESS; + isc_nmsocket_t *listen_sock = NULL; + isc_tlsctx_t *ctx = atomic_load(&use_TLS) ? server_tlsctx : NULL; + char req_url[256]; + + result = isc_nm_listenhttp(listen_nm, (isc_nmiface_t *)&tcp_listen_addr, + 0, NULL, NULL, &listen_sock); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Accept connections but don't send responses, forcing client + * reads to time out. + */ + noanswer = true; + result = isc_nm_http_endpoint(listen_sock, DOH_PATH, + doh_receive_request_cb, NULL, 0); + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Shorten all the TCP client timeouts to 0.05 seconds. + * timeout_retry_cb() will give up after five timeouts. + */ + isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + sockaddr_to_url(&tcp_listen_addr, false, req_url, sizeof(req_url), + DOH_PATH); + isc_nm_httpconnect(connect_nm, NULL, (isc_nmiface_t *)&tcp_listen_addr, + req_url, atomic_load(&POST), timeout_request_cb, + NULL, ctx, 50, 0); + + /* + * Sleep until sends reaches 5. + */ + for (size_t i = 0; i < 1000; i++) { + if (atomic_load(&ctimeouts) == 5) { + break; + } + usleep(1000); + } + assert_true(atomic_load(&ctimeouts) == 5); + + isc_nm_stoplistening(listen_sock); + isc_nmsocket_close(&listen_sock); + assert_null(listen_sock); + isc_nm_closedown(connect_nm); +} + +static void +doh_timeout_recovery_POST(void **state) { + atomic_store(&POST, true); + doh_timeout_recovery(state); +} + +static void +doh_timeout_recovery_GET(void **state) { + atomic_store(&POST, false); + doh_timeout_recovery(state); +} + static void doh_receive_send_reply_cb(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, void *cbarg) { @@ -1636,6 +1772,10 @@ main(void) { nm_teardown), cmocka_unit_test_setup_teardown(doh_noresponse_GET, nm_setup, nm_teardown), + cmocka_unit_test_setup_teardown(doh_timeout_recovery_POST, + nm_setup, nm_teardown), + cmocka_unit_test_setup_teardown(doh_timeout_recovery_GET, + nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(doh_recv_one_POST, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(doh_recv_one_GET, nm_setup, From f0d75ee7c3f80cce08eafa0932d74f8fbecfb469 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 21 Apr 2021 12:03:10 -0700 Subject: [PATCH 09/11] fix DOH timeout recovery as with TLS, the destruction of a client stream on failed read needs to be conditional: if we reached failed_read_cb() as a result of a timeout on a timer which has subsequently been reset, the stream must not be closed. --- lib/isc/netmgr/http.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index f1595eee94..64fbc39ab1 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -867,7 +867,9 @@ http_readcb(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, UNUSED(handle); if (result != ISC_R_SUCCESS) { - session->reading = false; + if (result != ISC_R_TIMEDOUT) { + session->reading = false; + } failed_read_cb(result, session); return; } @@ -2352,20 +2354,35 @@ 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) { 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); - ISC_LIST_DEQUEUE(session->cstreams, cstream, link); cstream->read_cb(session->handle, result, &(isc_region_t){ cstream->rbuf, cstream->rbufsize }, cstream->read_cbarg); - put_http_cstream(session->mctx, cstream); + 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; } + + /* + * If result was ISC_R_TIMEDOUT and the timer was reset, + * then we still have active streams and should not close + * the session. + */ + if (ISC_LIST_EMPTY(session->cstreams)) { + finish_http_session(session); + } } else { isc_nmsocket_h2_t *h2data = NULL; /* stream socket */ for (h2data = ISC_LIST_HEAD(session->sstreams); h2data != NULL; @@ -2386,9 +2403,12 @@ failed_read_cb(isc_result_t result, isc_nm_http_session_t *session) { h2data = next; } - } - finish_http_session(session); + /* + * All streams are now destroyed; close the session. + */ + finish_http_session(session); + } } static const bool base64url_validation_table[256] = { From 7f367b0c7fa9b61b1873d0b4655d56b0ffdd17b7 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 21 Apr 2021 17:35:07 -0700 Subject: [PATCH 10/11] use the correct handle when calling the read callback when calling isc_nm_read() on an HTTP socket, the read callback was being run with the incorrect handle. this has been corrected. --- lib/isc/netmgr/http.c | 33 ++++++++++++++++++++++++++------- lib/isc/tests/doh_test.c | 13 +++---------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 64fbc39ab1..a77216000d 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -122,6 +122,7 @@ struct isc_nm_http_session { size_t nsstreams; isc_nmhandle_t *handle; + isc_nmhandle_t *client_httphandle; isc_nmsocket_t *serversocket; isc_nmiface_t server_iface; @@ -412,11 +413,13 @@ finish_http_session(isc_nm_http_session_t *session) { if (session->closed) { return; } + if (session->handle != NULL) { if (!session->closed) { session->closed = true; isc_nm_cancelread(session->handle); } + if (!ISC_LIST_EMPTY(session->cstreams)) { http_cstream_t *cstream = ISC_LIST_HEAD(session->cstreams); @@ -426,7 +429,8 @@ finish_http_session(isc_nm_http_session_t *session) { ISC_LIST_DEQUEUE(session->cstreams, cstream, link); cstream->read_cb( - session->handle, ISC_R_UNEXPECTED, + session->client_httphandle, + ISC_R_UNEXPECTED, &(isc_region_t){ cstream->rbuf, cstream->rbufsize }, cstream->read_cbarg); @@ -437,6 +441,10 @@ finish_http_session(isc_nm_http_session_t *session) { isc_nmhandle_detach(&session->handle); } + if (session->client_httphandle != NULL) { + isc_nmhandle_detach(&session->client_httphandle); + } + INSIST(ISC_LIST_EMPTY(session->cstreams)); /* detach from server socket */ @@ -1049,15 +1057,24 @@ get_http_cstream(isc_nmsocket_t *sock, http_cstream_t **streamp) { } static void -http_call_connect_cb(isc_nmsocket_t *sock, isc_result_t result) { +http_call_connect_cb(isc_nmsocket_t *sock, isc_nm_http_session_t *session, + isc_result_t result) { isc__nm_uvreq_t *req = NULL; + isc_nmhandle_t *httphandle = isc__nmhandle_get(sock, &sock->peer, + &sock->iface->addr); REQUIRE(sock->connect_cb != NULL); req = isc__nm_uvreq_get(sock->mgr, sock); req->cb.connect = sock->connect_cb; req->cbarg = sock->connect_cbarg; - req->handle = isc__nmhandle_get(sock, &sock->peer, &sock->iface->addr); + 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); @@ -1066,8 +1083,8 @@ http_call_connect_cb(isc_nmsocket_t *sock, isc_result_t result) { static void transport_connect_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmsocket_t *http_sock = (isc_nmsocket_t *)cbarg; - isc_nm_http_session_t *session = NULL; isc_nmsocket_t *transp_sock = NULL; + isc_nm_http_session_t *session = NULL; http_cstream_t *cstream = NULL; isc_mem_t *mctx = NULL; @@ -1134,13 +1151,15 @@ transport_connect_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { } http_transpost_tcp_nodelay(handle); - http_call_connect_cb(http_sock, result); + + http_call_connect_cb(http_sock, session, result); + http_do_bio(session, NULL, NULL, NULL); isc__nmsocket_detach(&http_sock); return; error: - http_call_connect_cb(http_sock, result); + http_call_connect_cb(http_sock, session, result); if (http_sock->h2.connect.uri != NULL) { isc_mem_free(mctx, http_sock->h2.connect.uri); @@ -2361,7 +2380,7 @@ failed_read_cb(isc_result_t result, isc_nm_http_session_t *session) { cstream = ISC_LIST_HEAD(session->cstreams); while (cstream != NULL) { http_cstream_t *next = ISC_LIST_NEXT(cstream, link); - cstream->read_cb(session->handle, result, + cstream->read_cb(session->client_httphandle, result, &(isc_region_t){ cstream->rbuf, cstream->rbufsize }, cstream->read_cbarg); diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index d5f4c83e34..ec7c8883c9 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -581,20 +581,13 @@ timeout_retry_cb(isc_nmhandle_t *handle, isc_result_t eresult, return; } - /* - * XXX: We should be attaching to a readhandle in - * timeout_request_cb() and detaching it here, but we - * don't do this because the handle passed to this function - * is for the tcpsocket, not the httpsocket. This is a - * bug. - */ - /* isc_nmhandle_detach(&handle); */ + isc_nmhandle_detach(&handle); } static void timeout_request_cb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isc_nmhandle_t *sendhandle = NULL; - /* isc_nmhandle_t *readhandle = NULL; */ + isc_nmhandle_t *readhandle = NULL; REQUIRE(VALID_NMHANDLE(handle)); @@ -608,7 +601,7 @@ timeout_request_cb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { .length = send_msg.len }, timeout_query_sent_cb, arg); - /* isc_nmhandle_attach(handle, &readhandle); */ + isc_nmhandle_attach(handle, &readhandle); isc_nm_read(handle, timeout_retry_cb, NULL); return; From 62033110b9f54b50cea97503fc8c67fed5a54a65 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 22 Apr 2021 13:29:34 +0300 Subject: [PATCH 11/11] Use a constant for timeouts in soft-timeout tests It makes it easier to change the value should the need arise. --- lib/isc/tests/doh_test.c | 9 ++++++--- lib/isc/tests/netmgr_test.c | 19 +++++++++++-------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index ec7c8883c9..71dedbad6a 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -77,6 +77,9 @@ static atomic_bool use_TLS = ATOMIC_VAR_INIT(false); static isc_tlsctx_t *server_tlsctx = NULL; static isc_tlsctx_t *client_tlsctx = NULL; +/* Timeout for soft-timeout tests (0.05 seconds) */ +#define T_SOFT 50 + #define NSENDS 100 #define NWRITES 10 @@ -577,7 +580,7 @@ timeout_retry_cb(isc_nmhandle_t *handle, isc_result_t eresult, atomic_fetch_add(&ctimeouts, 1); if (eresult == ISC_R_TIMEDOUT && atomic_load(&ctimeouts) < 5) { - isc_nmhandle_settimeout(handle, 50); + isc_nmhandle_settimeout(handle, T_SOFT); return; } @@ -636,12 +639,12 @@ doh_timeout_recovery(void **state) { * Shorten all the TCP client timeouts to 0.05 seconds. * timeout_retry_cb() will give up after five timeouts. */ - isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + isc_nm_settimeouts(connect_nm, T_SOFT, T_SOFT, T_SOFT, T_SOFT); sockaddr_to_url(&tcp_listen_addr, false, req_url, sizeof(req_url), DOH_PATH); isc_nm_httpconnect(connect_nm, NULL, (isc_nmiface_t *)&tcp_listen_addr, req_url, atomic_load(&POST), timeout_request_cb, - NULL, ctx, 50, 0); + NULL, ctx, T_SOFT, 0); /* * Sleep until sends reaches 5. diff --git a/lib/isc/tests/netmgr_test.c b/lib/isc/tests/netmgr_test.c index f0c27714cd..8f1ae9ece7 100644 --- a/lib/isc/tests/netmgr_test.c +++ b/lib/isc/tests/netmgr_test.c @@ -107,6 +107,9 @@ static isc_nm_recv_cb_t connect_readcb = NULL; #define NSENDS 100 +/* Timeout for soft-timeout tests (0.05 seconds) */ +#define T_SOFT 50 + /* Timeouts in miliseconds */ #define T_INIT 120 * 1000 #define T_IDLE 120 * 1000 @@ -810,7 +813,7 @@ timeout_retry_cb(isc_nmhandle_t *handle, isc_result_t eresult, F(); if (eresult == ISC_R_TIMEDOUT && atomic_load(&csends) < 5) { - isc_nmhandle_settimeout(handle, 50); + isc_nmhandle_settimeout(handle, T_SOFT); connect_send(handle); return; } @@ -844,7 +847,7 @@ udp_timeout_recovery(void **state __attribute__((unused))) { isc_refcount_increment0(&active_cconnects); isc_nm_udpconnect(connect_nm, (isc_nmiface_t *)&udp_connect_addr, (isc_nmiface_t *)&udp_listen_addr, connect_connect_cb, - NULL, 50, 0); + NULL, T_SOFT, 0); WAIT_FOR_EQ(cconnects, 1); WAIT_FOR_GE(csends, 1); @@ -1275,10 +1278,10 @@ stream_timeout_recovery(void **state __attribute__((unused))) { /* * Shorten all the client timeouts to 0.05 seconds. */ - isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + isc_nm_settimeouts(connect_nm, T_SOFT, T_SOFT, T_SOFT, T_SOFT); connect_readcb = timeout_retry_cb; isc_refcount_increment0(&active_cconnects); - stream_connect(connect_connect_cb, NULL, 50, 0); + stream_connect(connect_connect_cb, NULL, T_SOFT, 0); WAIT_FOR_EQ(cconnects, 1); WAIT_FOR_GE(csends, 1); @@ -1836,11 +1839,11 @@ tcpdns_timeout_recovery(void **state __attribute__((unused))) { * timeout_retry_cb() will give up after five timeouts. */ connect_readcb = timeout_retry_cb; - isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + isc_nm_settimeouts(connect_nm, T_SOFT, T_SOFT, T_SOFT, T_SOFT); isc_refcount_increment0(&active_cconnects); isc_nm_tcpdnsconnect(connect_nm, (isc_nmiface_t *)&tcp_connect_addr, (isc_nmiface_t *)&tcp_listen_addr, - connect_connect_cb, NULL, 50, 0); + connect_connect_cb, NULL, T_SOFT, 0); WAIT_FOR_EQ(cconnects, 1); WAIT_FOR_GE(csends, 1); @@ -2429,11 +2432,11 @@ tlsdns_timeout_recovery(void **state __attribute__((unused))) { * timeout_retry_cb() will give up after five timeouts. */ connect_readcb = timeout_retry_cb; - isc_nm_settimeouts(connect_nm, 50, 50, 50, 50); + isc_nm_settimeouts(connect_nm, T_SOFT, T_SOFT, T_SOFT, T_SOFT); isc_refcount_increment0(&active_cconnects); isc_nm_tlsdnsconnect(connect_nm, (isc_nmiface_t *)&tcp_connect_addr, (isc_nmiface_t *)&tcp_listen_addr, - connect_connect_cb, NULL, 50, 0, + connect_connect_cb, NULL, T_SOFT, 0, tcp_connect_tlsctx); WAIT_FOR_EQ(cconnects, 1);