BUG/MEDIUM: mux-h2: fix read0 handling on partial frames

Since commit aade4edc1 ("BUG/MEDIUM: mux-h2: Don't handle pending read0
too early on streams"), we've met a few cases where an early connection
close wouldn't be properly handled if some data were pending in a frame
header, because the test now considers the buffer's contents before
accepting to report the close, but given that frame headers or preface
are consumed at once, the buffer cannot make progress when it's stuck
at intermediary lengths.

In order to address this, this patch introduces two flags in the h2c
connection to store any reported shutdown and failed parsing. The idea
is that we cannot rely on conn_xprt_read0_pending() in the parser since
it wouldn't consider data pending in the buffer nor intermediary layers,
but we know for certain that after a read0 is reported by the transport
layer in presence of an RD_SH on the connection, no more progress will
be made there. This alone is not sufficient to decide to end processing,
we can only do this once these final data have been submitted to a parser.
Therefore, now when a parser fails on missing data, we check if a read0
has already been reported on this connection, and if so we set a new
END_REACHED flag on the connection to indicate a failure to process the
final data. The h2c_read0_pending() function now simply reports this
flag's status. This way we're certain that the input shutdown is only
considered after the demux attempted to parse the last frame.

Maybe over the long term the subscribe() API should be improved to
synchronously fail when trying to subscribe for an even that will not
happen. This may be an elegant solution that could possibly work across
multiple layers and even muxes, and be usable at a few specific places
where that's needed.

Given the patch above was backported as far as 2.0, this one should be
backported there as well. It is possible that the fcgi mux has the same
issue, but this was not analysed yet.

Thanks to Pierre Cheynier for providing detailed traces allowing to
quickly narrow the problem down, and to Olivier for his analysis.

1 files changed, 34 insertions, 12 deletions

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 5d1475597..39d901650 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -64,7 +64,9 @@ static const struct h2s *h2_idle_stream;
 #define H2_CF_GOAWAY_FAILED     0x00002000  // a GOAWAY frame failed to be sent
 #define H2_CF_WAIT_FOR_HS       0x00004000  // We did check that at least a stream was waiting for handshake
 #define H2_CF_IS_BACK           0x00008000  // this is an outgoing connection
-#define H2_CF_WINDOW_OPENED     0x00010000 // demux increased window already advertised
+#define H2_CF_WINDOW_OPENED     0x00010000  // demux increased window already advertised
+#define H2_CF_RCVD_SHUT         0x00020000  // a recv() attempt already failed on a shutdown
+#define H2_CF_END_REACHED       0x00040000  // pending data too short with RCVD_SHUT present
 
 /* H2 connection state, in h2c->st0 */
 enum h2_cs {
@@ -657,15 +659,15 @@ static void h2_trace(enum trace_level level, uint64_t mask, const struct trace_s
 }
 
 
-/* Detect a pending read0 for a H2 connection. It happens if a read0 is pending
- * on the connection AND if there is no more data in the demux buffer. The
- * function returns 1 to report a read0 or 0 otherwise.
+/* Detect a pending read0 for a H2 connection. It happens if a read0 was
+ * already reported on a previous xprt->rcvbuf() AND a frame parser failed
+ * to parse pending data, confirming no more progress is possible because
+ * we're facing a truncated frame. The function returns 1 to report a read0
+ * or 0 otherwise.
  */
-static int h2c_read0_pending(struct h2c *h2c)
+static inline int h2c_read0_pending(struct h2c *h2c)
 {
-	if (conn_xprt_read0_pending(h2c->conn) && !b_data(&h2c->dbuf))
-		return 1;
-	return 0;
+	return !!(h2c->flags & H2_CF_END_REACHED);
 }
 
 /* returns true if the connection is allowed to expire, false otherwise. A
@@ -3371,6 +3373,8 @@ static void h2_process_demux(struct h2c *h2c)
 		/* error or missing data condition met above ? */
 		if (ret <= 0) {
 			TRACE_DEVEL("insufficient data to proceed", H2_EV_RX_FRAME, h2c->conn, h2s);
+			if (h2c->flags & H2_CF_RCVD_SHUT)
+				h2c->flags |= H2_CF_END_REACHED;
 			break;
 		}
 
@@ -3394,8 +3398,7 @@ static void h2_process_demux(struct h2c *h2c)
 		h2c_send_conn_wu(h2c);
 	}
 
- fail:
-	/* we can go here on missing data, blocked response or error */
+ done:
 	if (h2s && h2s->cs &&
 	    (b_data(&h2s->rxbuf) ||
 	     h2c_read0_pending(h2c) ||
@@ -3416,6 +3419,15 @@ static void h2_process_demux(struct h2c *h2c)
 	h2c_restart_reading(h2c, 0);
  out:
 	TRACE_LEAVE(H2_EV_H2C_WAKE, h2c->conn);
+	return;
+
+ fail:
+	/* we can go here on missing data, blocked response or error, but we
+	 * need to check if we've met a short read condition.
+	 */
+	if (h2c->flags & H2_CF_RCVD_SHUT)
+		h2c->flags |= H2_CF_END_REACHED;
+	goto done;
 }
 
 /* resume each h2s eligible for sending in list head <head> */
@@ -3550,6 +3562,11 @@ static int h2_recv(struct h2c *h2c)
 		return 0;
 	}
 
+	if (h2c->flags & H2_CF_RCVD_SHUT) {
+		TRACE_DEVEL("leaving on rcvd_shut", H2_EV_H2C_RECV, h2c->conn);
+		return 0;
+	}
+
 	b_realign_if_empty(buf);
 	if (!b_data(buf)) {
 		/* try to pre-align the buffer like the
@@ -3569,8 +3586,13 @@ static int h2_recv(struct h2c *h2c)
 	ret = max ? conn->xprt->rcv_buf(conn, conn->xprt_ctx, buf, max, 0) : 0;
 
 	if (max && !ret && h2_recv_allowed(h2c)) {
-		TRACE_DATA("failed to receive data, subscribing", H2_EV_H2C_RECV, h2c->conn);
-		conn->xprt->subscribe(conn, conn->xprt_ctx, SUB_RETRY_RECV, &h2c->wait_event);
+		if (conn_xprt_read0_pending(h2c->conn)) {
+			TRACE_DATA("received read0", H2_EV_H2C_RECV, h2c->conn);
+			h2c->flags |= H2_CF_RCVD_SHUT;
+		} else {
+			TRACE_DATA("failed to receive data, subscribing", H2_EV_H2C_RECV, h2c->conn);
+			conn->xprt->subscribe(conn, conn->xprt_ctx, SUB_RETRY_RECV, &h2c->wait_event);
+		}
 	} else if (ret)
 		TRACE_DATA("received data", H2_EV_H2C_RECV, h2c->conn, 0, 0, (void*)(long)ret);