From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Fri, 3 Jan 2025 15:25:14 +0000 (+0100)
Subject: BUG/MINOR: mux-quic: handle closure of uni-stream
X-Git-Tag: v3.2-dev3~11
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=801e39e1ccfc7404354d6870e779f83351593df0;p=thirdparty%2Fhaproxy.git

BUG/MINOR: mux-quic: handle closure of uni-stream

This commit is a direct follow-up to the previous one. As already
described, a previous fix was merged to prevent streamdesc attach
operation on already completed QCS instances scheduled for purging. This
was implemented by skipping app proto decoding.

However, this has a bad side-effect for remote uni-directional stream.
If receiving a FIN stream frame on such a stream, it will considered as
complete because streamdesc are never attached to a uni stream. Due to
the mentionned new fix, this prevent analysis of this last frame for
every uni stream.

To fix this, do not skip anymore app proto decoding for completed QCS.
Update instead qcs_attach_sc() to transform it as a noop function if QCS
is already fully closed before streamdesc instantiation. However,
success return value is still used to prevent an invalid decoding error
report.

The impact of this bug should be minor. Indeed, HTTP3 and QPACK uni
streams are never closed by the client as this is invalid due to the
spec. The only issue was that this prevented QUIC MUX to close the
connection with error H3_ERR_CLOSED_CRITICAL_STREAM.

This must be backported along the previous patch, at least to 3.1, and
eventually to 2.8 if mentionned patches are merged there.
---

diff --git a/src/h3.c b/src/h3.c
index 48bf6d8de9..06d954ebc9 100644
--- a/src/h3.c
+++ b/src/h3.c
@@ -938,8 +938,8 @@ static ssize_t h3_headers_to_htx(struct qcs *qcs, const struct buffer *buf,
 	if (htx)
 		htx_to_buf(htx, &htx_buf);
 
-	/* buffer is transferred to the stream connector and set to NULL
-	 * except on stream creation error.
+	/* Buffer is transferred to the stream connector and set to NULL except
+	 * on stream creation error or QCS already fully closed.
 	 */
 	if (b_size(&htx_buf)) {
 		b_free(&htx_buf);
diff --git a/src/mux_quic.c b/src/mux_quic.c
index 5ff0060903..6e5cad01a7 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -899,7 +899,12 @@ void qcs_send_metadata(struct qcs *qcs)
  * the first received request data. <fin> must be set if the whole request is
  * already received.
  *
- * Returns 0 on success else a negative error code.
+ * Note that if <qcs> is already fully closed, no streamdesc is instantiated.
+ * This is useful if a RESET_STREAM was already emitted in response to a
+ * STOP_SENDING.
+ *
+ * Returns 0 on success else a negative error code. If stream is already fully
+ * closed and nothing is performed, it is considered as a success case.
  */
 int qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)
 {
@@ -908,6 +913,11 @@ int qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)
 
 	TRACE_ENTER(QMUX_EV_STRM_RECV, qcc->conn, qcs);
 
+	if (qcs->st == QC_SS_CLO) {
+		TRACE_STATE("skip attach on already closed stream", QMUX_EV_STRM_RECV, qcc->conn, qcs);
+		goto out;
+	}
+
 	/* TODO duplicated from mux_h2 */
 	sess->t_idle = ns_to_ms(now_ns - sess->accept_ts) - sess->t_handshake;
 
@@ -958,6 +968,7 @@ int qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)
 		se_fl_set_error(qcs->sd);
 	}
 
+ out:
 	TRACE_LEAVE(QMUX_EV_STRM_RECV, qcc->conn, qcs);
 	return 0;
 }
@@ -1152,7 +1163,7 @@ static int qcc_decode_qcs(struct qcc *qcc, struct qcs *qcs)
 	if (qcs_is_close_remote(qcs))
 		fin = 1;
 
-	if (!(qcs->flags & QC_SF_READ_ABORTED) && !qcs_is_completed(qcs)) {
+	if (!(qcs->flags & QC_SF_READ_ABORTED)) {
 		ret = qcc->app_ops->rcv_buf(qcs, &b, fin);
 
 		if (qcc->glitches != prev_glitches)