BUG/MINOR: mux-h2: also count streams for refused ones

There are a few places where we can reject an incoming stream based on
technical errors such as decoded headers that are too large for the
internal buffers, or memory allocation errors. In this case we send
an RST_STREAM to abort the request, but the total stream counter was
not incremented. That's not really a problem, until one starts to try
to enforce a total stream limit using tune.h2.fe.max-total-streams,
and which will not count such faulty streams. Typically a client that
learns too large cookies and tries to replay them in a way that
overflows the maximum buffer size would be rejected and depending on
how they're implemented, they might retry forever.

This patch removes the stream count increment from h2s_new() and moves
it instead to the calling functions, so that it translates the decision
to process a new stream instead of a successfully decoded stream. The
result is that such a bogus client will now be blocked after reaching
the total stream limit.

This can be validated this way:

  global
        tune.h2.fe.max-total-streams 128
        expose-experimental-directives
        trace h2 sink stdout
        trace h2 level developer
        trace h2 verbosity complete
        trace h2 start now

  frontend h
        bind :8080
        mode http
        redirect location /

Sending this will fill frames with 15972 bytes of cookie headers that
expand to 16500 for storage+index once decoded, causing "message too large"
events:

  (dev/h2/mkhdr.sh -t p;dev/h2/mkhdr.sh -t s;
   for sid in {0..1000}; do
     dev/h2/mkhdr.sh  -t h -i $((sid*2+1)) -f es,eh \
       -R "828684410f7777772e6578616d706c652e636f6d \
           $(for i in {1..66}; do
             echo -n 60 7F 73 433d $(for j in {1..24}; do
               echo -n 2e313233343536373839; done);
            done) ";
   done) | nc 0 8080

Now it properly stops after sending 128 streams.

This may be backported wherever commit 983ac4397 ("MINOR: mux-h2:
support limiting the total number of H2 streams per connection") is
present, since without it, that commit is less effective.

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 8c352faa42100b9cc4d4b8a67319451b829404e9..8847e06c5a4002e58210672caed5755b34921cdf 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -1558,7 +1558,6 @@ static struct h2s *h2s_new(struct h2c *h2c, int id)
 
        eb32_insert(&h2c->streams_by_id, &h2s->by_id);
        h2c->nb_streams++;
-       h2c->stream_cnt++;
 
        HA_ATOMIC_INC(&h2c->px_counters->open_streams);
        HA_ATOMIC_INC(&h2c->px_counters->total_streams);
@@ -1693,6 +1692,9 @@ static struct h2s *h2c_bck_stream_new(struct h2c *h2c, struct stconn *sc, struct
        h2s->sess = sess;
        h2c->nb_sc++;
 
+       /* on the backend we can afford to only count total streams upon success */
+       h2c->stream_cnt++;
+
  out:
        if (likely(h2s))
                TRACE_LEAVE(H2_EV_H2S_NEW, h2c->conn, h2s);
@@ -2812,11 +2814,11 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
 
        /* 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);
-               sess_log(h2c->conn->owner);
-               session_inc_http_req_ctr(h2c->conn->owner);
-               session_inc_http_err_ctr(h2c->conn->owner);
-               goto out;
+               goto strm_err;
        }
 
        if (error <= 0) {
@@ -2830,20 +2832,8 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
                /* Failed to decode this stream (e.g. too large request)
                 * but the HPACK decompressor is still synchronized.
                 */
-               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;
-
                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 send_rst;
+               goto strm_err;
        }
 
        TRACE_USER("rcvd H2 request  ", H2_EV_RX_FRAME|H2_EV_RX_HDR|H2_EV_STRM_NEW, h2c->conn, 0, &rxbuf);
@@ -2853,6 +2843,7 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
         */
        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,
@@ -2891,6 +2882,20 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
        h2s = NULL;
        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