BUG/MINOR: mux-h2: mark the stream as open before processing it not after

When a client doesn't respect the h2 MAX_CONCURRENT_STREAMS setting, we
rightfully send RST_STREAM to it so that the client closes. But the
max_id is only updated on the successful path of h2c_handle_stream_new(),
which may be reentered for partial frames or CONTINUATION frames, and as
a result we don't increment it if an extraneous stream ID is rejected.

Normally it doesn't have any consequence. But on a POST it can have some
if the DATA frame immediately follows the faulty HEADERS frame: with
max_id not incremented, the stream remains in IDLE state, and the DATA
frame now lands in an invalid state from a protocol's perspective, which
must lead to a connection error instead of a stream error.

This can be tested by modifying the code to send an arbitrarily large
MAX_CONCURRENT_STREAM setting and using h2load to send more concurrent
streams than configured: with a GET, only a tiny fraction of them will
report an error (e.g. 101 streams for 100 accepted will result in ~1%
failure), but when sending data, most of the streams will be reported
as failed because the connection will be closed. By updating the max_id
earlier, the stream is now considered as closed when the DATA frame
arrives and it's silently discarded.

This must be backported to all versions but only if the code is exactly
the same. Under no circumstance this ID may be updated for a partial frame
(i.e. only update it before or just after calling h2c_frt_steam_new()).

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 20193d4e6435f3cdae0566131b1b1cc5b705d175..3d04b8e187543d450185e3c1110ba1e7f5fee291 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -2840,6 +2840,12 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
 
        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;
+
        /* Note: we don't emit any other logs below because ff 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.
@@ -2867,11 +2873,6 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
                else
                        h2s_close(h2s);
        }
-
-       /* update the max stream ID if the request is being processed */
-       if (h2s->id > h2c->max_id)
-               h2c->max_id = h2s->id;
-
        return h2s;
 
  conn_err: