From: Frédéric Lécaille <flecaille@haproxy.com>
Date: Thu, 2 Dec 2021 13:46:19 +0000 (+0100)
Subject: MINOR: quic: Race issue when consuming RX packets buffer
X-Git-Tag: v2.6-dev1~311
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d61bc8db5920a549d66e03ff02461ab163a2e2ee;p=thirdparty%2Fhaproxy.git

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 14f1de27cb..2a361ec7ee 100644
--- 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 2809de92d5..20e3e4efee 100644
--- 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);