mirrors/haproxy
1
0
Fork 0
mirror of https://git.haproxy.org/git/haproxy.git/ synced 2025-09-21 05:41:26 +02:00
Code Issues Projects Releases Wiki Activity

BUG/MEDIUM: mux-h2: refine connection vs stream error on headers

Browse Source 
Commit 7021a8c4d8 ("BUG/MINOR: mux-h2: also count streams for refused
ones") addressed stream counting issues on some error cases but not
completely correctly regarding the conn_err vs stream_err case. Indeed,
contrary to the initial analysis, h2c_dec_hdrs() can set H2_CS_ERROR
when facing some unrecoverable protocol errors, and it's not correct
to send it to strm_err which will only send the RST_STREAM frame and
the subsequent GOAWAY frame is in fact the result of the read timeout.

The difficulty behind this lies on the sequence of output validations
because h2c_dec_hdrs() returns two results at once:
  - frame processing status (done/incomplete/failed)
  - connection error status

The original ordering requires to write 2 exemplaries of the exact
same error handling code disposed differently, which the patch above
tried to factor to one. After careful inspection of h2c_dec_hdrs()
and its comments, it's clear that it always returns -1 on failure,
*including* connection errors. This means we can rearrange the test
to get rid of the missing data first, and immediately enter the
no-return zone where both the stream and connection errors can be
checked at the same place, making sure to consistently maintain
error counters. This is way better because we don't have to update
stream counters on the error path anymore. h2spec now passes the
test much faster.

This will need to be backported to the same branches as the commit
above, which was already backported to 2.9.
This commit is contained in:
Willy Tarreau 2024-01-18 17:01:45 +01:00
parent 62ef7966f0
commit e1c8bfd0ed
1 changed files with 31 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
src/mux_h2.c
View File

@ -2812,39 +2812,47 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
error = h2c_dec_hdrs(h2c, &rxbuf, &flags, &body_len, NULL); error = h2c_dec_hdrs(h2c, &rxbuf, &flags, &body_len, NULL);
/* unrecoverable error ? */ if (error == 0) {
if (h2c->st0 >= H2_CS_ERROR) { /* No error but missing data for demuxing, it is an incomplete frame */
/* This is mainly used for completeness, but a stream error is if (!(h2c->flags &H2_CF_DEM_BLOCK_ANY))
* currently not set by h2c_dec_hdrs(). h2c->flags |= H2_CF_DEM_SHORT_READ;
*/ goto out;
TRACE_USER("Unrecoverable error decoding H2 request", H2_EV_RX_FRAME|H2_EV_RX_HDR|H2_EV_STRM_NEW|H2_EV_STRM_END, h2c->conn, 0, &rxbuf);
goto strm_err;
} }
if (error <= 0) { /* Now we cannot roll back and we won't come back here anymore for this
if (error == 0) { * stream, so this stream ID is open from a protocol perspective, even
/* Demux not blocked because of the stream, it is an incomplete frame */ * if incomplete or broken, we want to count it as attempted.
if (!(h2c->flags &H2_CF_DEM_BLOCK_ANY)) */
h2c->flags |= H2_CF_DEM_SHORT_READ; if (h2c->dsi > h2c->max_id)
goto out; // missing data h2c->max_id = h2c->dsi;
h2c->stream_cnt++;
if (error < 0) {
/* Failed to decode this stream. This might be due to a
* recoverable error affecting only the stream (e.g. too large
* request for buffer, that leaves the HPACK decompressor still
* synchronized), or a non-recoverable error such as an invalid
* frame type sequence (e.g. other frame type interleaved with
* CONTINUATION), in which h2c_dec_hdrs() has already set the
* error code in the connection and counted it in the relevant
* stats. We still count a req error in both cases.
*/
sess_log(h2c->conn->owner);
session_inc_http_req_ctr(h2c->conn->owner);
session_inc_http_err_ctr(h2c->conn->owner);
if (h2c->st0 >= H2_CS_ERROR) {
TRACE_USER("Unrecoverable error decoding H2 request", H2_EV_RX_FRAME|H2_EV_RX_HDR|H2_EV_STRM_NEW|H2_EV_STRM_END, h2c->conn, 0, &rxbuf);
goto out;
} }
/* Failed to decode this stream (e.g. too large request) /* recoverable stream error (e.g. too large request) */
* but the HPACK decompressor is still synchronized.
*/
TRACE_USER("rcvd unparsable H2 request", H2_EV_RX_FRAME|H2_EV_RX_HDR|H2_EV_STRM_NEW|H2_EV_STRM_END, h2c->conn, h2s, &rxbuf); TRACE_USER("rcvd unparsable H2 request", H2_EV_RX_FRAME|H2_EV_RX_HDR|H2_EV_STRM_NEW|H2_EV_STRM_END, h2c->conn, h2s, &rxbuf);
goto strm_err; goto strm_err;
} }
TRACE_USER("rcvd H2 request ", H2_EV_RX_FRAME|H2_EV_RX_HDR|H2_EV_STRM_NEW, h2c->conn, 0, &rxbuf); TRACE_USER("rcvd H2 request ", H2_EV_RX_FRAME|H2_EV_RX_HDR|H2_EV_STRM_NEW, h2c->conn, 0, &rxbuf);
/* Now we cannot roll back and we won't come back here anymore for this
* stream, this stream ID is open.
*/
if (h2c->dsi > h2c->max_id)
h2c->max_id = h2c->dsi;
h2c->stream_cnt++;
/* Note: we don't emit any other logs below because if we return /* Note: we don't emit any other logs below because if we return
* positively from h2c_frt_stream_new(), the stream will report the error, * positively from h2c_frt_stream_new(), the stream will report the error,
* and if we return in error, h2c_frt_stream_new() will emit the error. * and if we return in error, h2c_frt_stream_new() will emit the error.
@ -2883,19 +2891,8 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
goto leave; goto leave;
strm_err: strm_err:
sess_log(h2c->conn->owner);
session_inc_http_req_ctr(h2c->conn->owner);
session_inc_http_err_ctr(h2c->conn->owner);
h2s = (struct h2s*)h2_error_stream; h2s = (struct h2s*)h2_error_stream;
/* This stream ID is now opened anyway until we send the RST on
* it, it must not be reused.
*/
if (h2c->dsi > h2c->max_id)
h2c->max_id = h2c->dsi;
h2c->stream_cnt++;
send_rst: send_rst:
/* make the demux send an RST for the current stream. We may only /* make the demux send an RST for the current stream. We may only
* do this if we're certain that the HEADERS frame was properly * do this if we're certain that the HEADERS frame was properly