BUG/MAJOR: mux-h1/mux-h2/htx: Fix HTTP tunnel management at the mux level

Tunnel management between the H1 and H2 multiplexers is a bit blurred. And
the HTX is not enough well defined on this point to make things clear. In
fact, Establishing a tunnel between an H2 client and an H1 server, or the
opposite is buggy because the both multiplexers don't handle the EOM block
the same way when a tunnel is established. In fact, the H2 multiplexer is
pretty strict and add an END_STREAM flag when an EOM block is found, while
the H1 multiplexer is more flexible.

The purpose of this patch is to make the EOM block usage pretty clear and to
fix the HTTP multiplexers to really handle HTTP tunnels in the right
way. Now, an EOM block is used to mark the end of an HTTP message,
semantically speaking. That means it may be followed by tunneled data. Thus,
CONNECT requests are now finished by an EOM block, just after the EOH block.

On the H1 multiplexer side, a tunnel is now only established on the response
path. So a CONNECT request remains in a DONE state waiting for the 2xx
response. On the H2 multiplexer side, a flag is used to know an HTTP tunnel
is requested, to not immediately add the END_STREAM flag on the EOM block.

All these changes are sensitives and not backportable because of recent
changes. The same problem exists on earlier versions and should be
addressed. But it will only be possible with a specific patchset.

This patch relies on the following ones :

  * MEDIUM: mux-h1: Properly handle tunnel establishments and aborts
  * MEDIUM: mux-h2: Close streams when processing data for an aborted tunnel
  * MEDIUM: mux-h2: Block client data on server side waiting tunnel establishment
  * MINOR: mux-h2: Add 2 flags to help to properly handle tunnel mode
  * MINOR: mux-h1: Split H1C_F_WAIT_OPPOSITE flag to separate input/output sides
  * MINOR: mux-h1/mux-fcgi: Don't set TUNNEL mode if payload length is unknown

diff --git a/src/h1_htx.c b/src/h1_htx.c
index 5ef995f7dc65d152ee08a254ec1ecea427bcc874..1126eca6e25b5171a2be2433daf1a40173276485 100644 (file)
--- a/src/h1_htx.c
+++ b/src/h1_htx.c
@@ -43,16 +43,6 @@ static size_t h1_eval_htx_size(const struct ist p1, const struct ist p2, const s
        return sz;
 }
 
-/* Switch the message to tunnel mode. On the request, it must only be called for
- * a CONNECT method. On the response, this function must only be called on
- * successful replies to CONNECT requests or on protocol switching.
- */
-static void h1_set_tunnel_mode(struct h1m *h1m)
-{
-       h1m->flags &= ~(H1_MF_XFER_LEN|H1_MF_CLEN|H1_MF_CHNK);
-       h1m->state = H1_MSG_TUNNEL;
-}
-
 /* Check the validity of the request version. If the version is valid, it
  * returns 1. Otherwise, it returns 0.
  */
@@ -146,8 +136,6 @@ static unsigned int h1m_htx_sl_flags(struct h1m *h1m)
                else
                        flags |= HTX_SL_F_BODYLESS;
        }
-       if (h1m->state == H1_MSG_TUNNEL)
-               flags |= HTX_SL_F_BODYLESS;
        if (h1m->flags & H1_MF_CONN_UPG)
                flags |= HTX_SL_F_CONN_UPG;
        return flags;
@@ -186,8 +174,8 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx
        h1m->flags |= H1_MF_XFER_LEN;
 
        if (h1sl->rq.meth == HTTP_METH_CONNECT) {
-               /* Switch CONNECT requests to tunnel mode */
-               h1_set_tunnel_mode(h1m);
+               h1m->flags &= ~(H1_MF_CLEN|H1_MF_CHNK);
+               h1m->curr_len = h1m->body_len = 0;
        }
 
        used = htx_used_space(htx);
@@ -281,9 +269,9 @@ static int h1_postparse_res_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx
        }
 
        if (((h1m->flags & H1_MF_METH_CONNECT) && code >= 200 && code < 300) || code == 101) {
-               /* Switch successful replies to CONNECT requests and
-                * protocol switching to tunnel mode. */
-               h1_set_tunnel_mode(h1m);
+               h1m->flags &= ~(H1_MF_CLEN|H1_MF_CHNK);
+               h1m->flags |= H1_MF_XFER_LEN;
+               h1m->curr_len = h1m->body_len = 0;
        }
        else if ((h1m->flags & H1_MF_METH_HEAD) || (code >= 100 && code < 200) ||
                 (code == 204) || (code == 304)) {

diff --git a/src/h2.c b/src/h2.c
index 2d9410d827bf7471f069640dd2869e3bd8249d53..5b8bf44feaec48e4ec557ba18d311a60195aa1e1 100644 (file)
--- a/src/h2.c
+++ b/src/h2.c
@@ -499,11 +499,19 @@ int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *ms
        }
 
        /* now send the end of headers marker */
-       htx_add_endof(htx, HTX_BLK_EOH);
+       if (!htx_add_endof(htx, HTX_BLK_EOH))
+               goto fail;
 
        /* Set bytes used in the HTX message for the headers now */
        sl->hdrs_bytes = htx_used_space(htx) - used;
 
+       if (*msgf & H2_MSGF_BODY_TUNNEL) {
+               /* Add the EOM for tunnel requests (CONNECT) */
+               htx->flags |= HTX_FL_EOI; /* no more message data are expected */
+               if (!htx_add_endof(htx, HTX_BLK_EOM))
+                       goto fail;
+       }
+
        ret = 1;
        return ret;
 
@@ -704,11 +712,19 @@ int h2_make_htx_response(struct http_hdr *list, struct htx *htx, unsigned int *m
        }
 
        /* now send the end of headers marker */
-       htx_add_endof(htx, HTX_BLK_EOH);
+       if (!htx_add_endof(htx, HTX_BLK_EOH))
+               goto fail;
 
        /* Set bytes used in the HTX message for the headers now */
        sl->hdrs_bytes = htx_used_space(htx) - used;
 
+       if (*msgf & H2_MSGF_BODY_TUNNEL) {
+               /* Tunnel sucessfully established, add the EOM now, all data are part of the tunnel */
+               htx->flags |= HTX_FL_EOI; /* no more message data are expected */
+               if (!htx_add_endof(htx, HTX_BLK_EOM))
+                       goto fail;
+       }
+
        ret = 1;
        return ret;
 

diff --git a/src/mux_h1.c b/src/mux_h1.c
index b73ceb1fc3e211c34c114570f05fc0fede265424..113cb8f745bc9dc57c1a045928b6a7816efd59b6 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -1270,78 +1270,36 @@ static void h1_emit_chunk_crlf(struct buffer *buf)
 }
 
 /*
- * Switch the request to tunnel mode. This function must only be called for
- * CONNECT requests. On the client side, if the response is not finished, the
- * mux is mark as busy on input.
+ * Switch the stream to tunnel mode. This function must only be called on 2xx
+ * (successful) replies to CONNECT requests or on 101 (switching protocol).
  */
-static void h1_set_req_tunnel_mode(struct h1s *h1s)
+static void h1_set_tunnel_mode(struct h1s *h1s)
 {
-       h1s->req.flags &= ~(H1_MF_XFER_LEN|H1_MF_CLEN|H1_MF_CHNK);
-       h1s->req.state = H1_MSG_TUNNEL;
-       TRACE_STATE("switch H1 request in tunnel mode", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1s->h1c->conn, h1s);
-
-       if (!(h1s->h1c->flags & H1C_F_IS_BACK)) {
-               h1s->flags &= ~H1S_F_PARSING_DONE;
-               if (h1s->res.state < H1_MSG_DONE) {
-                       h1s->h1c->flags |= H1C_F_WAIT_OUTPUT;
-                       TRACE_STATE("Disable read on h1c (wait_output)", H1_EV_RX_DATA|H1_EV_H1C_BLK, h1s->h1c->conn, h1s);
-               }
-       }
-       else if (h1s->h1c->flags & H1C_F_WAIT_OUTPUT) {
-               h1s->h1c->flags &= ~H1C_F_WAIT_OUTPUT;
-               tasklet_wakeup(h1s->h1c->wait_event.tasklet);
-               TRACE_STATE("Re-enable read on h1c", H1_EV_RX_DATA|H1_EV_H1C_BLK|H1_EV_H1C_WAKE, h1s->h1c->conn, h1s);
-       }
-}
+       struct h1c *h1c = h1s->h1c;
 
-/*
- * Switch the response to tunnel mode. This function must only be called on
- * successful replies to CONNECT requests or on protocol switching. In this
- * last case, this function takes care to switch the request to tunnel mode if
- * possible. On the server side, if the request is not finished, the mux is mark
- * as busy on input.
- */
-static void h1_set_res_tunnel_mode(struct h1s *h1s)
-{
+       h1s->req.state = H1_MSG_TUNNEL;
+       h1s->req.flags &= ~(H1_MF_XFER_LEN|H1_MF_CLEN|H1_MF_CHNK);
 
-       h1s->res.flags &= ~(H1_MF_XFER_LEN|H1_MF_CLEN|H1_MF_CHNK);
        h1s->res.state = H1_MSG_TUNNEL;
-       TRACE_STATE("switch H1 response in tunnel mode", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1s->h1c->conn, h1s);
+       h1s->res.flags &= ~(H1_MF_XFER_LEN|H1_MF_CLEN|H1_MF_CHNK);
 
-       if (h1s->h1c->flags & H1C_F_IS_BACK) {
-               h1s->flags &= ~H1S_F_PARSING_DONE;
-               /* On protocol switching, switch the request to tunnel mode if it is in
-                * DONE state. Otherwise we will wait the end of the request to switch
-                * it in tunnel mode.
-                */
-               if (h1s->req.state < H1_MSG_DONE) {
-                       h1s->h1c->flags |= H1C_F_WAIT_OUTPUT;
-                       TRACE_STATE("Disable read on h1c (wait_output)", H1_EV_RX_DATA|H1_EV_H1C_BLK, h1s->h1c->conn, h1s);
-               }
-               else  {
-                       h1s->req.flags &= ~(H1_MF_XFER_LEN|H1_MF_CLEN|H1_MF_CHNK);
-                       h1s->req.state = H1_MSG_TUNNEL;
-                       TRACE_STATE("switch H1 request in tunnel mode", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1s->h1c->conn, h1s);
+       TRACE_STATE("switch H1 stream in tunnel mode", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1c->conn, h1s);
+       if (h1c->flags & H1C_F_IS_BACK)
+                h1s->flags &= ~H1S_F_PARSING_DONE;
 
-                       if (h1s->h1c->flags & H1C_F_WAIT_INPUT) {
-                               h1s->h1c->flags &= ~H1C_F_WAIT_INPUT;
-                               h1_wake_stream_for_send(h1s);
-                               if (b_data(&h1s->h1c->obuf))
-                                       tasklet_wakeup(h1s->h1c->wait_event.tasklet);
-                               TRACE_STATE("Re-enable send on h1c", H1_EV_TX_DATA|H1_EV_H1C_BLK|H1_EV_H1C_WAKE, h1s->h1c->conn, h1s);
-                       }
-               }
+       if (h1c->flags & H1C_F_WAIT_OUTPUT) {
+               h1c->flags &= ~H1C_F_WAIT_OUTPUT;
+               if (b_data(&h1c->ibuf))
+                       h1_wake_stream_for_recv(h1s);
+               tasklet_wakeup(h1c->wait_event.tasklet);
+               TRACE_STATE("Re-enable read on h1c", H1_EV_RX_DATA|H1_EV_H1C_BLK|H1_EV_H1C_WAKE, h1c->conn, h1s);
        }
-       else {
-               h1s->req.flags &= ~(H1_MF_XFER_LEN|H1_MF_CLEN|H1_MF_CHNK);
-               h1s->req.state = H1_MSG_TUNNEL;
-               TRACE_STATE("switch H1 request in tunnel mode", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1s->h1c->conn, h1s);
-
-               if (h1s->h1c->flags & H1C_F_WAIT_OUTPUT) {
-                       h1s->h1c->flags &= ~H1C_F_WAIT_OUTPUT;
-                       tasklet_wakeup(h1s->h1c->wait_event.tasklet);
-                       TRACE_STATE("Re-enable read on h1c", H1_EV_RX_DATA|H1_EV_H1C_BLK|H1_EV_H1C_WAKE, h1s->h1c->conn, h1s);
-               }
+       if (h1c->flags & H1C_F_WAIT_INPUT) {
+               h1c->flags &= ~H1C_F_WAIT_INPUT;
+               h1_wake_stream_for_send(h1s);
+               if (b_data(&h1c->obuf))
+                       tasklet_wakeup(h1c->wait_event.tasklet);
+               TRACE_STATE("Re-enable send on h1c", H1_EV_TX_DATA|H1_EV_H1C_BLK|H1_EV_H1C_WAKE, h1c->conn, h1s);
        }
 }
 
@@ -1383,16 +1341,11 @@ static size_t h1_process_headers(struct h1s *h1s, struct h1m *h1m, struct htx *h
                h1_capture_bad_message(h1s->h1c, h1s, h1m, buf);
        }
 
-       if (!(h1m->flags & H1_MF_RESP)) {
+       if (!(h1m->flags & H1_MF_RESP))
                h1s->meth = h1sl.rq.meth;
-               if (h1m->state == H1_MSG_TUNNEL)
-                       h1_set_req_tunnel_mode(h1s);
-       }
-       else {
+       else
                h1s->status = h1sl.st.status;
-               if (h1m->state == H1_MSG_TUNNEL)
-                       h1_set_res_tunnel_mode(h1s);
-       }
+
        h1_process_input_conn_mode(h1s, h1m, htx);
        *ofs += ret;
 
@@ -1511,6 +1464,9 @@ static size_t h1_process_input(struct h1c *h1c, struct buffer *buf, size_t count
        if (h1s->flags & (H1S_F_PARSING_ERROR|H1S_F_NOT_IMPL_ERROR))
                goto end;
 
+       if (h1c->flags & H1C_F_WAIT_OUTPUT)
+               goto end;
+
        do {
                size_t used = htx_used_space(htx);
 
@@ -1565,8 +1521,9 @@ static size_t h1_process_input(struct h1c *h1c, struct buffer *buf, size_t count
                                           H1_EV_RX_DATA|H1_EV_RX_EOI, h1c->conn, h1s, htx);
                        }
 
-                       if (!(h1m->flags & H1_MF_RESP) && h1s->status == 101)
-                               h1_set_req_tunnel_mode(h1s);
+                       if ((h1m->flags & H1_MF_RESP) &&
+                           ((h1s->meth == HTTP_METH_CONNECT && h1s->status >= 200 && h1s->status < 300) || h1s->status == 101))
+                               h1_set_tunnel_mode(h1s);
                        else {
                                if (h1s->req.state < H1_MSG_DONE || h1s->res.state < H1_MSG_DONE) {
                                        /* Unfinished transaction: block this input side waiting the end of the output side */
@@ -1596,7 +1553,8 @@ static size_t h1_process_input(struct h1c *h1c, struct buffer *buf, size_t count
                }
 
                count -= htx_used_space(htx) - used;
-       } while (!(h1s->flags & (H1S_F_PARSING_ERROR|H1S_F_NOT_IMPL_ERROR)));
+       } while (!(h1s->flags & (H1S_F_PARSING_ERROR|H1S_F_NOT_IMPL_ERROR)) && !(h1c->flags & H1C_F_WAIT_OUTPUT));
+
 
        if (h1s->flags & (H1S_F_PARSING_ERROR|H1S_F_NOT_IMPL_ERROR)) {
                TRACE_PROTO("parsing or not-implemented error", H1_EV_RX_DATA|H1_EV_H1S_ERR, h1c->conn, h1s);
@@ -1660,10 +1618,11 @@ static size_t h1_process_input(struct h1c *h1c, struct buffer *buf, size_t count
                h1s->cs->flags &= ~CS_FL_MAY_SPLICE;
        }
 
-       /* Don't set EOI on the conn-stream for protocol upgrade requests, wait
-        * the response to do so or not depending on the status code.
+       /* Don't set EOI on the conn-stream for protocol upgrade or connect
+        * requests, wait the response to do so or not depending on the status
+        * code.
         */
-       if ((h1s->flags & H1S_F_PARSING_DONE) && !(h1m->flags & H1_MF_CONN_UPG))
+       if ((h1s->flags & H1S_F_PARSING_DONE) && (h1s->meth != HTTP_METH_CONNECT) && !(h1m->flags & H1_MF_CONN_UPG))
                h1s->cs->flags |= CS_FL_EOI;
 
        if (h1s_data_pending(h1s) && !htx_is_empty(htx))
@@ -1966,11 +1925,13 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
                                            H1_EV_TX_DATA|H1_EV_TX_HDRS, h1c->conn, h1s);
 
                                if (!(h1m->flags & H1_MF_RESP) && h1s->meth == HTTP_METH_CONNECT) {
-                                       goto done;
+                                       /* Must have a EOM before tunnel data */
+                                       h1m->state = H1_MSG_DONE;
                                }
                                else if ((h1m->flags & H1_MF_RESP) &&
                                         ((h1s->meth == HTTP_METH_CONNECT && h1s->status >= 200 && h1s->status < 300) || h1s->status == 101)) {
-                                       goto done;
+                                       /* Must have a EOM before tunnel data */
+                                       h1m->state = H1_MSG_DONE;
                                }
                                else if ((h1m->flags & H1_MF_RESP) &&
                                         h1s->status < 200 && (h1s->status == 100 || h1s->status >= 102)) {
@@ -2088,10 +2049,11 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
                                        /* a successful reply to a CONNECT or a protocol switching is sent
                                         * to the client. Switch the response to tunnel mode.
                                         */
-                                       h1_set_res_tunnel_mode(h1s);
+                                       h1_set_tunnel_mode(h1s);
                                        TRACE_STATE("switch H1 response in tunnel mode", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1c->conn, h1s);
                                }
-                               else if (h1s->h1c->flags & H1C_F_WAIT_OUTPUT) {
+
+                               if (h1s->h1c->flags & H1C_F_WAIT_OUTPUT) {
                                        h1s->h1c->flags &= ~H1C_F_WAIT_OUTPUT;
                                        h1c->conn->xprt->subscribe(h1c->conn, h1c->conn->xprt_ctx, SUB_RETRY_RECV, &h1c->wait_event);
                                        TRACE_STATE("Re-enable read on h1c", H1_EV_TX_DATA|H1_EV_H1C_BLK|H1_EV_H1C_WAKE, h1c->conn, h1s);

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 38a0e2b8b89ed9118e97d8fd3871d4ee5cf5a63c..7ef2c6bee7f01f7c7b7eaf41754c761cbc65ea9b 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4719,7 +4719,7 @@ next_frame:
        if (!(msgf & H2_MSGF_RSP_1XX))
                *flags |= H2_SF_HEADERS_RCVD;
 
-       if ((h2c->dff & H2_F_HEADERS_END_STREAM)) {
+       if (htx_get_tail_type(htx) != HTX_BLK_EOM && (h2c->dff & H2_F_HEADERS_END_STREAM)) {
                /* Mark the end of message using EOM */
                htx->flags |= HTX_FL_EOI; /* no more data are expected. Only EOM remains to add now */
                if (!htx_add_endof(htx, HTX_BLK_EOM)) {
@@ -4857,12 +4857,19 @@ try_again:
         * transferred.
         */
 
-       if (h2c->dff & H2_F_DATA_END_STREAM) {
-               htx->flags |= HTX_FL_EOI; /* no more data are expected. Only EOM remains to add now */
-               if (!htx_add_endof(htx, HTX_BLK_EOM)) {
-                       TRACE_STATE("h2s rxbuf is full, failed to add EOM", H2_EV_RX_FRAME|H2_EV_RX_DATA|H2_EV_H2S_BLK, h2c->conn, h2s);
-                       h2c->flags |= H2_CF_DEM_SFULL;
-                       goto fail;
+       if (!(h2s->flags & H2_SF_BODY_TUNNEL) && (h2c->dff & H2_F_DATA_END_STREAM)) {
+               /* no more data are expected for this message. This add the EOM
+                * flag but only on the response path or if no tunnel attempt
+                * was aborted. Otherwise (request path + tunnel abrted), the
+                * EOM was already reported.
+                */
+               if ((h2c->flags & H2_CF_IS_BACK) || !(h2s->flags & H2_SF_TUNNEL_ABRT)) {
+                       htx->flags |= HTX_FL_EOI; /* no more data are expected. Only EOM remains to add now */
+                       if (!htx_add_endof(htx, HTX_BLK_EOM)) {
+                               TRACE_STATE("h2s rxbuf is full, failed to add EOM", H2_EV_RX_FRAME|H2_EV_RX_DATA|H2_EV_H2S_BLK, h2c->conn, h2s);
+                               h2c->flags |= H2_CF_DEM_SFULL;
+                               goto fail;
+                       }
                }
        }
 
@@ -5056,7 +5063,14 @@ static size_t h2s_frt_make_resp_headers(struct h2s *h2s, struct htx *htx)
         * FIXME: we should also set it when we know for sure that the
         * content-length is zero as well as on 204/304
         */
-       if (blk_end && htx_get_blk_type(blk_end) == HTX_BLK_EOM && h2s->status >= 200)
+       if ((h2s->flags & H2_SF_BODY_TUNNEL) && h2s->status >= 200 && h2s->status < 300) {
+               /* Don't set EOM if a tunnel is successfully established
+                * (2xx responses to a connect). In this case, the EOM must be found
+                */
+               if (!blk_end || htx_get_blk_type(blk_end) != HTX_BLK_EOM)
+                       goto fail;
+       }
+       else if (blk_end && htx_get_blk_type(blk_end) == HTX_BLK_EOM && h2s->status >= 200)
                es_now = 1;
 
        if (!h2s->cs || h2s->cs->flags & CS_FL_SHW)
@@ -5422,6 +5436,9 @@ static size_t h2s_bck_make_req_headers(struct h2s *h2s, struct htx *htx)
                        es_now = 1;
        }
        else {
+               /* For CONNECT requests, the EOM must be found and eaten without setting the ES */
+               if (!blk_end || htx_get_blk_type(blk_end) != HTX_BLK_EOM)
+                       goto fail;
                h2s->flags |= H2_SF_BODY_TUNNEL;
        }
 
@@ -5754,7 +5771,12 @@ static size_t h2s_make_data(struct h2s *h2s, struct buffer *buf, size_t count)
        if (type == HTX_BLK_EOM) {
                total++; // EOM counts as one byte
                count--;
-               es_now = 1;
+
+               /* EOM+empty: we may need to add END_STREAM (except for tunneled
+                * message)
+                */
+               if (!(h2s->flags & H2_SF_BODY_TUNNEL))
+                       es_now = 1;
        }
 
        if (es_now)