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 f0ca2bb165db8dd6a9a99a8dae82d83a31e73bbc..8b0851ff60c428639b1a1e4eebee48e964aeb40f 100644 (file)
--- 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 fd5311d837299cba56b60f77ec6830c19c711713..a6d5d020ada918bee8abee70dce74f0c12ea1304 100644 (file)
--- 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;
 }