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 5ef995f..1126eca 100644
--- a/src/h1_htx.c
+++ b/src/h1_htx.c
@@ -43,16 +43,6 @@
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 @@
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 @@
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 @@
}
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 2d9410d..5b8bf44 100644
--- a/src/h2.c
+++ b/src/h2.c
@@ -499,11 +499,19 @@
}
/* 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 @@
}
/* 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 b73ceb1..113cb8f 100644
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -1270,78 +1270,36 @@
}
/*
- * 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 @@
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 @@
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 @@
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 @@
}
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 @@
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 @@
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 @@
/* 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 38a0e2b..7ef2c6b 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4719,7 +4719,7 @@
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 @@
* 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 @@
* 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 @@
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 @@
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)