BUG/MAJOR: quic: Crash after discarding packet number spaces

This previous patch was not sufficient to prevent haproxy from
crashing when some Handshake packets had to be inspected before being possibly
retransmitted:

     "BUG/MAJOR: quic: Crash upon retransmission of dgrams with several packets"

This patch introduced another issue: access to packets which have been
released because still attached to others (in the same datagram). This was
the case for instance when discarding the Initial packet number space before
inspecting an Handshake packet in the same datagram through its ->prev or
member in our case.

This patch implements quic_tx_packet_dgram_detach() which detaches a packet
from the adjacent ones in the same datagram to be called when ackwowledging
a packet (as done in the previous commit) and when releasing its memory. This
was, we are sure the released packets will not be accessed during retransmissions.

Thank you to @gabrieltz for having reported this issue in GH #1903.

Must be backported to 2.6.

diff --git a/include/haproxy/quic_conn.h b/include/haproxy/quic_conn.h
index a2292d008d4d7602f4366a04f94417b8f629bc35..6b2b70b221057994951d999f572bcd42eed1d672 100644 (file)
--- a/include/haproxy/quic_conn.h
+++ b/include/haproxy/quic_conn.h
@@ -487,6 +487,19 @@ static inline int64_t quic_pktns_get_largest_acked_pn(struct quic_pktns *pktns)
        return eb64_entry(ar, struct quic_arng_node, first)->last;
 }
 
+/* The TX packets sent in the same datagram are linked to each others in
+ * the order they are built. This function detach a packet from its successor
+ * and predecessor in the same datagram.
+ */
+static inline void quic_tx_packet_dgram_detach(struct quic_tx_packet *pkt)
+{
+       if (pkt->prev)
+               pkt->prev->next = pkt->next;
+       if (pkt->next)
+               pkt->next->prev = pkt->prev;
+}
+
+
 /* Increment the reference counter of <pkt> */
 static inline void quic_tx_packet_refinc(struct quic_tx_packet *pkt)
 {
@@ -498,6 +511,10 @@ static inline void quic_tx_packet_refdec(struct quic_tx_packet *pkt)
 {
        if (!HA_ATOMIC_SUB_FETCH(&pkt->refcnt, 1)) {
                BUG_ON(!LIST_ISEMPTY(&pkt->frms));
+               /* If there are others packet in the same datagram <pkt> is attached to,
+                * detach the previous one and the next one from <pkt>.
+                */
+               quic_tx_packet_dgram_detach(pkt);
                pool_free(pool_head_quic_tx_packet, pkt);
        }
 }

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 1d67248c6763507726d0a9179b37495f974becef..dd748945f449ef777a21530fbea3ac70e5f6ccdc 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1759,10 +1759,7 @@ static inline struct eb64_node *qc_ackrng_pkts(struct quic_conn *qc,
                /* If there are others packet in the same datagram <pkt> is attached to,
                 * detach the previous one and the next one from <pkt>.
                 */
-               if (pkt->prev)
-                       pkt->prev->next = pkt->next;
-               if (pkt->next)
-                       pkt->next->prev = pkt->prev;
+               quic_tx_packet_dgram_detach(pkt);
                node = eb64_prev(node);
                eb64_delete(&pkt->pn_node);
        }
@@ -3237,7 +3234,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
                /* keep trace of the first packet in the datagram */
                if (!first_pkt)
                        first_pkt = cur_pkt;
-               /* Attach the current one to the previous one */
+               /* Attach the current one to the previous one and vice versa */
                if (prv_pkt) {
                        prv_pkt->next = cur_pkt;
                        cur_pkt->prev = prv_pkt;