From: Frédéric Lécaille <flecaille@haproxy.com>
Date: Wed, 27 Apr 2022 05:17:56 +0000 (+0200)
Subject: BUG/MEDIUM: quic: Possible crash on STREAM frame loss
X-Git-Tag: v2.6-dev8~18
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=77cb38d22d4f1f6b0edae2417a65c2316906796f;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: quic: Possible crash on STREAM frame loss

A crash is possible under such circumtances:
    - The congestion window is drastically reduced to its miniaml value
    when a quic listener is experiencing extreme packet loss ;
    - we enqueue several STREAM frames to be resent and some of them could not be
    transmitted ;
    - some of the still in flight are acknowledged and trigger the
    stream memory releasing ;
    - when we come back to send the remaing STREAM frames, haproxy
    crashes when it tries to build them.

To fix this issue, we mark the STREAM frame as lost when detected as lost.
Then a lookup if performed for the stream the STREAM frames are attached
to before building them. They are released if the stream is no more available
or the data range of the frame is consumed.
---

diff --git a/include/haproxy/quic_frame-t.h b/include/haproxy/quic_frame-t.h
index ebd416bfc7..5636cc810d 100644
--- a/include/haproxy/quic_frame-t.h
+++ b/include/haproxy/quic_frame-t.h
@@ -96,6 +96,8 @@ enum quic_frame_type {
 
 /* Flag a TX frame as acknowledged */
 #define QUIC_FL_TX_FRAME_ACKED             0x01
+/* Flag a TX frame as lost */
+#define QUIC_FL_TX_FRAME_LOST              0x02
 
 #define QUIC_STREAM_FRAME_TYPE_FIN_BIT     0x01
 #define QUIC_STREAM_FRAME_TYPE_LEN_BIT     0x02
diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 12833c352e..0065e7fcf0 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -1717,6 +1717,13 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
 		}
 		else {
 			TRACE_PROTO("to resend frame", QUIC_EV_CONN_PRSAFRM, qc, frm);
+			if (QUIC_FT_STREAM_8 <= frm->type && frm->type <= QUIC_FT_STREAM_F) {
+				/* Mark this STREAM frame as lost. A look up their stream descriptor
+				 * will be performed to check the stream is not consumed or released.
+				 */
+				fprintf(stderr, "LOST STREAM FRAME\n");
+				frm->flags = QUIC_FL_TX_FRAME_LOST;
+			}
 			LIST_APPEND(&tmp, &frm->list);
 		}
 	}
@@ -5541,6 +5548,37 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
 			break;
 
 		case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F:
+			if (cf->flags & QUIC_FL_TX_FRAME_LOST) {
+				struct eb64_node *node = NULL;
+				struct qc_stream_desc *stream_desc = NULL;
+				struct quic_stream *strm = &cf->stream;
+
+				/* As this frame has been already lost, ensure the stream is always
+				 * available or the range of this frame is not consumed before
+				 * resending it.
+				 */
+				node = eb64_lookup(&qc->streams_by_id, strm->id);
+				if (!node) {
+					TRACE_PROTO("released stream", QUIC_EV_CONN_PRSAFRM, qc, strm);
+					LIST_DELETE(&cf->list);
+					pool_free(pool_head_quic_frame, cf);
+					continue;
+				}
+
+				stream_desc = eb64_entry(node, struct qc_stream_desc, by_id);
+				if (strm->offset.key + strm->len <= stream_desc->ack_offset) {
+					TRACE_PROTO("ignored frame frame in already acked range",
+					            QUIC_EV_CONN_PRSAFRM, qc, strm);
+					LIST_DELETE(&cf->list);
+					pool_free(pool_head_quic_frame, cf);
+					continue;
+				}
+				else if (strm->offset.key < stream_desc->ack_offset) {
+					strm->offset.key = stream_desc->ack_offset;
+					TRACE_PROTO("updated partially acked frame",
+					            QUIC_EV_CONN_PRSAFRM, qc, strm);
+				}
+			}
 			/* Note that these frames are accepted in short packets only without
 			 * "Length" packet field. Here, <*len> is used only to compute the
 			 * sum of the lengths of the already built frames for this packet.