BUG/MAJOR: mux-h2: fix race condition between close on both ends
A crash in H2 was reported in issue #52. It turns out that there is a
small but existing race by which a conn_stream could detach itself
using h2_detach(), not being able to destroy the h2s due to pending
output data blocked by flow control, then upon next h2s activity
(transfer_data or trailers parsing), an ES flag may need to be turned
into a CS_FL_REOS bit, causing a dereference of a NULL stream. This is
a side effect of the fact that we still have a few places which
incorrectly depend on the CS flags, while these flags should only be
set by h2_rcv_buf() and h2_snd_buf().
All candidate locations along this path have been secured against this
risk, but the code should really evolve to stop depending on CS anymore.
This fix must be backported to 1.9 and possibly partially to 1.8.
diff --git a/src/mux_h2.c b/src/mux_h2.c
index ebd4fe8..b641d22 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -1938,7 +1938,8 @@
h2s->st = H2_SS_HREM;
else
h2s_close(h2s);
- h2s->cs->flags |= CS_FL_REOS;
+ if (h2s->cs)
+ h2s->cs->flags |= CS_FL_REOS;
}
/* update the max stream ID if the request is being processed */
@@ -2005,14 +2006,15 @@
if (h2c->dff & H2_F_HEADERS_END_STREAM) {
h2s->flags |= H2_SF_ES_RCVD;
- h2s->cs->flags |= CS_FL_REOS;
+ if (h2s->cs)
+ h2s->cs->flags |= CS_FL_REOS;
}
- if (h2s->cs->flags & CS_FL_ERROR && h2s->st < H2_SS_ERROR)
+ if (h2s->cs && h2s->cs->flags & CS_FL_ERROR && h2s->st < H2_SS_ERROR)
h2s->st = H2_SS_ERROR;
- else if (h2s->cs->flags & CS_FL_REOS && h2s->st == H2_SS_OPEN)
+ else if (h2s->cs && h2s->cs->flags & CS_FL_REOS && h2s->st == H2_SS_OPEN)
h2s->st = H2_SS_HREM;
- else if (h2s->cs->flags & CS_FL_REOS && h2s->st == H2_SS_HLOC)
+ else if ((!h2s || h2s->cs->flags & CS_FL_REOS) && h2s->st == H2_SS_HLOC)
h2s_close(h2s);
return h2s;
@@ -2082,7 +2084,8 @@
h2s_close(h2s);
h2s->flags |= H2_SF_ES_RCVD;
- h2s->cs->flags |= CS_FL_REOS;
+ if (h2s->cs)
+ h2s->cs->flags |= CS_FL_REOS;
if (h2s->flags & H2_SF_DATA_CLEN && h2s->body_len) {
/* RFC7540#8.1.2 */
@@ -3694,7 +3697,8 @@
if (h2c->dff & H2_F_DATA_END_STREAM) {
h2s->flags |= H2_SF_ES_RCVD;
- h2s->cs->flags |= CS_FL_REOS;
+ if (h2s->cs)
+ h2s->cs->flags |= CS_FL_REOS;
}
if (htx)
htx_to_buf(htx, csbuf);
@@ -3826,7 +3830,7 @@
}
/* we may need to add END_STREAM */
- if (((h1m->flags & H1_MF_CLEN) && !h1m->body_len) || h2s->cs->flags & CS_FL_SHW)
+ if (((h1m->flags & H1_MF_CLEN) && !h1m->body_len) || !h2s->cs || h2s->cs->flags & CS_FL_SHW)
es_now = 1;
/* update the frame's size */
@@ -4286,7 +4290,7 @@
(h2s->status >= 200 || h2s->status == 101))
es_now = 1;
- if (h2s->cs->flags & CS_FL_SHW)
+ if (!h2s->cs || h2s->cs->flags & CS_FL_SHW)
es_now = 1;
/* update the frame's size */
@@ -4534,7 +4538,7 @@
if (sl->flags & HTX_SL_F_BODYLESS)
es_now = 1;
- if (h2s->cs->flags & CS_FL_SHW)
+ if (!h2s->cs || h2s->cs->flags & CS_FL_SHW)
es_now = 1;
/* update the frame's size */