From 927b88ba00c41212bb5352e655623562593f015e Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Mon, 4 Mar 2019 08:03:25 +0100 Subject: [PATCH] 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. --- src/mux_h2.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/mux_h2.c b/src/mux_h2.c index ebd4fe838..b641d2226 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -1938,7 +1938,8 @@ static struct h2s *h2c_frt_handle_headers(struct h2c *h2c, struct h2s *h2s) 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 @@ static struct h2s *h2c_bck_handle_headers(struct h2c *h2c, struct h2s *h2s) 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 @@ static int h2c_frt_handle_data(struct h2c *h2c, struct h2s *h2s) 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 @@ try_again: 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 @@ static size_t h2s_frt_make_resp_headers(struct h2s *h2s, const struct buffer *bu } /* 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 @@ static size_t h2s_htx_frt_make_resp_headers(struct h2s *h2s, struct htx *htx) (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 @@ static size_t h2s_htx_bck_make_req_headers(struct h2s *h2s, struct htx *htx) 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 */