From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Mon, 17 Jun 2024 13:39:24 +0000 (+0200)
Subject: BUG/MAJOR: quic: do not loop on emission on closing/draining state
X-Git-Tag: v3.1-dev2~33
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=937324d493eefb54d2f68820a9afcf5cfd66bdf5;p=thirdparty%2Fhaproxy.git

BUG/MAJOR: quic: do not loop on emission on closing/draining state

To emit CONNECTION_CLOSE frame, a special buffer is allocated via
qc_txb_store(). This is due to QUIC_FL_CONN_IMMEDIATE_CLOSE flag.
However this flag is reset after qc_send_ppkts() invocation to prevent
reemission of CONNECTION_CLOSE frame.

qc_send() can invoke multiple times a series of qc_prep_pkts() +
qc_send_ppkts() to emit several datagrams. However, this may cause a
crash if on first loop a CONNECTION_CLOSE is emitted. On the next loop
iteration, QUIC_FL_CONN_IMMEDIATE_CLOSE is resetted, thus qc_prep_pkts()
will use the wrong buffer size as end delimiter. In some cases, this may
cause a BUG_ON() crash due to b_add() outside of buffer.

This bug can be reproduced by using a while loop of ngtcp2-client and
interrupting them randomly via Ctrl+C.

Here is the patch which introduce this regression :
  cdfceb10ae136b02e51f9bb346321cf0045d58e0
  MINOR: quic: refactor qc_prep_pkts() loop
---

diff --git a/src/quic_tx.c b/src/quic_tx.c
index a1705784c4..c963688ae4 100644
--- a/src/quic_tx.c
+++ b/src/quic_tx.c
@@ -708,8 +708,13 @@ int qc_send(struct quic_conn *qc, int old_data, struct list *send_list)
 		qc->flags |= QUIC_FL_CONN_RETRANS_OLD_DATA;
 	}
 
-	/* Prepare and send packets until we could not further prepare packets. */
-	while (!LIST_ISEMPTY(send_list)) {
+	/* Prepare and send packets until we could not further prepare packets.
+	 * Sending must be interrupted if a CONNECTION_CLOSE was already sent
+	 * previously and is currently not needed.
+	 */
+	while (!LIST_ISEMPTY(send_list) &&
+	       (!(qc->flags & (QUIC_FL_CONN_CLOSING|QUIC_FL_CONN_DRAINING)) ||
+	        (qc->flags & QUIC_FL_CONN_IMMEDIATE_CLOSE))) {
 		/* Buffer must always be empty before qc_prep_pkts() usage.
 		 * qc_send_ppkts() ensures it is cleared on success.
 		 */
@@ -728,6 +733,12 @@ int qc_send(struct quic_conn *qc, int old_data, struct list *send_list)
 			TRACE_DEVEL("stopping on qc_prep_pkts() return", QUIC_EV_CONN_TXPKT, qc);
 			break;
 		}
+
+		if ((qc->flags & QUIC_FL_CONN_DRAINING) &&
+		    !(qc->flags & QUIC_FL_CONN_IMMEDIATE_CLOSE)) {
+			TRACE_DEVEL("draining connection", QUIC_EV_CONN_TXPKT, qc);
+			break;
+		}
 	}
 
 	qc_txb_release(qc);