From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Wed, 6 Nov 2024 16:18:45 +0000 (+0100)
Subject: BUG/MEDIUM: quic: do not consider ACK on released stream as error
X-Git-Tag: v3.1-dev12~19
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3b851a326b96286c5887566ceee4a9f925adcc58;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: quic: do not consider ACK on released stream as error

When an ACK is received by haproxy, a lookup is performed to retrieve
the related emitted frames. For STREAM type frames, a lookup is
performed under quic_conn stream_desc tree. Indeed, the corresponding
stream instance could be already released if multiple ACK were received
refering to the same stream offset, which can happen notably if
retransmission occured.

qc_handle_newly_acked_frm() implements this logic. If the case with an
already released stream is encounted, an error is returned. In the end,
this error is propagated via qc_parse_pkt_frms() into
qc_treat_rx_pkts(), despite being in fact a perfectly valid case. Fix
this by adjusting ACK handling function to return a success value for
the particular case of released stream instead.

The impact of this bug is unknown, but it can have several consequences.
* if the packet with the ACK contains other frames after it, their
  content will be skipped
* the packet won't be acknowledged by haproxy, even if it contains other
  frames and is ack-eliciting. This may cause unneeded retransmission by
  the client.
* RTT sampling information related to this ACK is ignored by haproxy

Finally, it also caused the increment of the quic_conn counter
dropped_parsing (droppars in "show quic" output) which should be
reserved only for real error cases.

This regression is present since the following patch :
  e7578084b0536e3e5988be7f09091c85beb8fa9d
  MINOR: quic: implement dedicated type for out-of-order stream ACK

Before, qc_handle_newly_acked_frm() return type was always ignored. As
such, no backport is needed.
---

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 3204f3fe5b..69a36ec77c 100644
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -232,8 +232,7 @@ static int qc_handle_newly_acked_frm(struct quic_conn *qc, struct quic_frame *fr
 		if (!node) {
 			TRACE_DEVEL("acked stream for released stream", QUIC_EV_CONN_ACKSTRM, qc, strm_frm);
 			qc_release_frm(qc, frm);
-			/* early return */
-			goto leave;
+			/* return as success */
 		}
 		else {
 			stream = eb64_entry(node, struct qc_stream_desc, by_id);