From: Frédéric Lécaille <flecaille@haproxy.com>
Date: Thu, 16 Mar 2023 11:30:36 +0000 (+0100)
Subject: BUG/MINOR: quic: Missing STREAM frame length updates
X-Git-Tag: v2.8-dev6~59
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fc546ab6a7346605c62760569c9afd0c99c6dd76;p=thirdparty%2Fhaproxy.git

BUG/MINOR: quic: Missing STREAM frame length updates

Some STREAM frame lengths were not updated before being duplicated, built
of requeued contrary to their ack offsets. This leads haproxy to crash when
receiving acknowledgements for such frames with this thread #1 backtrace:

 Thread 1 (Thread 0x7211b6ffd640 (LWP 986141)):
 #0  ha_crash_now () at include/haproxy/bug.h:52
 No locals.
 #1  b_del (b=<optimized out>, del=<optimized out>) at include/haproxy/buf.h:436
 No locals.
 #2  qc_stream_desc_ack (stream=stream@entry=0x7211b6fd9bc8, offset=offset@entry=53176, len=len@entry=1122) at src/quic_stream.c:111

Thank you to @Tristan971 for having provided such traces which reveal this issue:

[04|quic|5|c_conn.c:1865] qc_requeue_nacked_pkt_tx_frms(): entering : qc@0x72119c22cfe0
[04|quic|5|_frame.c:1179] qc_frm_unref(): entering : qc@0x72119c22cfe0
[04|quic|5|_frame.c:1186] qc_frm_unref(): remove frame reference : qc@0x72119c22cfe0 frm@0x72118863d260 STREAM_F uni=0 fin=1 id=460 off=52957 len=1122 3244
[04|quic|5|_frame.c:1194] qc_frm_unref(): leaving : qc@0x72119c22cfe0
[04|quic|5|c_conn.c:1902] qc_requeue_nacked_pkt_tx_frms(): updated partially acked frame : qc@0x72119c22cfe0 frm@0x72119c472290 STREAM_F uni=0 fin=1 id=460 off=53176 len=1122

Note that haproxy has much more chance to crash if this frame is the last one
(fin bit set). But another condition must be fullfilled to update the ack offset.
A previous STREAM frame from the same stream with the same offset but with less
data must be acknowledged by the peer. This is the condition to update the ack offset.

For others frames without fin bit in the same conditions, I guess the stream may be
truncated because too much data are removed from the stream when they are
acknowledged.

Must be backported to 2.6 and 2.7.
---

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 8623afefbe..6fd8412f2f 100644
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1898,6 +1898,7 @@ static inline int qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
 			}
 			else if (strm_frm->offset.key < stream_desc->ack_offset) {
 				strm_frm->offset.key = stream_desc->ack_offset;
+				strm_frm->len -= stream_desc->ack_offset - strm_frm->offset.key;
 				TRACE_DEVEL("updated partially acked frame",
 				            QUIC_EV_CONN_PRSAFRM, qc, frm);
 			}
@@ -2553,6 +2554,7 @@ static void qc_dup_pkt_frms(struct quic_conn *qc,
 			}
 			else if (strm_frm->offset.key < stream_desc->ack_offset) {
 				strm_frm->offset.key = stream_desc->ack_offset;
+				strm_frm->len -= stream_desc->ack_offset - strm_frm->offset.key;
 				TRACE_DEVEL("updated partially acked frame",
 				            QUIC_EV_CONN_PRSAFRM, qc, frm);
 			}
@@ -7288,6 +7290,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
 				}
 				else if (strm->offset.key < stream_desc->ack_offset) {
 					strm->offset.key = stream_desc->ack_offset;
+					strm->len -= stream_desc->ack_offset - strm->offset.key;
 					TRACE_DEVEL("updated partially acked frame",
 					            QUIC_EV_CONN_PRSAFRM, qc, cf);
 				}