From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Mon, 2 Dec 2024 15:24:45 +0000 (+0100)
Subject: MINOR: mux-quic: clean up zero-copy done_ff callback
X-Git-Tag: v3.2-dev1~41
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7885a3b3e159323ae3e67f7bbfb4567f2031dc9e;p=thirdparty%2Fhaproxy.git

MINOR: mux-quic: clean up zero-copy done_ff callback

Recently, an issue was found with QUIC zero-copy forwarding on 3.0
version. A desynchronization could occur internally in QCS Tx bytes
counters which would cause a BUG_ON() crash on qcs_destroy() when the
stream is detached.

It was silently fixed in version 3.1 by the following patch. As it was
considered as an optimization, it was not scheduled yet for backport.

  6697e87ae5e1f569dc87cf690b5ecfc049c4aab0
  MINOR: mux-quic: Don't send an emtpy H3 DATA frame during zero-copy forwarding

This mistake has been caused due to some counter-intuitive manipulation
in QUIC zero-copy implementation. Try to streamline this in QUIC MUX
done_ff callback and its application protocol counterpart. Especially
for values exchanged between MUX and application on one side, and MUX
and stconn layer as done_fastfwd return value.

First, application done_ff callback now returns the length of the wholly
encoded frame. For HTTP/3, it means header length + payload length h3
frame. This value can then be reused as qcc_send_stream() argument to
increase QCS Tx soft offset.

As previously, special care has been taken to ensure that QUIC MUX
done_ff only return the transferred data bytes. Thus, any extra offset
for HTTP/3 header is properly excluded. This is mandatory for stconn
layer to consider the transfer has completed.

Secondly, remove duplicated code in application done_ff to reset iobuf
info. This is now factorize in QUIC MUX done_ff itself.

This patch is related to github issue #2678.
---

diff --git a/src/h3.c b/src/h3.c
index 1006813c59..9f7bb02067 100644
--- a/src/h3.c
+++ b/src/h3.c
@@ -2179,9 +2179,14 @@ static size_t h3_nego_ff(struct qcs *qcs, size_t count)
 	return ret;
 }
 
+/* Finalize encoding of a HTTP/3 data frame after zero-copy for <qcs> stream.
+ * No frame is encoded if <qcs> iobuf indicates that no data were transferred.
+ *
+ * Return the payload size of the H3 data frame or 0 if no frame encoded.
+ */
 static size_t h3_done_ff(struct qcs *qcs)
 {
-	size_t total = qcs->sd->iobuf.data;
+	size_t total = 0;
 	TRACE_ENTER(H3_EV_STRM_SEND, qcs->qcc->conn, qcs);
 
 	h3_debug_printf(stderr, "%s\n", __func__);
@@ -2189,15 +2194,14 @@ static size_t h3_done_ff(struct qcs *qcs)
 	if (qcs->sd->iobuf.data) {
 		TRACE_DATA("encoding DATA frame (fast forward)",
 		           H3_EV_TX_FRAME|H3_EV_TX_DATA, qcs->qcc->conn, qcs);
+
 		b_sub(qcs->sd->iobuf.buf, qcs->sd->iobuf.data);
 		b_putchr(qcs->sd->iobuf.buf, 0x00); /* h3 frame type = DATA */
 		b_quic_enc_int(qcs->sd->iobuf.buf, qcs->sd->iobuf.data, QUIC_VARINT_MAX_SIZE); /* h3 frame length */
 		b_add(qcs->sd->iobuf.buf, qcs->sd->iobuf.data);
-	}
 
-	qcs->sd->iobuf.buf = NULL;
-	qcs->sd->iobuf.offset = 0;
-	qcs->sd->iobuf.data = 0;
+		total = qcs->sd->iobuf.offset + qcs->sd->iobuf.data;
+	}
 
 	TRACE_LEAVE(H3_EV_STRM_SEND, qcs->qcc->conn, qcs);
 	return total;
diff --git a/src/hq_interop.c b/src/hq_interop.c
index d0c6b43c2d..18e9e2ccc2 100644
--- a/src/hq_interop.c
+++ b/src/hq_interop.c
@@ -214,14 +214,8 @@ static size_t hq_interop_nego_ff(struct qcs *qcs, size_t count)
 
 static size_t hq_interop_done_ff(struct qcs *qcs)
 {
-	const size_t ret = qcs->sd->iobuf.data;
-
-	/* No header required for HTTP/0.9, simply mark ff as done. */
-	qcs->sd->iobuf.buf = NULL;
-	qcs->sd->iobuf.offset = 0;
-	qcs->sd->iobuf.data = 0;
-
-	return ret;
+	/* No header required for HTTP/0.9. */
+	return qcs->sd->iobuf.data;
 }
 
 static int hq_interop_attach(struct qcs *qcs, void *conn_ctx)
diff --git a/src/mux_quic.c b/src/mux_quic.c
index a06b36ddf0..24e71ab929 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -3385,12 +3385,14 @@ static size_t qmux_strm_done_ff(struct stconn *sc)
 		goto end;
 	}
 
-	if (data)
-		data += sd->iobuf.offset;
 	total = qcs->qcc->app_ops->done_ff(qcs);
+	if (total || qcs->flags & QC_SF_FIN_STREAM)
+		qcc_send_stream(qcs, 0, total);
 
-	if (data || qcs->flags & QC_SF_FIN_STREAM)
-		qcc_send_stream(qcs, 0, data);
+	/* Reset stconn iobuf information. */
+	qcs->sd->iobuf.buf = NULL;
+	qcs->sd->iobuf.offset = 0;
+	qcs->sd->iobuf.data = 0;
 
 	/* Similar to snd_buf callback. */
 	if (!(qcs->qcc->wait_event.events & SUB_RETRY_SEND))
@@ -3400,7 +3402,7 @@ static size_t qmux_strm_done_ff(struct stconn *sc)
 
   end:
 	TRACE_LEAVE(QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs);
-	return total;
+	return data;
 }
 
 static int qmux_strm_resume_ff(struct stconn *sc, unsigned int flags)