From: Willy Tarreau Date: Mon, 3 Jun 2019 06:17:30 +0000 (+0200) Subject: BUG/MEDIUM: connection: fix multiple handshake polling issues X-Git-Tag: v2.0-dev6~73 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6499b9d996ea8f57749021f56ed635c4688a727e;p=thirdparty%2Fhaproxy.git BUG/MEDIUM: connection: fix multiple handshake polling issues Connection handshakes were rarely stacked on top of each other, but the recent experiments consisting in sending PROXY over SOCKS4 revealed a number of issues in these lower layers. First, each handler waiting for data MUST subscribe to recv events with __conn_sock_want_recv() and MUST unsubscribe from send events using __conn_sock_stop_send() to avoid any wake-up loop in case a previous sender has set this. Second, each handler waiting for sending MUST subscribe to send events with __conn_sock_want_send() and MUST unsubscribe from recv events using __conn_sock_stop_recv() to avoid any wake-up loop in case some data are available on the connection. Till now this was done at various random places, and in particular the cases where the FD was not ready for recv forgot to re-enable reading. Second, while senders can happily use conn_sock_send() which automatically handles EINTR, loops, and marks the FD as not ready with fd_cant_send(), there is no equivalent for recv so receivers facing EAGAIN MUST call fd_cant_send() to enable polling. It could be argued that implementing an equivalent conn_sock_recv() function could be useful and more long-term proof than the current situation. Third, both types of handlers MUST unsubscribe from their respective events once they managed to do their job, and none may even play with __conn_xprt_*(). Here again this was lacking, and one surprizing call to __conn_xprt_stop_recv() was present in the proxy protocol parser for TCP6 messages! Thanks to Alexander Liu for his help on this issue. This patch must be backported to 1.9 and possibly some older versions, though the SOCKS parts should be dropped. --- diff --git a/src/connection.c b/src/connection.c index f0ca2bb165..8b0851ff60 100644 --- a/src/connection.c +++ b/src/connection.c @@ -487,7 +487,7 @@ int conn_recv_proxy(struct connection *conn, int flag) goto fail; if (!fd_recv_ready(conn->handle.fd)) - return 0; + goto not_ready; do { ret = recv(conn->handle.fd, trash.area, trash.size, MSG_PEEK); @@ -496,7 +496,7 @@ int conn_recv_proxy(struct connection *conn, int flag) continue; if (errno == EAGAIN) { fd_cant_recv(conn->handle.fd); - return 0; + goto not_ready; } goto recv_abort; } @@ -597,7 +597,6 @@ int conn_recv_proxy(struct connection *conn, int flag) } line++; } - __conn_xprt_stop_recv(conn); if (!dst_s || !sport_s || !dport_s) goto bad_header; @@ -746,8 +745,14 @@ int conn_recv_proxy(struct connection *conn, int flag) conn->flags &= ~flag; conn->flags |= CO_FL_RCVD_PROXY; + __conn_sock_stop_recv(conn); return 1; + not_ready: + __conn_sock_want_recv(conn); + __conn_sock_stop_send(conn); + return 0; + missing: /* Missing data. Since we're using MSG_PEEK, we can only poll again if * we have not read anything. Otherwise we need to fail because we won't @@ -803,7 +808,7 @@ int conn_recv_netscaler_cip(struct connection *conn, int flag) goto fail; if (!fd_recv_ready(conn->handle.fd)) - return 0; + goto not_ready; do { ret = recv(conn->handle.fd, trash.area, trash.size, MSG_PEEK); @@ -812,7 +817,7 @@ int conn_recv_netscaler_cip(struct connection *conn, int flag) continue; if (errno == EAGAIN) { fd_cant_recv(conn->handle.fd); - return 0; + goto not_ready; } goto recv_abort; } @@ -944,8 +949,14 @@ int conn_recv_netscaler_cip(struct connection *conn, int flag) } while (0); conn->flags &= ~flag; + __conn_sock_stop_recv(conn); return 1; + not_ready: + __conn_sock_want_recv(conn); + __conn_sock_stop_send(conn); + return 0; + missing: /* Missing data. Since we're using MSG_PEEK, we can only poll again if * we have not read anything. Otherwise we need to fail because we won't @@ -1049,6 +1060,7 @@ int conn_send_socks4_proxy_request(struct connection *conn) out_wait: __conn_sock_stop_recv(conn); + __conn_sock_want_send(conn); return 0; } @@ -1065,7 +1077,7 @@ int conn_recv_socks4_proxy_response(struct connection *conn) goto fail; if (!fd_recv_ready(conn->handle.fd)) - return 0; + goto not_ready; do { /* SOCKS4 Proxy will response with 8 bytes, 0x00 | 0x5A | 0x00 0x00 | 0x00 0x00 0x00 0x00 @@ -1100,8 +1112,7 @@ int conn_recv_socks4_proxy_response(struct connection *conn) } if (errno == EAGAIN) { fd_cant_recv(conn->handle.fd); - __conn_sock_want_recv(conn); - return 0; + goto not_ready; } goto recv_abort; } @@ -1111,9 +1122,7 @@ int conn_recv_socks4_proxy_response(struct connection *conn) /* Missing data. Since we're using MSG_PEEK, we can only poll again if * we are not able to read enough data. */ - fd_cant_recv(conn->handle.fd); - __conn_sock_want_recv(conn); - return 0; + goto not_ready; } /* @@ -1159,6 +1168,11 @@ int conn_recv_socks4_proxy_response(struct connection *conn) conn->flags &= ~CO_FL_SOCKS4_RECV; return 1; + not_ready: + __conn_sock_want_recv(conn); + __conn_sock_stop_send(conn); + return 0; + recv_abort: if (conn->err_code == CO_ER_NONE) { conn->err_code = CO_ER_SOCKS4_ABORT; diff --git a/src/stream_interface.c b/src/stream_interface.c index fd5311d837..a6d5d020ad 100644 --- a/src/stream_interface.c +++ b/src/stream_interface.c @@ -422,6 +422,7 @@ int conn_si_send_proxy(struct connection *conn, unsigned int flag) if (conn->flags & CO_FL_WAIT_L4_CONN) conn->flags &= ~CO_FL_WAIT_L4_CONN; conn->flags &= ~flag; + __conn_sock_stop_send(conn); return 1; out_error: @@ -431,6 +432,7 @@ int conn_si_send_proxy(struct connection *conn, unsigned int flag) out_wait: __conn_sock_stop_recv(conn); + __conn_sock_want_send(conn); return 0; }