From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Fri, 21 Jun 2024 12:45:04 +0000 (+0200)
Subject: BUG/MINOR: h3: fix crash on STOP_SENDING receive after GOAWAY emission
X-Git-Tag: v3.1-dev2~29
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=85838822ba37a92b2dcc43205a07c2b33208b985;p=thirdparty%2Fhaproxy.git

BUG/MINOR: h3: fix crash on STOP_SENDING receive after GOAWAY emission

After emitting a HTTP/3 GOAWAY frame, opening of streams higher than
advertised ID was prevented. h3_attach operation would return success
but without allocating H3S stream context for QCS. In addition, the
stream would be immediately scheduled for RESET_STREAM emission.

Despite the immediate stream close, the current is not sufficient enough
and can cause crashes. When of this occurence can be found if
STOP_SENDING is the first frame received for a stream. A crash would
occur under qcc_recv_stop_sending() after h3_attach invokation, when
h3_close() is used which try to access to H3S context.

To fix this, change h3_attach API. In case of success, H3S stream
context is always allocated, even if the stream will be scheduled for
immediate close. This renders the code more reliable.

This crash should be extremely rare, as it can only happen after GOAWAY
emission, which is only used on soft-stop or reload.

This should solve the second crash occurence reported on GH #2607.

This must be backported up to 2.8.
---

diff --git a/src/h3.c b/src/h3.c
index 321d883a77..3c9463d922 100644
--- a/src/h3.c
+++ b/src/h3.c
@@ -2200,6 +2200,11 @@ static int h3_close(struct qcs *qcs, enum qcc_app_ops_close_side side)
 	return 0;
 }
 
+/* Allocates HTTP/3 stream context relative to <qcs>. If the operation cannot
+ * be performed, an error is returned and <qcs> context is unchanged.
+ *
+ * Returns 0 on success else non-zero.
+ */
 static int h3_attach(struct qcs *qcs, void *conn_ctx)
 {
 	struct h3c *h3c = conn_ctx;
@@ -2207,27 +2212,6 @@ static int h3_attach(struct qcs *qcs, void *conn_ctx)
 
 	TRACE_ENTER(H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
 
-	/* RFC 9114 5.2. Connection Shutdown
-	 *
-	 * Upon sending
-	 * a GOAWAY frame, the endpoint SHOULD explicitly cancel (see
-	 * Sections 4.1.1 and 7.2.3) any requests or pushes that have
-	 * identifiers greater than or equal to the one indicated, in
-	 * order to clean up transport state for the affected streams.
-	 * The endpoint SHOULD continue to do so as more requests or
-	 * pushes arrive.
-	 */
-	if (h3c->flags & H3_CF_GOAWAY_SENT && qcs->id >= h3c->id_goaway &&
-	    quic_stream_is_bidi(qcs->id)) {
-		/* Reject request and do not allocate a h3s context.
-		 * TODO support push uni-stream rejection.
-		 */
-		TRACE_STATE("reject stream higher than goaway", H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
-		qcc_abort_stream_read(qcs);
-		qcc_reset_stream(qcs, H3_ERR_REQUEST_REJECTED);
-		goto done;
-	}
-
 	h3s = pool_alloc(pool_head_h3s);
 	if (!h3s) {
 		TRACE_ERROR("h3s allocation failure", H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
@@ -2254,7 +2238,25 @@ static int h3_attach(struct qcs *qcs, void *conn_ctx)
 		h3s->type = H3S_T_UNKNOWN;
 	}
 
- done:
+	/* RFC 9114 5.2. Connection Shutdown
+	 *
+	 * Upon sending
+	 * a GOAWAY frame, the endpoint SHOULD explicitly cancel (see
+	 * Sections 4.1.1 and 7.2.3) any requests or pushes that have
+	 * identifiers greater than or equal to the one indicated, in
+	 * order to clean up transport state for the affected streams.
+	 * The endpoint SHOULD continue to do so as more requests or
+	 * pushes arrive.
+	 */
+	if (h3c->flags & H3_CF_GOAWAY_SENT && qcs->id >= h3c->id_goaway &&
+	    quic_stream_is_bidi(qcs->id)) {
+		TRACE_STATE("close stream outside of goaway range", H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
+		qcc_abort_stream_read(qcs);
+		qcc_reset_stream(qcs, H3_ERR_REQUEST_REJECTED);
+	}
+
+	/* TODO support push uni-stream rejection. */
+
 	TRACE_LEAVE(H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
 	return 0;