From: Willy Tarreau Date: Wed, 27 Dec 2017 10:02:06 +0000 (+0100) Subject: BUG/MEDIUM: h2: properly handle and report some stream errors X-Git-Tag: v1.9-dev1~544 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a20a519b8f6d56b95f7f6b5e2b88087ac5889b2e;p=thirdparty%2Fhaproxy.git BUG/MEDIUM: h2: properly handle and report some stream errors Some stream errors applied to half-closed and closed streams are not properly reported, especially after the stream transistions to the closed state. The reason is that the code checks for this "error" stream state in order to send an RST frame. But if the stream was just closed or was already closed, there's no way to validate this condition, and the error is never reported to the peer. In order to address this situation, we'll add a new FRAME_E demux state which indicates that the previously parsed frame triggered a stream error of type STREAM CLOSED that needs to be reported. Proceeding like this will ensure that we don't lose that information even if we can't immediately send the message. It also removes the confusion where FRAME_A could be used either for ACKs or for RST. The state transition has been added after every h2s_error() on the demux path. It seems that we might need to have two distinct h2s_error() functions, one for the mux and another one for the demux, though it would provide little benefit. It also becomes more apparent that the H2_SS_ERROR state is only used to detect the need to report an error on the mux direction. Maybe this will have to be revisited later. This simple change managed to eliminate 5 bugs reported by h2spec. This fix must be backported to 1.8. --- diff --git a/src/mux_h2.c b/src/mux_h2.c index 4ee20b492a..18d4562ad5 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -67,7 +67,8 @@ enum h2_cs { H2_CS_SETTINGS1, // preface OK, waiting for first settings frame H2_CS_FRAME_H, // first settings frame ok, waiting for frame header H2_CS_FRAME_P, // frame header OK, waiting for frame payload - H2_CS_FRAME_A, // frame payload OK, trying to send ACK/RST frame + H2_CS_FRAME_A, // frame payload OK, trying to send ACK frame + H2_CS_FRAME_E, // frame payload OK, trying to send RST frame H2_CS_ERROR, // send GOAWAY(errcode) and close the connection ASAP H2_CS_ERROR2, // GOAWAY(errcode) sent, close the connection ASAP H2_CS_ENTRIES // must be last @@ -1429,7 +1430,7 @@ static int h2c_handle_window_update(struct h2c *h2c, struct h2s *h2s) strm_err: if (h2s) { h2s_error(h2s, error); - h2c->st0 = H2_CS_FRAME_A; + h2c->st0 = H2_CS_FRAME_E; } else h2c_error(h2c, error); @@ -1613,7 +1614,7 @@ static int h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s) if (h2s->st >= H2_SS_ERROR) { /* stream error : send RST_STREAM */ - h2c->st0 = H2_CS_FRAME_A; + h2c->st0 = H2_CS_FRAME_E; } else { /* update the max stream ID if the request is being processed */ @@ -1630,7 +1631,7 @@ static int h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s) strm_err: if (h2s) { h2s_error(h2s, error); - h2c->st0 = H2_CS_FRAME_A; + h2c->st0 = H2_CS_FRAME_E; } else h2c_error(h2c, error); @@ -1689,7 +1690,7 @@ static int h2c_frt_handle_data(struct h2c *h2c, struct h2s *h2s) if (h2s->st >= H2_SS_ERROR) { /* stream error : send RST_STREAM */ - h2c->st0 = H2_CS_FRAME_A; + h2c->st0 = H2_CS_FRAME_E; } /* check for completion : the callee will change this to FRAME_A or @@ -1714,7 +1715,7 @@ static int h2c_frt_handle_data(struct h2c *h2c, struct h2s *h2s) strm_err: if (h2s) { h2s_error(h2s, error); - h2c->st0 = H2_CS_FRAME_A; + h2c->st0 = H2_CS_FRAME_E; } else h2c_error(h2c, error); @@ -1807,6 +1808,9 @@ static void h2_process_demux(struct h2c *h2c) h2_skip_frame_hdr(h2c->dbuf); } + if (h2c->st0 == H2_CS_FRAME_E) + goto strm_err; + /* Only H2_CS_FRAME_P and H2_CS_FRAME_A here */ h2s = h2c_st_by_id(h2c, h2c->dsi); @@ -1826,6 +1830,7 @@ static void h2_process_demux(struct h2c *h2c) * this state MUST be treated as a stream error */ h2s_error(h2s, H2_ERR_STREAM_CLOSED); + h2c->st0 = H2_CS_FRAME_E; goto strm_err; } @@ -1878,8 +1883,10 @@ static void h2_process_demux(struct h2c *h2c) * we have nowhere to store the partial HEADERS frame. * Let's abort the stream on an INTERNAL_ERROR here. */ - if (h2c->st0 == H2_CS_FRAME_P) + if (h2c->st0 == H2_CS_FRAME_P) { h2s_error(h2s, H2_ERR_INTERNAL_ERROR); + h2c->st0 = H2_CS_FRAME_E; + } break; case H2_FT_HEADERS: @@ -1928,14 +1935,12 @@ static void h2_process_demux(struct h2c *h2c) } strm_err: - /* RST are sent similarly to frame acks */ - if (h2s->st == H2_SS_ERROR) { - if (h2c->st0 == H2_CS_FRAME_P) - h2c->st0 = H2_CS_FRAME_A; + /* We may have to send an RST if not done yet */ + if (h2s->st == H2_SS_ERROR) + h2c->st0 = H2_CS_FRAME_E; - if (h2c->st0 == H2_CS_FRAME_A) - ret = h2c_send_rst_stream(h2c, h2s); - } + if (h2c->st0 == H2_CS_FRAME_E) + ret = h2c_send_rst_stream(h2c, h2s); /* error or missing data condition met above ? */ if (ret <= 0) @@ -2518,6 +2523,7 @@ static int h2_frt_decode_headers(struct h2s *h2s, struct buffer *buf, int count) if (!h2c->dfl) { h2s_error(h2s, H2_ERR_PROTOCOL_ERROR); // empty headers frame! + h2c->st0 = H2_CS_FRAME_E; return 0; }