MINOR: quic: refactor frame deallocation

Define a new function qc_frm_free() to handle frame deallocation. New
BUG_ON() statements ensure that the deallocated frame is not referenced
by other frame. To support this, all LIST_DELETE() have been replaced by
LIST_DEL_INIT(). This should enforce that frame deallocation is robust.

As a complement, qc_frm_unref() has been moved into quic_frame module.
It is justified as this is a utility function related to frame
deallocation. It allows to use it in quic_pktns_tx_pkts_release() before
calling qc_frm_free().

This should be backported up to 2.7.

diff --git a/include/haproxy/quic_conn.h b/include/haproxy/quic_conn.h
index b14765ba55ce65b059ad49d2fe711b339ac6acff..60afccd8deb086527afdd69c2c165123e758d199 100644 (file)
--- a/include/haproxy/quic_conn.h
+++ b/include/haproxy/quic_conn.h
@@ -532,9 +532,10 @@ static inline void quic_pktns_tx_pkts_release(struct quic_pktns *pktns, struct q
                if (pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING)
                        qc->path->ifae_pkts--;
                list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) {
-                       LIST_DELETE(&frm->list);
+                       qc_frm_unref(frm, qc);
+                       LIST_DEL_INIT(&frm->list);
                        quic_tx_packet_refdec(frm->pkt);
-                       pool_free(pool_head_quic_frame, frm);
+                       qc_frm_free(&frm);
                }
                eb64_delete(&pkt->pn_node);
                quic_tx_packet_refdec(pkt);

diff --git a/include/haproxy/quic_frame.h b/include/haproxy/quic_frame.h
index 2b5286ff227b54135fc2fb5d152848fb026d30eb..287be8fd977c5826504acd7620acda50acc68731 100644 (file)
--- a/include/haproxy/quic_frame.h
+++ b/include/haproxy/quic_frame.h
@@ -176,7 +176,8 @@ static inline struct quic_err quic_err_app(uint64_t code)
        return (struct quic_err){ .code = code, .app = 1 };
 }
 
-/* Allocate a quic_frame with type <type>.
+/* Allocate a quic_frame with type <type>. Frame must be freed with
+ * qc_frm_free().
  *
  * Returns the allocated frame or NULL on failure.
  */
@@ -202,7 +203,8 @@ static inline struct quic_frame *qc_frm_alloc(int type)
 
 /* Allocate a quic_frame by duplicating <origin> frame. This will create a new
  * frame of the same type with the same content. Internal fields such as packet
- * owner and flags are however resetted for the newly allocated frame.
+ * owner and flags are however resetted for the newly allocated frame. Frame
+ * must be freed with qc_frm_free().
  *
  * Returns the allocated frame or NULL on failure.
  */
@@ -229,5 +231,27 @@ static inline struct quic_frame *qc_frm_dup(struct quic_frame *origin)
        return frm;
 }
 
+void qc_frm_unref(struct quic_frame *frm, struct quic_conn *qc);
+
+/* Free a <frm> quic_frame. Remove it from parent element if still attached. */
+static inline void qc_frm_free(struct quic_frame **frm)
+{
+
+       /* Caller must ensure that no other frame points to <frm>. Use
+        * qc_frm_unref() to handle this properly.
+        */
+       BUG_ON(!LIST_ISEMPTY(&((*frm)->reflist)));
+       BUG_ON(LIST_INLIST(&((*frm)->ref)));
+
+       /* TODO simplify frame deallocation. In some code paths, we must
+        * manually call this LIST_DEL_INIT before using
+        * quic_tx_packet_refdec() and freeing the frame.
+        */
+       LIST_DEL_INIT(&((*frm)->list));
+
+       pool_free(pool_head_quic_frame, *frm);
+       *frm = NULL;
+}
+
 #endif /* USE_QUIC */
 #endif /* _HAPROXY_QUIC_FRAME_H */

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 1540c3bfde2fbd7acf08336f2cf04d7797783a32..1060635d241eb414f460629b64eb4575ed40d6f6 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -1604,7 +1604,7 @@ static int qcs_send_reset(struct qcs *qcs)
 
        LIST_APPEND(&frms, &frm->list);
        if (qc_send_frames(qcs->qcc, &frms)) {
-               pool_free(pool_head_quic_frame, frm);
+               qc_frm_free(&frm);
                TRACE_DEVEL("cannot send RESET_STREAM", QMUX_EV_QCS_SEND, qcs->qcc->conn, qcs);
                return 1;
        }
@@ -1659,7 +1659,7 @@ static int qcs_send_stop_sending(struct qcs *qcs)
 
        LIST_APPEND(&frms, &frm->list);
        if (qc_send_frames(qcs->qcc, &frms)) {
-               pool_free(pool_head_quic_frame, frm);
+               qc_frm_free(&frm);
                TRACE_DEVEL("cannot send STOP_SENDING", QMUX_EV_QCS_SEND, qcs->qcc->conn, qcs);
                return 1;
        }
@@ -1827,10 +1827,8 @@ static int qc_send(struct qcc *qcc)
        if (!LIST_ISEMPTY(&frms)) {
                struct quic_frame *frm, *frm2;
 
-               list_for_each_entry_safe(frm, frm2, &frms, list) {
-                       LIST_DELETE(&frm->list);
-                       pool_free(pool_head_quic_frame, frm);
-               }
+               list_for_each_entry_safe(frm, frm2, &frms, list)
+                       qc_frm_free(&frm);
        }
 
        TRACE_LEAVE(QMUX_EV_QCC_SEND, qcc->conn);
@@ -1974,8 +1972,7 @@ static void qc_release(struct qcc *qcc)
 
        while (!LIST_ISEMPTY(&qcc->lfctl.frms)) {
                struct quic_frame *frm = LIST_ELEM(qcc->lfctl.frms.n, struct quic_frame *, list);
-               LIST_DELETE(&frm->list);
-               pool_free(pool_head_quic_frame, frm);
+               qc_frm_free(&frm);
        }
 
        if (qcc->app_ops && qcc->app_ops->release)

diff --git a/src/quic_conn.c b/src/quic_conn.c
index a42a07a2717153a1d381ea978e5590c342bc6202..ec03fe6ae9aee66d14e23027f6f589d5f33283c4 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1517,29 +1517,6 @@ static int qc_pkt_decrypt(struct quic_conn *qc, struct quic_enc_level *qel,
 }
 
 
-/* Remove references to <frm> frame */
-static void qc_frm_unref(struct quic_conn *qc, struct quic_frame *frm)
-{
-       struct quic_frame *f, *tmp;
-
-       TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc);
-
-       list_for_each_entry_safe(f, tmp, &frm->reflist, ref) {
-               f->origin = NULL;
-               LIST_DELETE(&f->ref);
-               if (f->pkt) {
-                       TRACE_DEVEL("remove frame reference",
-                                   QUIC_EV_CONN_PRSAFRM, qc, f, &f->pkt->pn_node.key);
-               }
-               else {
-                       TRACE_DEVEL("remove frame reference for unsent frame",
-                                   QUIC_EV_CONN_PRSAFRM, qc, f);
-               }
-       }
-
-       TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc);
-}
-
 /* Release <frm> frame and mark its copies as acknowledged */
 void qc_release_frm(struct quic_conn *qc, struct quic_frame *frm)
 {
@@ -1563,7 +1540,7 @@ void qc_release_frm(struct quic_conn *qc, struct quic_frame *frm)
                if (f->pkt) {
                        f->flags |= QUIC_FL_TX_FRAME_ACKED;
                        f->origin = NULL;
-                       LIST_DELETE(&f->ref);
+                       LIST_DEL_INIT(&f->ref);
                        pn = f->pkt->pn_node.key;
                        TRACE_DEVEL("mark frame as acked from packet",
                                    QUIC_EV_CONN_PRSAFRM, qc, f, &pn);
@@ -1571,17 +1548,16 @@ void qc_release_frm(struct quic_conn *qc, struct quic_frame *frm)
                else {
                        TRACE_DEVEL("freeing unsent frame",
                                    QUIC_EV_CONN_PRSAFRM, qc, f);
-                       LIST_DELETE(&f->ref);
-                       LIST_DELETE(&f->list);
-                       pool_free(pool_head_quic_frame, f);
+                       LIST_DEL_INIT(&f->ref);
+                       qc_frm_free(&f);
                }
        }
-       LIST_DELETE(&frm->list);
+       LIST_DEL_INIT(&frm->list);
        pn = frm->pkt->pn_node.key;
        quic_tx_packet_refdec(frm->pkt);
        TRACE_DEVEL("freeing frame from packet",
                    QUIC_EV_CONN_PRSAFRM, qc, frm, &pn);
-       pool_free(pool_head_quic_frame, frm);
+       qc_frm_free(&frm);
 
        TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc);
 }
@@ -1792,12 +1768,12 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
 
        list_for_each_entry_safe(frm, frmbak, pkt_frm_list, list) {
                /* First remove this frame from the packet it was attached to */
-               LIST_DELETE(&frm->list);
+               LIST_DEL_INIT(&frm->list);
                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 */
-               qc_frm_unref(qc, frm);
+               qc_frm_unref(frm, qc);
                switch (frm->type) {
                case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F:
                {
@@ -1810,7 +1786,7 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
                                TRACE_DEVEL("released stream", QUIC_EV_CONN_PRSAFRM, qc, frm);
                                TRACE_DEVEL("freeing frame from packet", QUIC_EV_CONN_PRSAFRM,
                                            qc, frm, &pn);
-                               pool_free(pool_head_quic_frame, frm);
+                               qc_frm_free(&frm);
                                continue;
                        }
 
@@ -1819,7 +1795,7 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
                        if (strm_frm->offset.key + strm_frm->len <= stream_desc->ack_offset) {
                                TRACE_DEVEL("ignored frame in already acked range",
                                            QUIC_EV_CONN_PRSAFRM, qc, frm);
-                               pool_free(pool_head_quic_frame, frm);
+                               qc_frm_free(&frm);
                                continue;
                        }
                        else if (strm_frm->offset.key < stream_desc->ack_offset) {
@@ -1839,8 +1815,8 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
                        TRACE_DEVEL("ignored frame with old data from packet", QUIC_EV_CONN_PRSAFRM,
                                    qc, frm, &pn);
                        if (frm->origin)
-                               LIST_DELETE(&frm->ref);
-                       pool_free(pool_head_quic_frame, frm);
+                               LIST_DEL_INIT(&frm->ref);
+                       qc_frm_free(&frm);
                        continue;
                }
 
@@ -1848,7 +1824,7 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
                        TRACE_DEVEL("already acked frame", QUIC_EV_CONN_PRSAFRM, qc, frm);
                        TRACE_DEVEL("freeing frame from packet", QUIC_EV_CONN_PRSAFRM,
                                    qc, frm, &pn);
-                       pool_free(pool_head_quic_frame, frm);
+                       qc_frm_free(&frm);
                }
                else {
                        if (QUIC_FT_STREAM_8 <= frm->type && frm->type <= QUIC_FT_STREAM_F) {
@@ -1881,10 +1857,8 @@ static inline void free_quic_tx_packet(struct quic_conn *qc,
        if (!pkt)
                goto leave;
 
-       list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) {
-               LIST_DELETE(&frm->list);
-               pool_free(pool_head_quic_frame, frm);
-       }
+       list_for_each_entry_safe(frm, frmbak, &pkt->frms, list)
+               qc_frm_free(&frm);
        pool_free(pool_head_quic_tx_packet, pkt);
 
  leave:
@@ -1993,10 +1967,8 @@ static inline void qc_release_pktns_frms(struct quic_conn *qc,
 
        TRACE_ENTER(QUIC_EV_CONN_PHPKTS, qc);
 
-       list_for_each_entry_safe(frm, frmbak, &pktns->tx.frms, list) {
-               LIST_DELETE(&frm->list);
-               pool_free(pool_head_quic_frame, frm);
-       }
+       list_for_each_entry_safe(frm, frmbak, &pktns->tx.frms, list)
+               qc_frm_free(&frm);
 
        TRACE_LEAVE(QUIC_EV_CONN_PHPKTS, qc);
 }
@@ -3566,7 +3538,7 @@ static int quic_build_post_handshake_frames(struct quic_conn *qc)
 
                cid = new_quic_cid(&qc->cids, qc, i);
                if (!cid) {
-                       pool_free(pool_head_quic_frame, frm);
+                       qc_frm_free(&frm);
                        TRACE_ERROR("CID allocation error", QUIC_EV_CONN_IO_CB, qc);
                        goto err;
                }
@@ -3589,7 +3561,7 @@ static int quic_build_post_handshake_frames(struct quic_conn *qc)
  err:
        /* free the frames */
        list_for_each_entry_safe(frm, frmbak, &frm_list, list)
-               pool_free(pool_head_quic_frame, frm);
+               qc_frm_free(&frm);
 
        node = eb64_lookup_ge(&qc->cids, first);
        while (node) {
@@ -6718,7 +6690,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                        room -= flen;
                        if (dlen == cf->crypto.len) {
                                /* <cf> CRYPTO data have been consumed. */
-                               LIST_DELETE(&cf->list);
+                               LIST_DEL_INIT(&cf->list);
                                LIST_APPEND(outlist, &cf->list);
                        }
                        else {
@@ -6760,8 +6732,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                                node = eb64_lookup(&qc->streams_by_id, strm->id);
                                if (!node) {
                                        TRACE_DEVEL("released stream", QUIC_EV_CONN_PRSAFRM, qc, cf);
-                                       LIST_DELETE(&cf->list);
-                                       pool_free(pool_head_quic_frame, cf);
+                                       qc_frm_free(&cf);
                                        continue;
                                }
 
@@ -6769,8 +6740,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                                if (strm->offset.key + strm->len <= stream_desc->ack_offset) {
                                        TRACE_DEVEL("ignored frame frame in already acked range",
                                                    QUIC_EV_CONN_PRSAFRM, qc, cf);
-                                       LIST_DELETE(&cf->list);
-                                       pool_free(pool_head_quic_frame, cf);
+                                       qc_frm_free(&cf);
                                        continue;
                                }
                                else if (strm->offset.key < stream_desc->ack_offset) {
@@ -6818,7 +6788,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                        room -= flen;
                        if (dlen == cf->stream.len) {
                                /* <cf> STREAM data have been consumed. */
-                               LIST_DELETE(&cf->list);
+                               LIST_DEL_INIT(&cf->list);
                                LIST_APPEND(outlist, &cf->list);
 
                                /* Do not notify MUX on retransmission. */
@@ -6890,7 +6860,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
 
                        *len += flen;
                        room -= flen;
-                       LIST_DELETE(&cf->list);
+                       LIST_DEL_INIT(&cf->list);
                        LIST_APPEND(outlist, &cf->list);
                        break;
                }
@@ -7144,7 +7114,7 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
                                /* Note that <cf> was added from <frms> to <frm_list> list by
                                 * qc_build_frms().
                                 */
-                               LIST_DELETE(&cf->list);
+                               LIST_DEL_INIT(&cf->list);
                                LIST_INSERT(frms, &cf->list);
                                continue;
                        }

diff --git a/src/quic_frame.c b/src/quic_frame.c
index 38b524ec15b053b7ebe94c3aff4c669d112c7463..530a6407703d27fb0a6646145c6f81cca0772680 100644 (file)
--- a/src/quic_frame.c
+++ b/src/quic_frame.c
@@ -1171,3 +1171,25 @@ int qc_build_frm(unsigned char **buf, const unsigned char *end,
        return ret;
 }
 
+/* Detach all duplicated frames from <frm> reflist. */
+void qc_frm_unref(struct quic_frame *frm, struct quic_conn *qc)
+{
+       struct quic_frame *f, *tmp;
+
+       TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc);
+
+       list_for_each_entry_safe(f, tmp, &frm->reflist, ref) {
+               f->origin = NULL;
+               LIST_DEL_INIT(&f->ref);
+               if (f->pkt) {
+                       TRACE_DEVEL("remove frame reference",
+                                   QUIC_EV_CONN_PRSAFRM, qc, f, &f->pkt->pn_node.key);
+               }
+               else {
+                       TRACE_DEVEL("remove frame reference for unsent frame",
+                                   QUIC_EV_CONN_PRSAFRM, qc, f);
+               }
+       }
+
+       TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc);
+}