BUG/MAJOR: mux_quic: fix potential crash on RESET_STREAM receive

When a RESET_STREAM frame is received and upper layer is not allocated,
QCS instance is immediately locally closed and inserted into QCC purge
list. This operation has been performed since the following patch.
Before, the QCS would remain allocated and could possibly prevent the
connection closure if accounted as a request stream.

  commit b7e0aa231aa7af214c97f9540f7bba8498898b11
  BUG/MAJOR: mux_quic: fix leak on RESET_STREAM reception

Sadly, this causes a crash if QCS is already inserted in QCC send list.
This is because <el_send> element is used both for send and purge list,
as a QCS cannot be present in both at the same time.

Fix this skipping insertion in purge list if QCS is already present in
the send list. QCS is not closed locally either in this case. This
allows the QCS to finalize its emission before being closed and purged.

This latter case requires to extend check in qcm_ctrl_send() for QCS
purging when everything is emitted. This is now also performed if a
RESET_STREAM has been received and upper stream layer is not present.

These modifications reveal that it is probably not valid to silently
close a QCS without notifying the peer. Reaction to a RESET_STREAM is in
fact dependent on the application layer protocol and the QUIC stream
type. For now, the current code base should ensure that a bidi HTTP/3
stream is not kept forever after peer reset. A more thoughtful approach
may be implemented later in a future patch.

This must be backported up to 2.8.

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 23433aacc0b75a0cc7b115c470eda29b5ec0b0ab..0e11fc7cf81d2d60bd9135109c31c6dab31a3216 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -720,7 +720,17 @@ static void qcm_ctrl_send(struct qc_stream_desc *stream, uint64_t data, uint64_t
                LIST_DEL_INIT(&qcs->el_send);
                TRACE_STATE("stream sent done", QMUX_EV_QCS_SEND, qcc->conn, qcs);
 
-               if (qcs->flags & (QC_SF_FIN_STREAM|QC_SF_DETACH)) {
+               /* Everything sent, close the stream if no new data will come.
+                * This is the case if either FIN has been sent or upper layer
+                * is detached.
+                *
+                * This is also performed if peer has resetted its input as no
+                * more transfer should occur on this QCS instance.
+                * TODO this assumption may be invalid depending on QUIC stream
+                * type - app proto should be responsible to deal with RS.
+                */
+               if ((qcs->flags & (QC_SF_FIN_STREAM|QC_SF_DETACH)) ||
+                   (!qcs_sc(qcs) && (qcs->flags & QC_SF_RECV_RESET))) {
                        /* Close stream locally. */
                        qcs_close_local(qcs);
 
@@ -2360,13 +2370,11 @@ int qcc_recv_reset_stream(struct qcc *qcc, uint64_t id, uint64_t err, uint64_t f
        /* Remove stream from recv_list if present. */
        LIST_DEL_INIT(&qcs->el_recv);
 
-       /* Check if RESET_STREAM received before stream layer initialization.
-        * If true, prepare QCS purgeing immediately.
+       /* No more data will be received after peer reset. QCS can be purged
+        * if there is nothing to sent on our side.
         */
-       if (!qcs_sc(qcs) && !(qcs->flags & QC_SF_DETACH)) {
+       if (!qcs_sc(qcs) && !LIST_INLIST(&qcs->el_send)) {
                qcs_close_local(qcs);
-
-               BUG_ON(LIST_INLIST(&qcs->el_send));
                TRACE_STATE("add stream in purg_list", QMUX_EV_QCC_RECV|QMUX_EV_QCS_RECV, qcc->conn, qcs);
                LIST_APPEND(&qcs->qcc->purg_list, &qcs->el_send);
                tasklet_wakeup(qcs->qcc->wait_event.tasklet);