From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Thu, 21 Sep 2023 15:06:16 +0000 (+0200)
Subject: BUG/MINOR: mux-quic: remove full demux flag on ncbuf release
X-Git-Tag: v2.9-dev7~74
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7cf9cf705ed0284ac1160adb66be19c1d57986a9;p=thirdparty%2Fhaproxy.git

BUG/MINOR: mux-quic: remove full demux flag on ncbuf release

When rcv_buf stream callback is invoked, mux tasklet is woken up if
demux was previously blocked due to lack of buffer space. A BUG_ON() is
present to ensure there is data in qcs Rx buffer. If this is not the
case, wakeup is unneeded :

  BUG_ON(!ncb_data(&qcs->rx.ncbuf, 0));

This BUG_ON() may be triggered if RESET_STREAM is received after demux
has been blocked. On reset, Rx buffer is purged according to RFC 9000
which allows to discard any data not yet consumed. This will trigger the
BUG_ON() assertion if rcv_buf stream callback is invoked after this.

To prevent BUG_ON() crash, just clear demux block flag each time Rx
buffer is purged. This covers accordingly RESET_STREAM reception.

This should be backported up to 2.7.

This may fix github issue #2293.

This bug relies on several precondition so its occurence is rare. This
was reproduced by using a custom client which post big enough data to
fill the buffer. It then emits a RESET_STREAM in place of a proper FIN.
Moreover, mux code has been edited to artificially stalled stream read
to force demux blocking.

h3_data_to_htx:
-       return htx_sent;
+       return 1;

qcc_recv_reset_stream:
        qcs_free_ncbuf(qcs, &qcs->rx.ncbuf);
+       qcs_notify_recv(qcs);

qmux_strm_rcv_buf:
        char fin = 0;
+       static int i = 0;
+       if (++i < 2)
+               return 0;
        TRACE_ENTER(QMUX_EV_STRM_RECV, qcc->conn, qcs);
---

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 698bacf082..4a246c2c09 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -37,6 +37,11 @@ static void qcs_free_ncbuf(struct qcs *qcs, struct ncbuf *ncbuf)
 	offer_buffers(NULL, 1);
 
 	*ncbuf = NCBUF_NULL;
+
+	/* Reset DEM_FULL as buffer is released. This ensures mux is not woken
+	 * up from rcv_buf stream callback when demux was previously blocked.
+	 */
+	qcs->flags &= ~QC_SF_DEM_FULL;
 }
 
 /* Free <qcs> instance. This function is reserved for internal usage : it must
@@ -2743,9 +2748,8 @@ static size_t qmux_strm_rcv_buf(struct stconn *sc, struct buffer *buf,
 
 	/* Restart demux if it was interrupted on full buffer. */
 	if (ret && qcs->flags & QC_SF_DEM_FULL) {
-		/* There must be data left for demux if it was interrupted on
-		 * full buffer. If this assumption is incorrect wakeup is not
-		 * necessary.
+		/* Ensure DEM_FULL is only set if there is available data to
+		 * ensure we never do unnecessary wakeup here.
 		 */
 		BUG_ON(!ncb_data(&qcs->rx.ncbuf, 0));