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 b14765b..60afccd 100644
--- a/include/haproxy/quic_conn.h
+++ b/include/haproxy/quic_conn.h
@@ -532,9 +532,10 @@
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 2b5286f..287be8f 100644
--- a/include/haproxy/quic_frame.h
+++ b/include/haproxy/quic_frame.h
@@ -176,7 +176,8 @@
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 @@
/* 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 @@
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 1540c3b..1060635 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -1604,7 +1604,7 @@
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 @@
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 @@
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 @@
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 a42a07a..ec03fe6 100644
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1517,29 +1517,6 @@
}
-/* 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
*len += flen;
room -= flen;
- LIST_DELETE(&cf->list);
+ LIST_DEL_INIT(&cf->list);
LIST_APPEND(outlist, &cf->list);
break;
}
@@ -7144,7 +7114,7 @@
/* 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 38b524e..530a640 100644
--- a/src/quic_frame.c
+++ b/src/quic_frame.c
@@ -1171,3 +1171,25 @@
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);
+}