BUG/MEDIUM: mux-h2: decode trailers in HEADERS frames

This is not exactly a bug but a long-time design limitation. We used not
to decode trailers in H2, resulting in broken connections each time a
trailer was sent, since it was impossible to keep the HPACK decompressor
synchronized. Now that the sequencing of operations permits it, we must
make sure to at least properly decode them.

What we try to do is to identify if a HEADERS frame was already seen and
use this indication to know if it's a headers or a trailers. For this,
h2c_decode_headers() checks if the stream indicates that a HEADERS frame
was already received. If so, it decodes it and emits the trailing
0 CRLF CRLF in case of H1, or the HTX_EOD + HTX_EOM blocks in case of HTX,
to terminate the data stream.

The trailers contents are still deleted for now but the request works, and
the connection remains synchronized and usable for subsequent streams.

The correctness may be tested using a simple config and h2spec :

    h2spec -o 1000 -v -t -S -k -h 127.0.0.1 -p 4443 generic/4/4

This should definitely be backported to 1.9 given the low impact for the
benefit. However it cannot be backported to 1.8 since the operations cannot
be resumed. The following patches are also needed with this one :

   MINOR: mux-h2: make h2c_decode_headers() return a status, not a count
   MINOR: mux-h2: add a new dummy stream : h2_error_stream
   MEDIUM: mux-h2: make h2c_decode_headers() support recoverable errors
   BUG/MINOR: mux-h2: detect when the HTX EOM block cannot be added after headers
   MINOR: mux-h2: check for too many streams only for idle streams
   MINOR: mux-h2: set H2_SF_HEADERS_RCVD when a HEADERS frame was decoded

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 6b79e0ceefea26d769912f3301c8d01b0c9fc61e..9f0b89dbc9591a78713a7f9fd7fb8c5910476ccd 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -1863,9 +1863,12 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
 
        /* now either the frame is complete or the buffer is complete */
        if (h2s->st != H2_SS_IDLE) {
-               /* FIXME: stream already exists, this is only allowed for
-                * trailers (not supported for now).
-                */
+               /* The stream exists/existed, this must be a trailers frame */
+               if (h2s->st != H2_SS_CLOSED) {
+                       if (!h2c_decode_headers(h2c, &h2s->rxbuf, &h2s->flags))
+                               goto out;
+                       goto done;
+               }
                error = H2_ERR_PROTOCOL_ERROR;
                sess_log(h2c->conn->owner);
                goto conn_err;
@@ -1910,6 +1913,7 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s)
        h2s->rxbuf = rxbuf;
        h2s->flags |= flags;
 
+ done:
        if (h2c->dff & H2_F_HEADERS_END_STREAM)
                h2s->flags |= H2_SF_ES_RCVD;
 
@@ -3209,6 +3213,10 @@ static void h2_shutw(struct conn_stream *cs, enum cs_shw_mode mode)
  *   - H2_SF_DATA_CLEN when content-length is seen
  *   - H2_SF_DATA_CHNK when chunking should be used for the H1 conversion
  *   - H2_SF_HEADERS_RCVD once the frame is successfully decoded
+ *
+ * The H2_SF_HEADERS_RCVD flag is also looked at in the <flags> field prior to
+ * decoding, in order to detect if we're dealing with a headers or a trailers
+ * block (the trailers block appears after H2_SF_HEADERS_RCVD was seen).
  */
 static int h2c_decode_headers(struct h2c *h2c, struct buffer *rxbuf, uint32_t *flags)
 {
@@ -3358,6 +3366,10 @@ next_frame:
        /* OK now we have our header list in <list> */
        msgf = (h2c->dff & H2_F_HEADERS_END_STREAM) ? 0 : H2_MSGF_BODY;
 
+       if (*flags & H2_SF_HEADERS_RCVD)
+               goto trailers;
+
+       /* This is the first HEADERS frame so it's a headers block */
        if (htx) {
                /* HTX mode */
                if (h2c->flags & H2_CF_IS_BACK)
@@ -3384,12 +3396,24 @@ next_frame:
                        *flags |= H2_SF_DATA_CHNK;
        }
 
+ done:
        /* indicate that a HEADERS frame was received for this stream */
        *flags |= H2_SF_HEADERS_RCVD;
 
-       if (htx && h2c->dff & H2_F_HEADERS_END_STREAM)
-               if (!htx_add_endof(htx, HTX_BLK_EOM))
-                       goto fail;
+       if (h2c->dff & H2_F_HEADERS_END_STREAM) {
+               /* Mark the end of message, either using EOM in HTX or with the
+                * trailing CRLF after the end of trailers. Note that DATA_CHNK
+                * is not set during headers with END_STREAM.
+                */
+               if (htx) {
+                       if (!htx_add_endof(htx, HTX_BLK_EOM))
+                               goto fail;
+               }
+               else if (*flags & H2_SF_DATA_CHNK) {
+                       if (!b_putblk(rxbuf, "\r\n", 2))
+                               goto fail;
+               }
+       }
 
        /* success */
        ret = 1;
@@ -3419,6 +3443,36 @@ next_frame:
  fail:
        ret = -1;
        goto leave;
+
+ trailers:
+       /* This is the last HEADERS frame hence a trailer */
+
+       if (!(h2c->dff & H2_F_HEADERS_END_STREAM)) {
+               /* It's a trailer but it's missing ES flag */
+               h2c_error(h2c, H2_ERR_PROTOCOL_ERROR);
+               goto fail;
+       }
+
+       /* Trailers terminate a DATA sequence. In HTX we have to emit an EOD
+        * block, and when using chunks we must send the 0 CRLF marker. For
+        * other modes, the trailers are silently dropped.
+        */
+       if (htx) {
+               if (!htx_add_endof(htx, HTX_BLK_EOD))
+                       goto fail;
+               /* FIXME: emit the decoded trailers here. EOM will be sent
+                * when leaving.
+                */
+       }
+       else if (*flags & H2_SF_DATA_CHNK) {
+               /* Legacy mode with chunked encoding : we must finalize the
+                * data block message emit the trailing CRLF */
+               if (!b_putblk(rxbuf, "0\r\n", 3))
+                       goto fail;
+               /* FIXME: emit the decoded trailers here */
+       }
+
+       goto done;
 }
 
 /* Transfer the payload of a DATA frame to the HTTP/1 side. When content-length