BUG/MEDIUM: quic: do not use qcs from quic_stream on ACK parsing

The quic_stream frame stores the qcs instance. On ACK parsing, qcs is
accessed to clear the stream buffer. This can cause a segfault if the
MUX or the qcs is already released.

Consider the following scenario :

1. a STREAM frame is generated by the MUX
   transport layer emits the frame with PKN=1
   upper layer has finished the transfer so related qcs is detached

2. transport layer reemits the frame with PKN=2 because ACK was not
   received

3. ACK for PKN=1 is received, stream buffer is cleared
   at this stage, qcs may be freed by the MUX as it is detached

4. ACK for PKN=2 is received
   qcs for STREAM frame is dereferenced which will lead to a crash

To prevent this, qcs is never accessed from the quic_stream during ACK
parsing. Instead, a lookup is done on the MUX streams tree. If the MUX
is already released, no lookup is done. These checks prevents a possible
segfault.

This change may have an impact on the perf as now we are forced to use a
tree lookup operation. If this is the case, an alternative solution may
be to implement a refcount on qcs instances.

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index d0beb500f0163db42c81e8993b8442bae8646c54..c2003704ac193c5761926a48d986f1a5082cf9fe 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -1455,8 +1455,30 @@ static inline void qc_treat_acked_tx_frm(struct quic_conn *qc,
        switch (frm->type) {
        case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F:
        {
-               struct qcs *qcs = frm->stream.qcs;
                struct quic_stream *strm = &frm->stream;
+               struct eb64_node *node = NULL;
+               struct qcs *qcs = NULL;
+
+               /* do not use strm->qcs as the qcs instance might be freed at
+                * this stage. Use the id to do a proper lookup.
+                *
+                * TODO if lookup operation impact on the perf is noticeable,
+                * implement a refcount on qcs instances.
+                */
+               if (qc->mux_state == QC_MUX_READY) {
+                       node = eb64_lookup(&qc->qcc->streams_by_id, strm->id);
+                       qcs = eb64_entry(node, struct qcs, by_id);
+               }
+
+               if (!qcs) {
+                       TRACE_PROTO("acked stream for released stream", QUIC_EV_CONN_ACKSTRM, qc, strm);
+                       LIST_DELETE(&frm->list);
+                       quic_tx_packet_refdec(frm->pkt);
+                       pool_free(pool_head_quic_frame, frm);
+
+                       /* early return */
+                       return;
+               }
 
                TRACE_PROTO("acked stream", QUIC_EV_CONN_ACKSTRM, qc, strm, qcs);
                if (strm->offset.key <= qcs->tx.ack_offset) {