BUG/MEDIUM: mux-h1: Never reuse H1 connection if a shutw is pending
On the server side, when a H1 stream is detached from the connection, if the
connection is not reusable but some outgoing data remain, the connection is not
immediatly released. In this case, the connection is not inserted in any idle
connection list. But it is still attached to the session. Because of that, it
can be erroneously reused. h1_avail_streams() always report a free slot if no
stream is attached to the connection, independently on the connection's
state. It is obviously a bug. If a second request is handled by the same session
(it happens with H2 connections on the client side), this connection is reused
before we close it.
There is small window to hit the bug, but it may lead to very strange
behaviors. For instance, if a first h2 request is quickly aborted by the client
while it is blocked in the mux on the server side (so before any response is
received), a second request can be processed and sent to the server. Because the
connection was not closed, the possible reply to the first request will be
interpreted as a reply to the second one. It is probably the bug described by
Peter Fröhlich in the issue #290.
To fix the bug, a new flag has been added to know if an H1 connection is idle or
not. So now, H1C_F_CS_IDLE is set when a connection is idle and useable to
handle a new request. If it is set, we try to add the connection in an idle
connection list. And h1_avail_streams() only relies on this flag
now. Concretely, this flag is set when a K/A stream is detached and both the
request and the response are in DONE state. It is exclusive to other H1C_F_CS
flags.
This patch must be backported as far as 1.9.
diff --git a/src/mux_h1.c b/src/mux_h1.c
index d7e35cd..1f296fd 100644
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -50,6 +50,8 @@
#define H1C_F_CS_ERROR 0x00001000 /* connection must be closed ASAP because an error occurred */
#define H1C_F_CS_SHUTW_NOW 0x00002000 /* connection must be shut down for writes ASAP */
#define H1C_F_CS_SHUTDOWN 0x00004000 /* connection is shut down for read and writes */
+#define H1C_F_CS_IDLE 0x00008000 /* connection is idle and may be reused
+ * (exclusive to all H1C_F_CS flags and never set when an h1s is attached) */
#define H1C_F_WAIT_NEXT_REQ 0x00010000 /* waiting for the next request to start, use keep-alive timeout */
#define H1C_F_UPG_H2C 0x00020000 /* set if an upgrade to h2 should be done */
@@ -443,15 +445,16 @@
}
}
-/* returns the number of streams in use on a connection to figure if it's
- * idle or not. We can't have an h1s without a CS so checking h1s is fine,
- * as the caller will want to know if it was the last one after a detach().
+/* returns the number of streams in use on a connection to figure if it's idle
+ * or not. We rely on H1C_F_CS_IDLE to know if the connection is in-use or
+ * not. This flag is only set when no H1S is attached and when the previous
+ * stream, if any, was fully terminated without any error and in K/A mode.
*/
static int h1_used_streams(struct connection *conn)
{
struct h1c *h1c = conn->ctx;
- return h1c->h1s ? 1 : 0;
+ return ((h1c->flags & H1C_F_CS_IDLE) ? 0 : 1);
}
/* returns the number of streams still available on a connection */
@@ -539,7 +542,7 @@
if (h1c->flags & H1C_F_WAIT_NEXT_REQ)
h1s->flags |= H1S_F_NOT_FIRST;
- h1c->flags &= ~H1C_F_WAIT_NEXT_REQ;
+ h1c->flags &= ~(H1C_F_CS_IDLE|H1C_F_WAIT_NEXT_REQ);
if (!conn_is_back(h1c->conn)) {
if (h1c->px->options2 & PR_O2_REQBUG_OK)
@@ -609,11 +612,18 @@
h1s->send_wait->events &= ~SUB_RETRY_SEND;
h1c->flags &= ~H1C_F_IN_BUSY;
- h1c->flags |= H1C_F_WAIT_NEXT_REQ;
if (h1s->flags & (H1S_F_REQ_ERROR|H1S_F_RES_ERROR)) {
h1c->flags |= H1C_F_CS_ERROR;
TRACE_STATE("h1s on error, set error on h1c", H1_EV_H1C_ERR, h1c->conn, h1s);
}
+
+ if (!(h1c->flags & (H1C_F_CS_ERROR|H1C_F_CS_SHUTW_NOW|H1C_F_CS_SHUTDOWN)) && /* No error/shutdown on h1c */
+ !(h1c->conn->flags & (CO_FL_ERROR|CO_FL_SOCK_RD_SH|CO_FL_SOCK_WR_SH)) && /* No error/shutdown on conn */
+ (h1s->flags & H1S_F_WANT_KAL) && /* K/A possible */
+ h1s->req.state == H1_MSG_DONE && h1s->res.state == H1_MSG_DONE) { /* req/res in DONE state */
+ h1c->flags |= (H1C_F_CS_IDLE|H1C_F_WAIT_NEXT_REQ);
+ TRACE_STATE("set idle mode on h1c, waiting for the next request", H1_EV_H1C_ERR, h1c->conn, h1s);
+ }
pool_free(pool_head_h1s, h1s);
}
}
@@ -650,7 +660,7 @@
h1c->conn = conn;
h1c->px = proxy;
- h1c->flags = H1C_F_NONE;
+ h1c->flags = H1C_F_CS_IDLE;
h1c->ibuf = *input;
h1c->obuf = BUF_NULL;
h1c->h1s = NULL;
@@ -2137,10 +2147,9 @@
if (!h1s) {
if (h1c->flags & (H1C_F_CS_ERROR|H1C_F_CS_SHUTDOWN) ||
- conn->flags & (CO_FL_ERROR | CO_FL_SOCK_WR_SH) ||
- conn_xprt_read0_pending(conn))
+ conn->flags & (CO_FL_ERROR|CO_FL_SOCK_RD_SH|CO_FL_SOCK_WR_SH))
goto release;
- if (!conn_is_back(conn) && !(h1c->flags & (H1C_F_CS_SHUTW_NOW|H1C_F_CS_SHUTDOWN))) {
+ if (!conn_is_back(conn) && (h1c->flags & H1C_F_CS_IDLE)) {
TRACE_STATE("K/A incoming connection, create new H1 stream", H1_EV_H1C_WAKE, conn);
if (!h1s_create(h1c, NULL, NULL))
goto release;
@@ -2334,7 +2343,6 @@
struct h1s *h1s = cs->ctx;
struct h1c *h1c;
struct session *sess;
- int has_keepalive;
int is_not_first;
TRACE_ENTER(H1_EV_STRM_END, h1s ? h1s->h1c->conn : NULL, h1s);
@@ -2349,12 +2357,10 @@
h1c = h1s->h1c;
h1s->cs = NULL;
- has_keepalive = h1s->flags & H1S_F_WANT_KAL;
is_not_first = h1s->flags & H1S_F_NOT_FIRST;
h1s_destroy(h1s);
- if (conn_is_back(h1c->conn) && has_keepalive &&
- !(h1c->conn->flags & (CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH))) {
+ if (conn_is_back(h1c->conn) && (h1c->flags & H1C_F_CS_IDLE)) {
/* If there are any excess server data in the input buffer,
* release it and close the connection ASAP (some data may
* remain in the output buffer). This happens if a server sends
@@ -2363,7 +2369,7 @@
*/
if (b_data(&h1c->ibuf)) {
h1_release_buf(h1c, &h1c->ibuf);
- h1c->flags |= H1C_F_CS_SHUTW_NOW;
+ h1c->flags = (h1c->flags & ~H1C_F_CS_IDLE) | H1C_F_CS_SHUTW_NOW;
TRACE_DEVEL("remaining data on detach, kill connection", H1_EV_STRM_END|H1_EV_H1C_END);
goto release;
}