From: Amaury Denoyelle Date: Thu, 20 Mar 2025 15:01:16 +0000 (+0100) Subject: BUG/MEDIUM: mux-quic: fix crash on RS/SS emission if already close local X-Git-Tag: v3.2-dev8~11 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7ee1279f4b8416435faba5cb93a9be713f52e4df;p=thirdparty%2Fhaproxy.git 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", ) 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 ) 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. --- diff --git a/src/mux_quic.c b/src/mux_quic.c index f1d41d42a..5c39ef757 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -1517,6 +1517,33 @@ static void qcc_clear_frms(struct qcc *qcc) } } +/* Register stream for emission of STREAM, STOP_SENDING or RESET_STREAM. + * Set 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; + + qcc_clear_frms(qcc); + + if (urg) { + /* qcc_emit_rs_ss() relies on resetted/aborted streams in send_list front. */ + BUG_ON(!(qcs->flags & (QC_SF_TO_RESET|QC_SF_TO_STOP_SENDING|QC_SF_TXBUB_OOB))); + + 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 with error code . */ void qcc_reset_stream(struct qcs *qcs, int err) { @@ -1555,14 +1582,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 stream for emission of STREAM, STOP_SENDING or RESET_STREAM. - * Set to 1 if stream content should be treated in priority compared to - * other streams. For STREAM emission, must contains the size of the - * frame payload. This is used for flow control accounting. +/* Register stream for STREAM emission. Set to 1 if stream content + * should be treated in priority compared to other streams. must + * contains the size of the frame payload, used for flow control accounting. */ void qcc_send_stream(struct qcs *qcs, int urg, int count) { @@ -1570,22 +1596,9 @@ 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)); - - qcc_clear_frms(qcc); - - if (urg) { - /* qcc_emit_rs_ss() relies on resetted/aborted streams in send_list front. */ - BUG_ON(!(qcs->flags & (QC_SF_TO_RESET|QC_SF_TO_STOP_SENDING|QC_SF_TXBUB_OOB))); - - 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); @@ -1608,7 +1621,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: