From b6b7a6886a8ac66bc3932158740998a3bf2da014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 11 Oct 2022 09:10:16 +0200 Subject: [PATCH 1/3] Don't set load-balancing socket option on the UDP connect sockets The isc_nm_udpconnect() erroneously set the reuse port with load-balancing on the outgoing connected UDP sockets. This socket option makes only sense for the listening sockets. Don't set the load-balancing reuse port option on the outgoing UDP sockets. --- lib/isc/netmgr/udp.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index cd5513fc58..cda3d9d98c 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -846,10 +846,6 @@ isc_nm_udpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, RUNTIME_CHECK(result == ISC_R_SUCCESS || result == ISC_R_NOTIMPLEMENTED); - result = isc__nm_socket_reuse_lb(sock->fd); - RUNTIME_CHECK(result == ISC_R_SUCCESS || - result == ISC_R_NOTIMPLEMENTED); - (void)isc__nm_socket_incoming_cpu(sock->fd); (void)isc__nm_socket_disable_pmtud(sock->fd, sa_family); From af257140e69d5fc2d517f67eb503a8efa19823b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 11 Oct 2022 12:03:17 +0200 Subject: [PATCH 2/3] Make sure the unit test listening and connecting ports are different In rare circumstances, the UDP port for the listening socket and the UDP port for the connecting socket might be the same. Because we use the "reuse" port socket option, this isn't caught when binding the socket, and thus the connected client socket could send a datagram to itself, completely bypassing the server. This doesn't happen under normal operation mode because `named` is listening on a privileged port (53), and even if not, it doesn't usually talk to itself as the tests do. Pick an arbitrary port for listening (9153-9156) that is outside the ephemeral port range for the network manager related unit tests (except the `doh_test). --- tests/isc/netmgr_common.c | 53 ++------------------------------------- tests/isc/netmgr_common.h | 14 ++++++++--- tests/isc/tcp_test.c | 1 + tests/isc/tcpdns_test.c | 10 +++++++- tests/isc/tls_test.c | 1 + tests/isc/tlsdns_test.c | 10 +++++++- tests/isc/udp_test.c | 9 ++----- 7 files changed, 35 insertions(+), 63 deletions(-) diff --git a/tests/isc/netmgr_common.c b/tests/isc/netmgr_common.c index e92a59bad5..62c9ed74eb 100644 --- a/tests/isc/netmgr_common.c +++ b/tests/isc/netmgr_common.c @@ -95,69 +95,20 @@ bool allow_send_back = false; bool noanswer = false; bool stream_use_TLS = false; bool stream = false; +in_port_t stream_port = 0; isc_nm_recv_cb_t connect_readcb = NULL; -int -setup_ephemeral_port(isc_sockaddr_t *addr, sa_family_t family) { - socklen_t addrlen = sizeof(*addr); - uv_os_sock_t fd = -1; - int r; - - isc_sockaddr_fromin6(addr, &in6addr_loopback, 0); - - fd = socket(AF_INET6, family, 0); - if (fd < 0) { - perror("setup_ephemeral_port: socket()"); - return (-1); - } - - r = bind(fd, (const struct sockaddr *)&addr->type.sa, - sizeof(addr->type.sin6)); - if (r != 0) { - perror("setup_ephemeral_port: bind()"); - isc__nm_closesocket(fd); - return (r); - } - - r = getsockname(fd, (struct sockaddr *)&addr->type.sa, &addrlen); - if (r != 0) { - perror("setup_ephemeral_port: getsockname()"); - isc__nm_closesocket(fd); - return (r); - } - -#if IPV6_RECVERR -#define setsockopt_on(socket, level, name) \ - setsockopt(socket, level, name, &(int){ 1 }, sizeof(int)) - - r = setsockopt_on(fd, IPPROTO_IPV6, IPV6_RECVERR); - if (r != 0) { - perror("setup_ephemeral_port"); - isc__nm_closesocket(fd); - return (r); - } -#endif - - return (fd); -} - int setup_netmgr_test(void **state) { char *env_workers = getenv("ISC_TASK_WORKERS"); - uv_os_sock_t tcp_listen_sock = -1; size_t nworkers; tcp_connect_addr = (isc_sockaddr_t){ .length = 0 }; isc_sockaddr_fromin6(&tcp_connect_addr, &in6addr_loopback, 0); tcp_listen_addr = (isc_sockaddr_t){ .length = 0 }; - tcp_listen_sock = setup_ephemeral_port(&tcp_listen_addr, SOCK_STREAM); - if (tcp_listen_sock < 0) { - return (-1); - } - isc__nm_closesocket(tcp_listen_sock); - tcp_listen_sock = -1; + isc_sockaddr_fromin6(&tcp_listen_addr, &in6addr_loopback, stream_port); if (env_workers != NULL) { workers = atoi(env_workers); diff --git a/tests/isc/netmgr_common.h b/tests/isc/netmgr_common.h index 17eb27b041..31d63f3fc9 100644 --- a/tests/isc/netmgr_common.h +++ b/tests/isc/netmgr_common.h @@ -18,6 +18,16 @@ #include "netmgr/netmgr-int.h" +/* + * Pick unused port outside the ephemeral port range, so we don't clash with + * connected sockets. + */ +#define UDP_TEST_PORT 9153 +#define TCP_TEST_PORT 9154 +#define TLS_TEST_PORT 9155 +#define TCPDNS_TEST_PORT 9156 +#define TLSDNS_TEST_PORT 9157 + typedef void (*stream_connect_function)(isc_nm_t *nm); typedef void (*connect_func)(isc_nm_t *); @@ -119,6 +129,7 @@ extern bool allow_send_back; extern bool noanswer; extern bool stream_use_TLS; extern bool stream; +extern in_port_t stream_port; extern isc_nm_recv_cb_t connect_readcb; @@ -213,9 +224,6 @@ extern isc_nm_recv_cb_t connect_readcb; #define atomic_assert_int_ge(val, exp) assert_true(atomic_load(&val) >= exp) #define atomic_assert_int_gt(val, exp) assert_true(atomic_load(&val) > exp) -int -setup_ephemeral_port(isc_sockaddr_t *addr, sa_family_t family); - int setup_netmgr_test(void **state); int diff --git a/tests/isc/tcp_test.c b/tests/isc/tcp_test.c index de25a6be8e..b76394fa6f 100644 --- a/tests/isc/tcp_test.c +++ b/tests/isc/tcp_test.c @@ -132,6 +132,7 @@ ISC_TEST_LIST_END static int tcp_setup(void **state __attribute__((__unused__))) { + stream_port = TCP_TEST_PORT; stream_use_TLS = false; stream = true; diff --git a/tests/isc/tcpdns_test.c b/tests/isc/tcpdns_test.c index 691b246846..4a66629e0c 100644 --- a/tests/isc/tcpdns_test.c +++ b/tests/isc/tcpdns_test.c @@ -142,4 +142,12 @@ ISC_TEST_ENTRY_CUSTOM(tcpdns_recv_send, stream_recv_send_setup, stream_recv_send_teardown) ISC_TEST_LIST_END -ISC_TEST_MAIN + +static int +tcpdns_setup(void **state __attribute__((__unused__))) { + stream_port = TCPDNS_TEST_PORT; + + return (0); +} + +ISC_TEST_MAIN_CUSTOM(tcpdns_setup, NULL) diff --git a/tests/isc/tls_test.c b/tests/isc/tls_test.c index 211dd6bae4..e4d7f86ea4 100644 --- a/tests/isc/tls_test.c +++ b/tests/isc/tls_test.c @@ -131,6 +131,7 @@ ISC_TEST_LIST_END static int tls_setup(void **state __attribute__((__unused__))) { + stream_port = TLS_TEST_PORT; stream_use_TLS = true; stream = true; diff --git a/tests/isc/tlsdns_test.c b/tests/isc/tlsdns_test.c index f5033fb116..2dbc3d532e 100644 --- a/tests/isc/tlsdns_test.c +++ b/tests/isc/tlsdns_test.c @@ -147,4 +147,12 @@ ISC_TEST_ENTRY_CUSTOM(tlsdns_recv_send, stream_recv_send_setup, /* FIXME: Re-add the noalpn tests */ ISC_TEST_LIST_END -ISC_TEST_MAIN + +static int +tlsdns_setup(void **state __attribute__((__unused__))) { + stream_port = TCPDNS_TEST_PORT; + + return (0); +} + +ISC_TEST_MAIN_CUSTOM(tlsdns_setup, NULL) diff --git a/tests/isc/udp_test.c b/tests/isc/udp_test.c index 1006caa4d1..64066ec28f 100644 --- a/tests/isc/udp_test.c +++ b/tests/isc/udp_test.c @@ -56,8 +56,6 @@ static isc_sockaddr_t udp_connect_addr; static int setup_test(void **state) { - uv_os_sock_t udp_listen_sock = -1; - setup_loopmgr(state); setup_netmgr(state); @@ -65,11 +63,8 @@ setup_test(void **state) { isc_sockaddr_fromin6(&udp_connect_addr, &in6addr_loopback, 0); udp_listen_addr = (isc_sockaddr_t){ .length = 0 }; - udp_listen_sock = setup_ephemeral_port(&udp_listen_addr, SOCK_DGRAM); - if (udp_listen_sock < 0) { - return (-1); - } - isc__nm_closesocket(udp_listen_sock); + isc_sockaddr_fromin6(&udp_listen_addr, &in6addr_loopback, + UDP_TEST_PORT); atomic_store(&sreads, 0); atomic_store(&ssends, 0); From 076cdf7444d4cb840828c89444ee8256549b814a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 11 Oct 2022 12:29:48 +0200 Subject: [PATCH 3/3] Gracefully handle ISC_R_SHUTTINGDOWN in udp__send_cb The ISC_R_SHUTTINGDOWN should be handled the same as ISC_R_CANCELED in the udp__send_cb(), as we might be sending the data while the loopmgr/netmgr shutdown has been initiated. --- tests/isc/udp_test.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/isc/udp_test.c b/tests/isc/udp_test.c index 64066ec28f..81340a2174 100644 --- a/tests/isc/udp_test.c +++ b/tests/isc/udp_test.c @@ -774,6 +774,7 @@ udp__send_cb(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) { } } break; + case ISC_R_SHUTTINGDOWN: case ISC_R_CANCELED: break; default: