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

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.

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 8847e06c5a4002e58210672caed5755b34921cdf..cd88c887486ab18250d4bd24f2a40bea061bc16e 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -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);
 
-       /* unrecoverable error ? */
-       if (h2c->st0 >= H2_CS_ERROR) {
-               /* This is mainly used for completeness, but a stream error is
-                * currently not set by h2c_dec_hdrs().
-                */
-               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) {
+               /* No error but missing data for demuxing, it is an incomplete frame */
+               if (!(h2c->flags &H2_CF_DEM_BLOCK_ANY))
+                       h2c->flags |= H2_CF_DEM_SHORT_READ;
+               goto out;
        }
 
-       if (error <= 0) {
-               if (error == 0) {
-                       /* Demux not blocked because of the stream, it is an incomplete frame */
-                       if (!(h2c->flags &H2_CF_DEM_BLOCK_ANY))
-                               h2c->flags |= H2_CF_DEM_SHORT_READ;
-                       goto out; // missing data
-               }
+       /* Now we cannot roll back and we won't come back here anymore for this
+        * stream, so this stream ID is open from a protocol perspective, even
+        * if incomplete or broken, we want to count it as attempted.
+        */
+       if (h2c->dsi > h2c->max_id)
+               h2c->max_id = h2c->dsi;
+       h2c->stream_cnt++;
 
-               /* Failed to decode this stream (e.g. too large request)
-                * but the HPACK decompressor is still synchronized.
+       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;
+               }
+
+               /* recoverable stream error (e.g. too large request) */
                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;
        }
 
        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
         * 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.
@@ -2883,19 +2891,8 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
        goto leave;
 
  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;
 
-       /* 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:
        /* 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