From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Fri, 18 Oct 2024 15:46:06 +0000 (+0200)
Subject: BUG/MINOR: mux-quic: do not close STREAM with empty FIN if no data sent
X-Git-Tag: v3.1-dev11~90
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=68c8c910238f0b759d75b4da2128370abf184cd1;p=thirdparty%2Fhaproxy.git

BUG/MINOR: mux-quic: do not close STREAM with empty FIN if no data sent

A stream may be shut without any HTX EOM reported to report a proper
closure. This is the case for QCS instances flagged with
QC_SF_UNKNOWN_PL_LENGTH. Shut is performed with an empty FIN emission
instead of a RESET_STREAM. This has been implemented since the following
patch :

  24962dd1784dd22babc8da09a5fc8769617f89e3
  BUG/MEDIUM: mux-quic: do not emit RESET_STREAM for unknown length

However, in case of HTTP/3, an empty FIN should only be done after a
full message is emitted, which requires at least a HEADERS frame. If an
empty FIN is emitted without it, client may interpret this as invalid
and close the connection. To prevent this, fallback to a RESET_STREAM
emission if no data were emitted on the stream.

This was reproduced using ngtcp2-client with 10% loss (-r 0.1) on a
remote host, with httpterm request "/?s=100k&C=1&b=0&P=400". An error
ERR_H3_FRAME_UNEXPECTED is returned by ngtcp2-client when the bug
occurs.

Note that this change is incomplete. The message validity depends solely
on the application protocol in use. As such, a new app_ops callback
should be implemented to ensure the stream is closed accordingly.
However, this first patch ensures that at least HTTP/3 case is valid
while keeping a minimal backport process.

This should be backported up to 2.8.
---

diff --git a/src/mux_quic.c b/src/mux_quic.c
index ea8c38b5f7..302dc155b2 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -3359,8 +3359,12 @@ static void qmux_strm_shut(struct stconn *sc, unsigned int mode, struct se_abort
 	if (!qcs_is_close_local(qcs) &&
 	    !(qcs->flags & (QC_SF_FIN_STREAM|QC_SF_TO_RESET))) {
 
-		if (qcs->flags & QC_SF_UNKNOWN_PL_LENGTH) {
-			/* Close stream with a FIN STREAM frame. */
+		/* Close stream with FIN if length unknown and some data are
+		 * ready to be/already transmitted.
+		 * TODO select closure method on app proto layer
+		 */
+		if (qcs->flags & QC_SF_UNKNOWN_PL_LENGTH &&
+		    qcs->tx.fc.off_soft) {
 			if (!(qcc->flags & (QC_CF_ERR_CONN|QC_CF_ERRL))) {
 				TRACE_STATE("set FIN STREAM",
 				            QMUX_EV_STRM_SHUT, qcc->conn, qcs);
@@ -3485,7 +3489,7 @@ static const struct mux_ops qmux_ops = {
 	.subscribe   = qmux_strm_subscribe,
 	.unsubscribe = qmux_strm_unsubscribe,
 	.wake        = qmux_wake,
-	.shut       = qmux_strm_shut,
+	.shut        = qmux_strm_shut,
 	.ctl         = qmux_ctl,
 	.sctl        = qmux_sctl,
 	.show_sd     = qmux_strm_show_sd,