From: Christopher Faulet Date: Thu, 5 Dec 2019 09:23:37 +0000 (+0100) Subject: BUG/MEDIUM: mux-h1: Never reuse H1 connection if a shutw is pending X-Git-Tag: v2.2-dev1~213 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=aaa67bcef299486f1cdb75ef28b3ec6c39713ae6;p=thirdparty%2Fhaproxy.git 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 d7e35cd42e..1f296fddc2 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 @@ static inline void h1_release_buf(struct h1c *h1c, struct buffer *bptr) } } -/* 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 @@ static struct h1s *h1s_create(struct h1c *h1c, struct conn_stream *cs, struct se 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 @@ static void h1s_destroy(struct h1s *h1s) 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 @@ static int h1_init(struct connection *conn, struct proxy *proxy, struct session 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 @@ static int h1_process(struct h1c * h1c) 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 @@ static void h1_detach(struct conn_stream *cs) 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 @@ static void h1_detach(struct conn_stream *cs) 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 @@ static void h1_detach(struct conn_stream *cs) */ 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; }