From 2846888c573fcc610cdf71bcdd5bb6f92ffaf499 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 24 Mar 2023 15:32:02 +0100 Subject: [PATCH] Attach the accept "client" socket to .listener member of the socket When accepting a TCP connection in the higher layers (tlsstream, streamdns, and http) attach to the socket the connection was accepted on, and use this socket instead of the parent listening socket. This has an advantage - accessing the sock->listener now doesn't break the thread boundaries, so we can properly check whether the socket is being closed without requiring .closing member to be atomic_bool. --- lib/isc/netmgr/http.c | 58 ++++++------------------------------- lib/isc/netmgr/netmgr-int.h | 8 ++--- lib/isc/netmgr/netmgr.c | 31 +++++--------------- lib/isc/netmgr/streamdns.c | 24 ++++++--------- lib/isc/netmgr/tcp.c | 3 -- lib/isc/netmgr/tlsstream.c | 54 +++++++++++++--------------------- lib/isc/netmgr/udp.c | 5 ---- 7 files changed, 49 insertions(+), 134 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 51ef77dbe4..10e07c8b7a 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1688,11 +1688,9 @@ find_server_request_handler(const char *request_path, REQUIRE(VALID_NMSOCK(serversocket)); - if (serversocket->listening) { - handler = http_endpoints_find( - request_path, - http_get_listener_endpoints(serversocket, tid)); - } + handler = http_endpoints_find( + request_path, http_get_listener_endpoints(serversocket, tid)); + return (handler); } @@ -2430,57 +2428,31 @@ http_transpost_tcp_nodelay(isc_nmhandle_t *transphandle) { static isc_result_t httplisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { - isc_nmsocket_t *httplistensock = (isc_nmsocket_t *)cbarg; + isc_nmsocket_t *httpserver = (isc_nmsocket_t *)cbarg; isc_nm_http_session_t *session = NULL; - isc_nmsocket_t *listener = NULL, *httpserver = NULL; REQUIRE(VALID_NMHANDLE(handle)); REQUIRE(VALID_NMSOCK(handle->sock)); - if (handle->sock->type == isc_nm_tlssocket) { - REQUIRE(VALID_NMSOCK(handle->sock->listener)); - listener = handle->sock->listener; - httpserver = listener->h2.httpserver; - } else { - REQUIRE(VALID_NMSOCK(handle->sock->server)); - listener = handle->sock->server; - REQUIRE(VALID_NMSOCK(listener->parent)); - httpserver = listener->parent->h2.httpserver; - } - - /* - * NOTE: HTTP listener socket might be destroyed by the time this - * function gets invoked, so we need to do extra sanity checks to - * detect this case. - */ if (isc__nm_closing(handle->sock->worker)) { return (ISC_R_SHUTTINGDOWN); - } else if (isc__nmsocket_closing(handle->sock) || httpserver == NULL) { - return (ISC_R_CANCELED); - } - - if (result != ISC_R_SUCCESS) { - /* XXXWPK do nothing? */ + } else if (result != ISC_R_SUCCESS) { return (result); } - REQUIRE(VALID_NMSOCK(httplistensock)); - INSIST(httplistensock == httpserver); - - if (httplistensock->closing) { - return (ISC_R_CANCELED); - } + REQUIRE(VALID_NMSOCK(httpserver)); + REQUIRE(httpserver->type == isc_nm_httplistener); http_transpost_tcp_nodelay(handle); new_session(handle->sock->worker->mctx, NULL, &session); session->max_concurrent_streams = - atomic_load_relaxed(&httplistensock->h2.max_concurrent_streams); + atomic_load_relaxed(&httpserver->h2.max_concurrent_streams); initialize_nghttp2_server_session(session); handle->sock->h2.session = session; isc_nmhandle_attach(handle, &session->handle); - isc__nmsocket_attach(httplistensock, &session->serversocket); + isc__nmsocket_attach(httpserver, &session->serversocket); server_send_connection_header(session); /* TODO H2 */ @@ -2528,14 +2500,9 @@ isc_nm_listenhttp(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, return (result); } - isc__nmsocket_attach(sock, &sock->outer->h2.httpserver); - sock->nchildren = sock->outer->nchildren; sock->fd = (uv_os_sock_t)-1; - isc__nmsocket_barrier_init(sock); - - sock->listening = true; *sockp = sock; return (ISC_R_SUCCESS); } @@ -3204,13 +3171,6 @@ isc__nm_http_initsocket(isc_nmsocket_t *sock) { void isc__nm_http_cleanup_data(isc_nmsocket_t *sock) { - if ((sock->type == isc_nm_tcplistener || - sock->type == isc_nm_tlslistener) && - sock->h2.httpserver != NULL) - { - isc__nmsocket_detach(&sock->h2.httpserver); - } - if (sock->type == isc_nm_httplistener || sock->type == isc_nm_httpsocket) { diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index d4028090db..a852fefb42 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -430,8 +430,6 @@ typedef struct isc_nmsocket_h2 { int32_t stream_id; isc_nm_http_session_t *session; - isc_nmsocket_t *httpserver; - /* maximum concurrent streams (server-side) */ atomic_uint_fast32_t max_concurrent_streams; @@ -486,8 +484,6 @@ struct isc_nmsocket { /*% Parent socket for multithreaded listeners */ isc_nmsocket_t *parent; - /*% Listener socket this connection was accepted on */ - isc_nmsocket_t *listener; /*% TLS stuff */ struct tlsstream { @@ -562,6 +558,9 @@ struct isc_nmsocket { /*% server socket for connections */ isc_nmsocket_t *server; + /*% client socket for connections */ + isc_nmsocket_t *listener; + /*% Child sockets for multi-socket setups */ isc_nmsocket_t *children; uint_fast32_t nchildren; @@ -597,7 +596,6 @@ struct isc_nmsocket { */ bool closing; bool closed; - bool listening; bool connecting; bool connected; bool accepting; diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 36972df7e6..162d5261dd 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -942,7 +942,7 @@ nmhandle_free(isc_nmsocket_t *sock, isc_nmhandle_t *handle) { handle->dofree(handle->opaque); } - isc_mem_put(sock->worker->mctx, handle, sizeof(isc_nmhandle_t)); + isc_mem_put(sock->worker->mctx, handle, sizeof(*handle)); } static void @@ -1438,7 +1438,7 @@ isc__nm_closing(isc__networker_t *worker) { bool isc__nmsocket_closing(isc_nmsocket_t *sock) { - return (!isc__nmsocket_active(sock) || sock->closing || + return (!sock->active || sock->closing || isc__nm_closing(sock->worker) || (sock->server != NULL && !isc__nmsocket_active(sock->server))); } @@ -1746,42 +1746,27 @@ isc_nm_stoplistening(isc_nmsocket_t *sock) { } } -static void -nmsocket_stop_cb(void *arg) { - isc_nmsocket_t *listener = arg; - - isc_barrier_wait(&listener->stop_barrier); -} - void isc__nmsocket_stop(isc_nmsocket_t *listener) { REQUIRE(VALID_NMSOCK(listener)); REQUIRE(listener->tid == isc_tid()); REQUIRE(listener->tid == 0); - REQUIRE(listener->listening); + REQUIRE(listener->type == isc_nm_httplistener || + listener->type == isc_nm_tlslistener || + listener->type == isc_nm_streamdnslistener); REQUIRE(!listener->closing); listener->closing = true; - for (size_t i = 1; i < listener->nchildren; i++) { - isc__networker_t *worker = - &listener->worker->netmgr->workers[i]; - isc_async_run(worker->loop, nmsocket_stop_cb, listener); - } - - nmsocket_stop_cb(listener); - - listener->listening = false; + REQUIRE(listener->outer != NULL); + isc_nm_stoplistening(listener->outer); listener->accept_cb = NULL; listener->accept_cbarg = NULL; listener->recv_cb = NULL; listener->recv_cbarg = NULL; - if (listener->outer != NULL) { - isc_nm_stoplistening(listener->outer); - isc__nmsocket_detach(&listener->outer); - } + isc__nmsocket_detach(&listener->outer); listener->closed = true; } diff --git a/lib/isc/netmgr/streamdns.c b/lib/isc/netmgr/streamdns.c index 374d9712de..fc996bd7a6 100644 --- a/lib/isc/netmgr/streamdns.c +++ b/lib/isc/netmgr/streamdns.c @@ -642,25 +642,21 @@ streamdns_accept_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmsocket_t *listensock = (isc_nmsocket_t *)cbarg; isc_nmsocket_t *nsock; isc_sockaddr_t iface; - int tid; + int tid = isc_tid(); uint32_t initial = 0; - if (result != ISC_R_SUCCESS) { - return (result); - } - - INSIST(VALID_NMHANDLE(handle)); - INSIST(VALID_NMSOCK(handle->sock)); - INSIST(VALID_NMSOCK(listensock)); - INSIST(listensock->type == isc_nm_streamdnslistener); + REQUIRE(VALID_NMHANDLE(handle)); + REQUIRE(VALID_NMSOCK(handle->sock)); if (isc__nm_closing(handle->sock->worker)) { return (ISC_R_SHUTTINGDOWN); - } else if (isc__nmsocket_closing(handle->sock) || listensock->closing) { - return (ISC_R_CANCELED); + } else if (result != ISC_R_SUCCESS) { + return (result); } - tid = isc_tid(); + REQUIRE(VALID_NMSOCK(listensock)); + REQUIRE(listensock->type == isc_nm_streamdnslistener); + iface = isc_nmhandle_localaddr(handle); nsock = streamdns_sock_new(handle->sock->worker, isc_nm_streamdnssocket, &iface, true); @@ -675,7 +671,7 @@ streamdns_accept_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { nsock->accepting = true; nsock->active = true; - isc__nmsocket_attach(listensock, &nsock->listener); + isc__nmsocket_attach(handle->sock, &nsock->listener); isc_nmhandle_attach(handle, &nsock->outerhandle); handle->sock->streamdns.sock = nsock; @@ -753,10 +749,8 @@ isc_nm_listenstreamdns(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, listener->result = result; listener->active = true; - listener->listening = true; INSIST(listener->outer->streamdns.listener == NULL); listener->nchildren = listener->outer->nchildren; - isc__nmsocket_barrier_init(listener); isc__nmsocket_attach(listener, &listener->outer->streamdns.listener); *sockp = listener; diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 87949791b5..7163595399 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -415,8 +415,6 @@ start_tcp_child_job(void *arg) { goto done; } - sock->listening = true; - if (sock->tid == 0) { r = uv_tcp_getsockname(&sock->uv_handle.tcp, (struct sockaddr *)&ss, @@ -675,7 +673,6 @@ tcp_stop_cb(uv_handle_t *handle) { REQUIRE(!sock->closed); sock->closed = true; - sock->listening = false; isc__nm_incstats(sock, STATID_CLOSE); diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 75481e9d89..6432e493ab 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -111,8 +111,6 @@ inactive(isc_nmsocket_t *sock) { sock->outerhandle == NULL || !isc__nmsocket_active(sock->outerhandle->sock) || sock->outerhandle->sock->closing || - (sock->listener != NULL && - !isc__nmsocket_active(sock->listener)) || isc__nm_closing(sock->worker)); } @@ -384,53 +382,42 @@ tls_process_outgoing(isc_nmsocket_t *sock, bool finish, static int tls_try_handshake(isc_nmsocket_t *sock, isc_result_t *presult) { - int rv = 0; - isc_nmhandle_t *tlshandle = NULL; - REQUIRE(sock->tlsstream.state == TLS_HANDSHAKE); if (SSL_is_init_finished(sock->tlsstream.tls) == 1) { return (0); } - rv = SSL_do_handshake(sock->tlsstream.tls); + int rv = SSL_do_handshake(sock->tlsstream.tls); if (rv == 1) { + isc_nmhandle_t *tlshandle = NULL; isc_result_t result = ISC_R_SUCCESS; + + REQUIRE(sock->statichandle == NULL); INSIST(SSL_is_init_finished(sock->tlsstream.tls) == 1); - INSIST(sock->statichandle == NULL); + isc__nmsocket_log_tls_session_reuse(sock, sock->tlsstream.tls); tlshandle = isc__nmhandle_get(sock, &sock->peer, &sock->iface); tls_read_stop(sock); + + if (isc__nm_closing(sock->worker)) { + result = ISC_R_SHUTTINGDOWN; + } + if (sock->tlsstream.server) { /* - * We need to check for 'sock->listener->closing' to - * make sure that we are not breaking the contract by - * calling an accept callback after the listener socket - * was shot down. Also, in this case the accept callback - * can be 'NULL'. That can happen as calling the accept - * callback in TLS is deferred until handshake is done. - * There is a possibility for that to happen *after* the - * underlying TCP connection was accepted. That is, a - * situation possible when the underlying TCP connection - * was accepted, handshake related data transmission - * took place, but in the middle of that the socket is - * being shot down before the TLS accept callback could - * have been called. + * The listening sockets are now closed from outer + * to inner order, which means that this function + * will never be called when the outer socket has + * stopped listening. * * Also see 'isc__nmsocket_stop()' - the function used * to shut down the listening TLS socket - for more * details. */ - if (isc__nm_closing(sock->worker)) { - result = ISC_R_SHUTTINGDOWN; - } else if (isc__nmsocket_closing(sock) || - sock->listener->closing) - { - result = ISC_R_CANCELED; - } else { - result = sock->listener->accept_cb( - tlshandle, result, - sock->listener->accept_cbarg); + if (result == ISC_R_SUCCESS) { + result = sock->accept_cb(tlshandle, result, + sock->accept_cbarg); } } else { tls_call_connect_cb(sock, tlshandle, result); @@ -873,7 +860,9 @@ tlslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { return (ISC_R_TLSERROR); } - isc__nmsocket_attach(tlslistensock, &tlssock->listener); + tlssock->accept_cb = tlslistensock->accept_cb; + tlssock->accept_cbarg = tlslistensock->accept_cbarg; + isc__nmsocket_attach(handle->sock, &tlssock->listener); isc_nmhandle_attach(handle, &tlssock->outerhandle); tlssock->peer = handle->sock->peer; tlssock->read_timeout = @@ -952,10 +941,7 @@ isc_nm_listentls(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, INSIST(result != ISC_R_UNSET); tlssock->nchildren = tlssock->outer->nchildren; - isc__nmsocket_barrier_init(tlssock); - if (result == ISC_R_SUCCESS) { - tlssock->listening = true; *sockp = tlssock; } diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index dc985dd187..5fea2e8f33 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -168,8 +168,6 @@ start_udp_child_job(void *arg) { } sock->reading = true; - sock->listening = true; - done: result = isc_uverr2result(r); @@ -951,9 +949,6 @@ udp_close_cb(uv_handle_t *handle) { isc__nmsocket_detach(&sock->server); } - /* All sockets */ - sock->listening = false; - if (sock->parent != NULL) { /* listening socket (listen) */ isc__nmsocket_detach(&sock);