BUG/MINOR: quic: Possible crashes when dereferencing ->pkt quic_frame struct member

This was done at several places. First in qc_requeue_nacked_pkt_tx_frms.
This aim of this function is, if needed, to requeue all the TX frames of a lost
<pkt> packet passed as argument and detach them from this packet they have been
sent from. They are possible cases where the frm->pkt quic_frame struct member could
be NULL, as a result of a duplication of an original frame by qc_dup_pkt_frms(). This
function adds the duplicated frame to the original frame reference list:
        LIST_APPEND(&origin->reflist, &dup_frm->ref);
But, in this function, the packet which contains the frame is the one which is passed
as argument (for debug purpose). So let us prefer using this variable.
Also do not dereference this ->pkt quic_frame member in qc_release_frm() and
qc_frm_unref() and add a trace to catch the frame with a null ->pkt member.
They are logically frames which have not already been sent.

Thank you to Tristan for having reported such crashes in GH #1808.

Must be backported to 2.6

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index dd68eb6d72d444c875709c0a9a6d76d153634d7a..0c0e71982cfec4ed04bf43cba8db89e2e1e4b1be 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -1460,16 +1460,22 @@ static int qc_pkt_decrypt(struct quic_rx_packet *pkt, struct quic_enc_level *qel
 /* Remove references to <frm> frame */
 static void qc_frm_unref(struct quic_conn *qc, struct quic_frame *frm)
 {
-       uint64_t pn;
        struct quic_frame *f, *tmp;
 
        TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc);
 
        list_for_each_entry_safe(f, tmp, &frm->reflist, ref) {
-               pn = f->pkt->pn_node.key;
                f->origin = NULL;
                LIST_DELETE(&f->ref);
-               TRACE_DEVEL("remove frame reference", QUIC_EV_CONN_PRSAFRM, qc, f, &pn);
+               if (f->pkt) {
+                       TRACE_DEVEL("remove frame reference",
+                                   QUIC_EV_CONN_PRSAFRM, qc, f, &f->pkt->pn_node.key);
+               }
+               else {
+                       /* XXX TODO: what must be done for such a frame */
+                       TRACE_DEVEL("remove frame reference for unsent frame",
+                                   QUIC_EV_CONN_PRSAFRM, qc, f);
+               }
        }
 
        TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc);
@@ -1495,9 +1501,16 @@ void qc_release_frm(struct quic_conn *qc, struct quic_frame *frm)
         * the current one.
         */
        list_for_each_entry_safe(f, tmp, &origin->reflist, ref) {
-               pn = f->pkt->pn_node.key;
-               TRACE_DEVEL("mark frame as acked from packet",
-                           QUIC_EV_CONN_PRSAFRM, qc, f, &pn);
+               if (f->pkt) {
+                       pn = f->pkt->pn_node.key;
+                       TRACE_DEVEL("mark frame as acked from packet",
+                                   QUIC_EV_CONN_PRSAFRM, qc, f, &pn);
+               }
+               else {
+                       /* XXX TODO: what must be done for such a frame */
+                       TRACE_DEVEL("remove frame reference for unsent frame",
+                                   QUIC_EV_CONN_PRSAFRM, qc, f);
+               }
                f->flags |= QUIC_FL_TX_FRAME_ACKED;
                f->origin = NULL;
                LIST_DELETE(&f->ref);
@@ -1712,17 +1725,14 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
        struct quic_frame *frm, *frmbak;
        struct list tmp = LIST_HEAD_INIT(tmp);
        struct list *pkt_frm_list = &pkt->frms;
+       uint64_t pn = pkt->pn_node.key;
 
        TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc);
 
        list_for_each_entry_safe(frm, frmbak, pkt_frm_list, list) {
-               /* Only for debug */
-               uint64_t pn;
-
                /* First remove this frame from the packet it was attached to */
                LIST_DELETE(&frm->list);
-               pn = frm->pkt->pn_node.key;
-               quic_tx_packet_refdec(frm->pkt);
+               quic_tx_packet_refdec(pkt);
                /* At this time, this frame is not freed but removed from its packet */
                frm->pkt = NULL;
                /* Remove any reference to this frame */