From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Fri, 15 Apr 2022 09:47:03 +0000 (+0200)
Subject: BUG/MEDIUM: quic: properly clean frames on stream free
X-Git-Tag: v2.6-dev6~8
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f7ff9cbfe123131d9ffa215d5cfca9a5357d1b6d;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: quic: properly clean frames on stream free

A released qc_stream_desc is freed as soon as all its buffer content has
been acknowledged. However, it may still contains other frames waiting
for ACK pointing to deleted buffer content. This can happen on
retransmission.

When freeing a qc_stream_desc, free all its frames in acked_frms tree to
fix memory leak. This may also possibly fix a crash on retransmission.
Now, the frames are properly removed from a packet. This ensure we do
not retransmit a frame whose buffer is deallocated.
---

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 812c253563..36b21e2043 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -1441,6 +1441,27 @@ static int qc_stream_desc_free(struct qc_stream_desc *stream)
 	offer_buffers(NULL, 1);
 
 	if (stream->release) {
+		/* Free frames still waiting for an ACK. Even if the stream buf
+		 * is NULL, some frames could still be not acknowledged. This
+		 * is notably the case for retransmission where multiple frames
+		 * points to the same buffer content.
+		 */
+		struct eb64_node *frm_node = eb64_first(&stream->acked_frms);
+		while (frm_node) {
+			struct quic_stream *strm;
+			struct quic_frame *frm;
+
+			strm = eb64_entry(&frm_node->node, struct quic_stream, offset);
+
+			frm_node = eb64_next(frm_node);
+			eb64_delete(&strm->offset);
+
+			frm = container_of(strm, struct quic_frame, stream);
+			LIST_DELETE(&frm->list);
+			quic_tx_packet_refdec(frm->pkt);
+			pool_free(pool_head_quic_frame, frm);
+		}
+
 		eb64_delete(&stream->by_id);
 		pool_free(pool_head_quic_conn_stream, stream);
 
@@ -1490,8 +1511,10 @@ static int quic_stream_try_to_consume(struct quic_conn *qc,
 		pool_free(pool_head_quic_frame, frm);
 	}
 
-	if (!b_data(&stream->buf))
-		qc_stream_desc_free(stream);
+	if (!b_data(&stream->buf)) {
+		if (qc_stream_desc_free(stream))
+			TRACE_PROTO("stream released and freed", QUIC_EV_CONN_ACKSTRM, qc);
+	}
 
 	return ret;
 }
@@ -1547,8 +1570,14 @@ static inline void qc_treat_acked_tx_frm(struct quic_conn *qc,
 
 				if (!b_data(strm_frm->buf)) {
 					if (qc_stream_desc_free(stream)) {
-						/* early return */
-						return;
+						/* stream is freed at this stage,
+						 * no need to continue.
+						 */
+						TRACE_PROTO("stream released and freed", QUIC_EV_CONN_ACKSTRM, qc);
+						LIST_DELETE(&frm->list);
+						quic_tx_packet_refdec(frm->pkt);
+						pool_free(pool_head_quic_frame, frm);
+						break;
 					}
 				}
 			}