MINOR: quic: Race issue when consuming RX packets buffer

Add a null byte to the end of the RX buffer to notify the consumer there is no
more data to treat.
Modify quic_rx_packet_pool_purge() which is the function which remove the
RX packet from the buffer.
Also rename this function to quic_rx_pkts_del().
As the RX packets may be accessed by the QUIC connection handler (quic_conn_io_cb())
the function responsible of decrementing their reference counters must not
access other information than these reference counters! It was a very bad idea
to try to purge the RX buffer asap when executing this function.

diff --git a/include/haproxy/xprt_quic.h b/include/haproxy/xprt_quic.h
index 14f1de27cbc04c1459d0982085d64adc542dabf2..2a361ec7ee6d5242c94ea09c5627e2782d7b8a0a 100644 (file)
--- a/include/haproxy/xprt_quic.h
+++ b/include/haproxy/xprt_quic.h
@@ -1043,13 +1043,21 @@ static inline int qc_pkt_long(const struct quic_rx_packet *pkt)
  * for the connection.
  * Always succeeds.
  */
-static inline void quic_rx_packet_pool_purge(struct quic_conn *qc)
+static inline void quic_rx_pkts_del(struct quic_conn *qc)
 {
        struct quic_rx_packet *pkt, *pktback;
 
        list_for_each_entry_safe(pkt, pktback, &qc->rx.pkt_list, qc_rx_pkt_list) {
-               if (pkt->data != (unsigned char *)b_head(&qc->rx.buf))
+               if (pkt->data != (unsigned char *)b_head(&qc->rx.buf)) {
+                       size_t cdata;
+
+                       cdata = b_contig_data(&qc->rx.buf, 0);
+                       if (cdata && !*b_head(&qc->rx.buf)) {
+                               /* Consume the remaining data */
+                               b_del(&qc->rx.buf, cdata);
+                       }
                        break;
+               }
 
                if (!HA_ATOMIC_LOAD(&pkt->refcnt)) {
                        b_del(&qc->rx.buf, pkt->raw_len);
@@ -1065,30 +1073,14 @@ static inline void quic_rx_packet_refinc(struct quic_rx_packet *pkt)
        HA_ATOMIC_ADD(&pkt->refcnt, 1);
 }
 
-/* Decrement the reference counter of <pkt> */
+/* Decrement the reference counter of <pkt> while remaining positive */
 static inline void quic_rx_packet_refdec(struct quic_rx_packet *pkt)
 {
-       if (HA_ATOMIC_SUB_FETCH(&pkt->refcnt, 1))
-               return;
-
-       if (!pkt->qc) {
-               /* It is possible the connection for this packet has not already been
-                * identified. In such a case, we only need to free this packet.
-                */
-               pool_free(pool_head_quic_rx_packet, pkt);
-       }
-       else {
-               struct quic_conn *qc = pkt->qc;
+       int refcnt;
 
-               HA_RWLOCK_WRLOCK(QUIC_LOCK, &qc->rx.buf_rwlock);
-               if (pkt->data == (unsigned char *)b_head(&qc->rx.buf)) {
-                       b_del(&qc->rx.buf, pkt->raw_len);
-                       LIST_DELETE(&pkt->qc_rx_pkt_list);
-                       pool_free(pool_head_quic_rx_packet, pkt);
-                       quic_rx_packet_pool_purge(qc);
-               }
-               HA_RWLOCK_WRUNLOCK(QUIC_LOCK, &qc->rx.buf_rwlock);
-       }
+       do {
+               refcnt = HA_ATOMIC_LOAD(&pkt->refcnt);
+       } while (refcnt && !HA_ATOMIC_CAS(&pkt->refcnt, &refcnt, refcnt - 1));
 }
 
 /* Increment the reference counter of <pkt> */

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 2809de92d523f73084ba13f4f4b64e1c7ec9a9fc..20e3e4efeeaa4c0387eece841c2ec840a15d6ee9 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -4121,9 +4121,14 @@ static ssize_t qc_lstnr_pkt_rcv(unsigned char **buf, const unsigned char *end,
        }
 
        HA_RWLOCK_WRLOCK(QUIC_LOCK, &qc->rx.buf_rwlock);
+       quic_rx_pkts_del(qc);
        b_cspace = b_contig_space(&qc->rx.buf);
        if (b_cspace < pkt->len) {
                /* Let us consume the remaining contiguous space. */
+               if (b_cspace) {
+                       b_putchr(&qc->rx.buf, 0x00);
+                       b_cspace--;
+               }
                b_add(&qc->rx.buf, b_cspace);
                if (b_contig_space(&qc->rx.buf) < pkt->len) {
                        HA_RWLOCK_WRUNLOCK(QUIC_LOCK, &qc->rx.buf_rwlock);