From db991c2658e5b35dee0a18512f86ba107d724136 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Thu, 9 Feb 2023 17:53:41 +0100 Subject: [PATCH] BUG/MEDIUM: quic: fix crash when "option nolinger" is set in the frontend Commit 0aba11e9e ("MINOR: quic: remove unnecessary quic_session_accept()") overlooked one problem, in session_accept_fd() at the end, there's a bunch of FD-specific stuff that either sets up or resets the socket at the TCP level. The tests are mostly performed for AF_INET/AF_INET6 families but they're only for one part (i.e. to avoid setting up TCP options on UNIX sockets). Other pieces continue to configure the socket regardless of its family. All of this directly acts on the FD, which is not correct since the FD is not valid here, it corresponds to the QUIC handle. The issue is much more visible when "option nolinger" is enabled in the frontend, because the access to fdatb[cfd].state immediately crashes on the first connection, as can be seen in github issue #2030. This patch bypasses this setup for FD-less connections, such as QUIC. However some of them could definitely be relevant to the QUIC stack, or even to UNIX sockets sometimes. A better long-term solution would consist in implementing a setsockopt() equivalent at the protocol layer that would be used to configure the socket, either the FD or the QUIC conn depending on the case. Some of them would not always be implemented but that would allow to unify all this code. This fix must be backported everywhere the commit above is backported, namely 2.6 and 2.7. Thanks to github user @twomoses for the nicely detailed report. --- src/session.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/session.c b/src/session.c index a6cca8243..c53141d9e 100644 --- a/src/session.c +++ b/src/session.c @@ -192,7 +192,8 @@ int session_accept_fd(struct connection *cli_conn) */ if (!LIST_ISEMPTY(&p->tcp_req.l4_rules) && !tcp_exec_l4_rules(sess)) { /* let's do a no-linger now to close with a single RST. */ - setsockopt(cfd, SOL_SOCKET, SO_LINGER, (struct linger *) &nolinger, sizeof(struct linger)); + if (!(cli_conn->flags & CO_FL_FDLESS)) + setsockopt(cfd, SOL_SOCKET, SO_LINGER, (struct linger *) &nolinger, sizeof(struct linger)); ret = 0; /* successful termination */ goto out_free_sess; } @@ -200,6 +201,12 @@ int session_accept_fd(struct connection *cli_conn) if (conn_xprt_start(cli_conn) < 0) goto out_free_sess; + /* FIXME/WTA: we should implement the setsockopt() calls at the proto + * level instead and let non-inet protocols implement their own equivalent. + */ + if (cli_conn->flags & CO_FL_FDLESS) + goto skip_fd_setup; + /* Adjust some socket options */ if (l->rx.addr.ss_family == AF_INET || l->rx.addr.ss_family == AF_INET6) { setsockopt(cfd, IPPROTO_TCP, TCP_NODELAY, (char *) &one, sizeof(one)); @@ -245,6 +252,7 @@ int session_accept_fd(struct connection *cli_conn) if (global.tune.client_rcvbuf) setsockopt(cfd, SOL_SOCKET, SO_RCVBUF, &global.tune.client_rcvbuf, sizeof(global.tune.client_rcvbuf)); + skip_fd_setup: /* OK, now either we have a pending handshake to execute with and then * we must return to the I/O layer, or we can proceed with the end of * the stream initialization. In case of handshake, we also set the I/O @@ -292,7 +300,8 @@ int session_accept_fd(struct connection *cli_conn) out_free_conn: if (ret < 0 && l->bind_conf->xprt == xprt_get(XPRT_RAW) && - p->mode == PR_MODE_HTTP && l->bind_conf->mux_proto == NULL) { + p->mode == PR_MODE_HTTP && l->bind_conf->mux_proto == NULL && + !(cli_conn->flags & CO_FL_FDLESS)) { /* critical error, no more memory, try to emit a 500 response */ send(cfd, http_err_msgs[HTTP_ERR_500], strlen(http_err_msgs[HTTP_ERR_500]), MSG_DONTWAIT|MSG_NOSIGNAL);