BUG/MEDIUM: h2: never leave pending data in the output buffer on close
We currently don't process trailers on H2, but this has an impact : on
chunked HTTP/1 responses, we decide to emit the ES bit once we see the
0CRLF. From this point the stream switches to the CLOSED state, which
aborts processing of the remaining bytes. Thus the extra CRLF which ends
trailers is not processed and remains in the buffer. This prevents the
stream from being notified about end of transmission, which in turn keeps
the mux busy and prevents the connection from quitting.
The case of the trailers is not the root cause of this issue, though it
is what triggers it. The root cause is that upon error and/or close, once
we know we're not going to process any more data, we must absolutely flush
any remaining bytes from the output buffer, otherwise there is no way the
stream can quit. This is what this patch does.
It looks very likely related to the issues reported and debugged by
Janusz Dziemidowicz and Milan Petruzelka.
One way to reproduce it is to chain two proxies with the last one emitting
chunked data (typically using the stats page) :
global
stats socket /tmp/sock1 mode 666 level admin
stats timeout 1h
tune.ssl.default-dh-param 1024
tune.bufsize 16384
defaults
mode http
timeout connect 4s
timeout client 10s
timeout server 20s
listen px1
bind :4443 ssl crt rsa+dh2048.pem npn h2 alpn h2
server s1 127.0.0.1:4445
listen px2
bind :4444 ssl crt rsa+dh2048.pem npn h2 alpn h2
bind :4445
stats uri /
Then use curl to fetch the stats through px1 :
curl --http2 -k "https://127.0.0.1:4443/"
When curl is sent to the first one, "show sess" issued to the CLI will
show a remaining session during the client timeout. When curl is aimed at
port 4444 (px2), there is no such remaining session.
This fix needs to be backported to 1.8.
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 0d0101e..47df89e 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3302,6 +3302,14 @@
/* we may need to add END_STREAM */
/* FIXME: we should also detect shutdown(w) below, but how ? Maybe we
* could rely on the MSG_MORE flag as a hint for this ?
+ *
+ * FIXME: what we do here is not correct because we send end_stream
+ * before knowing if we'll have to send a HEADERS frame for the
+ * trailers. More importantly we're not consuming the trailing CRLF
+ * after the end of trailers, so it will be left to the caller to
+ * eat it. The right way to do it would be to measure trailers here
+ * and to send ES only if there are no trailers.
+ *
*/
if (((h1m->flags & H1_MF_CLEN) && !(h1m->curr_len - size)) ||
!h1m->curr_len || h1m->state >= HTTP_MSG_DONE)
@@ -3404,6 +3412,13 @@
}
}
+ if (h2s->st >= H2_SS_ERROR) {
+ /* trim any possibly pending data after we close (extra CR-LF,
+ * unprocessed trailers, abnormal extra data, ...)
+ */
+ bo_del(buf, buf->o);
+ }
+
/* RST are sent similarly to frame acks */
if (h2s->st == H2_SS_ERROR || h2s->flags & H2_SF_RST_RCVD) {
cs->flags |= CS_FL_ERROR;