BUG/MEDIUM: mux-quic: fix crash on RS/SS emission if already close local

A BUG_ON() is present in qcc_send_stream() to ensure that emission is
never performed with a stream already closed locally. However, this
function is also used for RESET_STREAM/STOP_SENDING emission. No
protection exists to ensure that RS/SS is not scheduled after stream
local closure, which would result in this BUG_ON() crash.

This crash can be triggered with the following QUIC client sequence :
1. SS is emitted to open a new stream. QUIC-MUX schedules a RS emission
   by and the stream is locally closed.
2. An invalid HTTP/3 request is sent on the same stream, for example
   with duplicated pseudo-headers. The objective is to ensure
   qcc_abort_stream_read() is called after stream closure, which results
   in the following backtrace.

 0x000055555566a620 in qcc_send_stream (qcs=0x7ffff0061420, urg=1, count=0) at src/mux_quic.c:1633
 1633            BUG_ON(qcs_is_close_local(qcs));
 [ ## gdb ## ] bt
 #0  0x000055555566a620 in qcc_send_stream (qcs=0x7ffff0061420, urg=1, count=0) at src/mux_quic.c:1633
 #1  0x000055555566a921 in qcc_abort_stream_read (qcs=0x7ffff0061420) at src/mux_quic.c:1658
 #2  0x0000555555685426 in h3_rcv_buf (qcs=0x7ffff0061420, b=0x7ffff748d3f0, fin=0) at src/h3.c:1454
 #3  0x0000555555668a67 in qcc_decode_qcs (qcc=0x7ffff0049eb0, qcs=0x7ffff0061420) at src/mux_quic.c:1315
 #4  0x000055555566c76e in qcc_recv (qcc=0x7ffff0049eb0, id=12, len=0, offset=23, fin=0 '\000',
     data=0x7fffe0049c1c "\366\r,\230\205\354\234\301;\2563\335\037k\306\334\037\260", <incomplete sequence \323>) at src/mux_quic.c:1901
 #5  0x0000555555692551 in qc_handle_strm_frm (pkt=0x7fffe00484b0, strm_frm=0x7ffff00539e0, qc=0x7fffe0049220, fin=0 '\000') at src/quic_rx.c:635
 #6  0x0000555555694530 in qc_parse_pkt_frms (qc=0x7fffe0049220, pkt=0x7fffe00484b0, qel=0x7fffe0075fc0) at src/quic_rx.c:980
 #7  0x0000555555696c7a in qc_treat_rx_pkts (qc=0x7fffe0049220) at src/quic_rx.c:1324
 #8  0x00005555556b781b in quic_conn_app_io_cb (t=0x7fffe0037f20, context=0x7fffe0049220, state=49232) at src/quic_conn.c:601
 #9  0x0000555555d53788 in run_tasks_from_lists (budgets=0x7ffff748e2b0) at src/task.c:603
 #10 0x0000555555d541ae in process_runnable_tasks () at src/task.c:886
 #11 0x00005555559c39e9 in run_poll_loop () at src/haproxy.c:2858
 #12 0x00005555559c41ea in run_thread_poll_loop (data=0x55555629fb40 <ha_thread_info+64>) at src/haproxy.c:3075

The proper solution is to not execute this BUG_ON() for RS/SS emission.
Indeed, it is valid and can be useful to emit these frames, even after
stream local closure.

To implement this, qcc_send_stream() has been rewritten as a mere
wrapper function around the new internal _qcc_send_stream(). The latter
is used only by QMUX for STREAM, RS and SS emission. Application layer
continue to use the original function for STREAM emission, with the
BUG_ON() still in place there.

This must be backported up to 2.8.

(cherry picked from commit 7ee1279f4b8416435faba5cb93a9be713f52e4df)
Signed-off-by: Aurelien DARRAGON <adarragon@haproxy.com>
(cherry picked from commit c44b72d88a48f81a3b2fd1433f372226131a2e5a)
[ada: ctx adjt: since 3a00193b2 ("MINOR: mux-quic: split STREAM and RS/SS
 emission") was not backported past 3.1, _qcc_send_stream() was based
 on prior qcc_send_stream() implementation]
Signed-off-by: Aurelien DARRAGON <adarragon@haproxy.com>

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 8da7a65..8e0789c 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -1148,6 +1148,28 @@ static void qcc_notify_fctl(struct qcc *qcc)
        }
 }
 
+/* Register <qcs> stream for emission of STREAM, STOP_SENDING or RESET_STREAM.
+ * Set <urg> to true if stream should be emitted in priority. This is useful
+ * when sending STOP_SENDING or RESET_STREAM, or for emission on an application
+ * control stream.
+ */
+static void _qcc_send_stream(struct qcs *qcs, int urg)
+{
+       struct qcc *qcc = qcs->qcc;
+
+       if (urg) {
+               LIST_DEL_INIT(&qcs->el_send);
+               LIST_INSERT(&qcc->send_list, &qcs->el_send);
+       }
+       else {
+               /* Cannot send STREAM if already closed. */
+               BUG_ON(qcs_is_close_local(qcs));
+
+               if (!LIST_INLIST(&qcs->el_send))
+                       LIST_APPEND(&qcs->qcc->send_list, &qcs->el_send);
+       }
+}
+
 /* Prepare for the emission of RESET_STREAM on <qcs> with error code <err>. */
 void qcc_reset_stream(struct qcs *qcs, int err)
 {
@@ -1186,14 +1208,13 @@ void qcc_reset_stream(struct qcs *qcs, int err)
                qcs_alert(qcs);
        }
 
-       qcc_send_stream(qcs, 1, 0);
+       _qcc_send_stream(qcs, 1);
        tasklet_wakeup(qcc->wait_event.tasklet);
 }
 
-/* Register <qcs> stream for emission of STREAM, STOP_SENDING or RESET_STREAM.
- * Set <urg> to 1 if stream content should be treated in priority compared to
- * other streams. For STREAM emission, <count> must contains the size of the
- * frame payload. This is used for flow control accounting.
+/* Register <qcs> stream for STREAM emission. Set <urg> to 1 if stream content
+ * should be treated in priority compared to other streams. <count> must
+ * contains the size of the frame payload, used for flow control accounting.
  */
 void qcc_send_stream(struct qcs *qcs, int urg, int count)
 {
@@ -1201,17 +1222,10 @@ void qcc_send_stream(struct qcs *qcs, int urg, int count)
 
        TRACE_ENTER(QMUX_EV_QCS_SEND, qcc->conn, qcs);
 
-       /* Cannot send if already closed. */
+       /* Cannot send STREAM if already closed. */
        BUG_ON(qcs_is_close_local(qcs));
 
-       if (urg) {
-               LIST_DEL_INIT(&qcs->el_send);
-               LIST_INSERT(&qcc->send_list, &qcs->el_send);
-       }
-       else {
-               if (!LIST_INLIST(&qcs->el_send))
-                       LIST_APPEND(&qcs->qcc->send_list, &qcs->el_send);
-       }
+       _qcc_send_stream(qcs, urg);
 
        if (count) {
                qfctl_sinc(&qcc->tx.fc, count);
@@ -1234,7 +1248,7 @@ void qcc_abort_stream_read(struct qcs *qcs)
        TRACE_STATE("abort stream read", QMUX_EV_QCS_END, qcc->conn, qcs);
        qcs->flags |= (QC_SF_TO_STOP_SENDING|QC_SF_READ_ABORTED);
 
-       qcc_send_stream(qcs, 1, 0);
+       _qcc_send_stream(qcs, 1);
        tasklet_wakeup(qcc->wait_event.tasklet);
 
  end: