BUG/MEDIUM: h2: properly handle and report some stream errors
Some stream errors applied to half-closed and closed streams are not
properly reported, especially after the stream transistions to the
closed state. The reason is that the code checks for this "error"
stream state in order to send an RST frame. But if the stream was
just closed or was already closed, there's no way to validate this
condition, and the error is never reported to the peer.
In order to address this situation, we'll add a new FRAME_E demux state
which indicates that the previously parsed frame triggered a stream error
of type STREAM CLOSED that needs to be reported. Proceeding like this
will ensure that we don't lose that information even if we can't
immediately send the message. It also removes the confusion where FRAME_A
could be used either for ACKs or for RST.
The state transition has been added after every h2s_error() on the demux
path. It seems that we might need to have two distinct h2s_error()
functions, one for the mux and another one for the demux, though it
would provide little benefit. It also becomes more apparent that the
H2_SS_ERROR state is only used to detect the need to report an error
on the mux direction. Maybe this will have to be revisited later.
This simple change managed to eliminate 5 bugs reported by h2spec.
This fix must be backported to 1.8.
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 4ee20b4..18d4562 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -67,7 +67,8 @@
H2_CS_SETTINGS1, // preface OK, waiting for first settings frame
H2_CS_FRAME_H, // first settings frame ok, waiting for frame header
H2_CS_FRAME_P, // frame header OK, waiting for frame payload
- H2_CS_FRAME_A, // frame payload OK, trying to send ACK/RST frame
+ H2_CS_FRAME_A, // frame payload OK, trying to send ACK frame
+ H2_CS_FRAME_E, // frame payload OK, trying to send RST frame
H2_CS_ERROR, // send GOAWAY(errcode) and close the connection ASAP
H2_CS_ERROR2, // GOAWAY(errcode) sent, close the connection ASAP
H2_CS_ENTRIES // must be last
@@ -1429,7 +1430,7 @@
strm_err:
if (h2s) {
h2s_error(h2s, error);
- h2c->st0 = H2_CS_FRAME_A;
+ h2c->st0 = H2_CS_FRAME_E;
}
else
h2c_error(h2c, error);
@@ -1613,7 +1614,7 @@
if (h2s->st >= H2_SS_ERROR) {
/* stream error : send RST_STREAM */
- h2c->st0 = H2_CS_FRAME_A;
+ h2c->st0 = H2_CS_FRAME_E;
}
else {
/* update the max stream ID if the request is being processed */
@@ -1630,7 +1631,7 @@
strm_err:
if (h2s) {
h2s_error(h2s, error);
- h2c->st0 = H2_CS_FRAME_A;
+ h2c->st0 = H2_CS_FRAME_E;
}
else
h2c_error(h2c, error);
@@ -1689,7 +1690,7 @@
if (h2s->st >= H2_SS_ERROR) {
/* stream error : send RST_STREAM */
- h2c->st0 = H2_CS_FRAME_A;
+ h2c->st0 = H2_CS_FRAME_E;
}
/* check for completion : the callee will change this to FRAME_A or
@@ -1714,7 +1715,7 @@
strm_err:
if (h2s) {
h2s_error(h2s, error);
- h2c->st0 = H2_CS_FRAME_A;
+ h2c->st0 = H2_CS_FRAME_E;
}
else
h2c_error(h2c, error);
@@ -1807,6 +1808,9 @@
h2_skip_frame_hdr(h2c->dbuf);
}
+ if (h2c->st0 == H2_CS_FRAME_E)
+ goto strm_err;
+
/* Only H2_CS_FRAME_P and H2_CS_FRAME_A here */
h2s = h2c_st_by_id(h2c, h2c->dsi);
@@ -1826,6 +1830,7 @@
* this state MUST be treated as a stream error
*/
h2s_error(h2s, H2_ERR_STREAM_CLOSED);
+ h2c->st0 = H2_CS_FRAME_E;
goto strm_err;
}
@@ -1878,8 +1883,10 @@
* we have nowhere to store the partial HEADERS frame.
* Let's abort the stream on an INTERNAL_ERROR here.
*/
- if (h2c->st0 == H2_CS_FRAME_P)
+ if (h2c->st0 == H2_CS_FRAME_P) {
h2s_error(h2s, H2_ERR_INTERNAL_ERROR);
+ h2c->st0 = H2_CS_FRAME_E;
+ }
break;
case H2_FT_HEADERS:
@@ -1928,14 +1935,12 @@
}
strm_err:
- /* RST are sent similarly to frame acks */
- if (h2s->st == H2_SS_ERROR) {
- if (h2c->st0 == H2_CS_FRAME_P)
- h2c->st0 = H2_CS_FRAME_A;
+ /* We may have to send an RST if not done yet */
+ if (h2s->st == H2_SS_ERROR)
+ h2c->st0 = H2_CS_FRAME_E;
- if (h2c->st0 == H2_CS_FRAME_A)
- ret = h2c_send_rst_stream(h2c, h2s);
- }
+ if (h2c->st0 == H2_CS_FRAME_E)
+ ret = h2c_send_rst_stream(h2c, h2s);
/* error or missing data condition met above ? */
if (ret <= 0)
@@ -2518,6 +2523,7 @@
if (!h2c->dfl) {
h2s_error(h2s, H2_ERR_PROTOCOL_ERROR); // empty headers frame!
+ h2c->st0 = H2_CS_FRAME_E;
return 0;
}