1berry/haproxy
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Amaury Denoyelle 2025-03-20 16:01:16 +01:00
parent 85f2f93d11
commit 7ee1279f4b
1 changed files with 34 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

55
src/mux_quic.c
View File

@ -1517,6 +1517,33 @@ static void qcc_clear_frms(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;
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 <qcs> with error code <err>. */
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 <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)
{
@ -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: