From: Willy Tarreau <w@1wt.eu>
Date: Mon, 6 May 2019 09:23:29 +0000 (+0200)
Subject: BUG/MEDIUM: mux-h2/htx: never wait for EOM when processing trailers
X-Git-Tag: v2.0-dev3~94
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fb07b3f82536aea25177c527b1002c2dfce19261;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: mux-h2/htx: never wait for EOM when processing trailers

In message https://www.mail-archive.com/haproxy@formilux.org/msg33541.html
Patrick Hemmer reported an interesting bug affecting H2 and trailers.

The problem is that in order to close the stream we have to see the EOM
block, but nothing guarantees it will atomically be delivered with the
trailers block(s). So the code currently waits for it by returning zero
when it was not found, resulting in the caller (h2_snd_buf()) to loop
forever calling it again.

The current internal connection/connstream API doesn't allow a send
actor to notify its caller that it cannot process the data until it
gets more, so even returning zero will only lead to calls in loops
without any guarantee that any progress will be made.

Some late amendments to HTX already guaranteed the atomicity of the
trailers block during snd_buf(), which is currently ensured by the
fact that producers create exactly one such trailers block for all
trailers. So in practice we can only loop between trailers and EOM.

This patch changes the behaviour by making h2s_htx_make_trailers()
become atomic by not consuming the EOM block. This way either it finds
the end of trailers marker (empty line) or it fails. Once it sends the
trailers block, ES is set so the stream turns HLOC or CLOSED. Thanks
to previous patch "MEDIUM: mux-h2: discard contents that are to be sent
after a shutdown" is is now safe to interrupt outgoing data processing,
and the late EOM block will silently be discarded when the caller
finally sends it.

This is a bit tricky but should remain solid by design, and seems like
the only option we have that is compatible with 1.9, where it must be
backported along with the aforementioned patch.
---

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 1f0121c5ef..91e826db74 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4985,7 +4985,7 @@ static size_t h2s_htx_frt_make_resp_data(struct h2s *h2s, struct buffer *buf, si
  * which might have happened subsequently to a successful send. The htx blocks
  * are automatically removed from the message. The htx message is assumed to be
  * valid since produced from the internal code. Processing stops when meeting
- * the EOM, which is also removed. All trailers are processed at once and sent
+ * the EOM, which is *not* removed. All trailers are processed at once and sent
  * as a single frame. The ES flag is always set.
  */
 static size_t h2s_htx_make_trailers(struct h2s *h2s, struct htx *htx)
@@ -5029,11 +5029,8 @@ static size_t h2s_htx_make_trailers(struct h2s *h2s, struct htx *htx)
 		if (type == HTX_BLK_UNUSED)
 			continue;
 
-		if (type != HTX_BLK_TLR) {
-			if (type == HTX_BLK_EOM)
-				blk_end = blk;
+		if (type != HTX_BLK_TLR)
 			break;
-		}
 
 		if (unlikely(hdr >= sizeof(list)/sizeof(list[0]) - 1))
 			goto fail;
@@ -5063,8 +5060,8 @@ static size_t h2s_htx_make_trailers(struct h2s *h2s, struct htx *htx)
 		}
 	}
 
-	if (!blk_end)
-		goto end; // end not found yet
+	if (list[hdr].n.len != 0)
+		goto fail; // empty trailer not found: internal error
 
 	chunk_reset(&outbuf);
 
@@ -5135,7 +5132,7 @@ static size_t h2s_htx_make_trailers(struct h2s *h2s, struct htx *htx)
 
 	/* OK we could properly deliver the response */
  done:
-	/* remove all header blocks including EOM and compute the corresponding size. */
+	/* remove all header blocks till the end and compute the corresponding size. */
 	ret = 0;
 	idx = htx_get_head(htx);
 	blk = htx_get_blk(htx, idx);
@@ -5143,7 +5140,6 @@ static size_t h2s_htx_make_trailers(struct h2s *h2s, struct htx *htx)
 		ret += htx_get_blksz(blk);
 		blk = htx_remove_blk(htx, blk);
 	}
-	blk = htx_remove_blk(htx, blk);
  end:
 	return ret;
  full: