From 24962dd1784dd22babc8da09a5fc8769617f89e3 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 24 Apr 2023 17:50:23 +0200 Subject: [PATCH] BUG/MEDIUM: mux-quic: do not emit RESET_STREAM for unknown length Some HTX responses may not always contain a EOM block. For example this is the case if content-length header is missing from the HTTP server response. Stream termination is thus signaled to QUIC mux via shutw callback. However, this is interpreted inconditionnally as an early close by the mux with a RESET_STREAM emission. Most of the times, QUIC clients report this as an error. To fix this, check if htx.extra is set to HTX_UNKOWN_PAYLOAD_LENGTH for a qcs instance. If true, shutw will never be used to emit a RESET_STREAM. Instead, the stream will be closed properly with a FIN STREAM frame. If all data were already transfered, an empty STREAM frame is sent. This fix may help with the github issue #2004 where chrome browser stop to use QUIC after receiving RESET_STREAM frames. This issue was reported by Vladimir Zakharychev. Thanks to him for his help and testing. It was also reproduced locally using httpterm with the query string "/?s=1k&b=0&C=1". This should be backported up to 2.7. --- include/haproxy/mux_quic-t.h | 1 + src/mux_quic.c | 31 +++++++++++++++++++++---------- src/qmux_http.c | 3 +++ 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/include/haproxy/mux_quic-t.h b/include/haproxy/mux_quic-t.h index 7b4dc91e7..a4bae20fa 100644 --- a/include/haproxy/mux_quic-t.h +++ b/include/haproxy/mux_quic-t.h @@ -123,6 +123,7 @@ struct qcc { #define QC_SF_TO_RESET 0x00000080 /* a RESET_STREAM must be sent */ #define QC_SF_HREQ_RECV 0x00000100 /* a full HTTP request has been received */ #define QC_SF_TO_STOP_SENDING 0x00000200 /* a STOP_SENDING must be sent */ +#define QC_SF_UNKNOWN_PL_LENGTH 0x00000400 /* HTX EOM may be missing from the stream layer */ /* Maximum size of stream Rx buffer. */ #define QC_S_RX_BUF_SZ (global.tune.bufsize - NCB_RESERVED_SZ) diff --git a/src/mux_quic.c b/src/mux_quic.c index 74e0fc854..e6ac7ffa8 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -2567,8 +2567,10 @@ static size_t qc_send_buf(struct stconn *sc, struct buffer *buf, } ret = qcs_http_snd_buf(qcs, buf, count, &fin); - if (fin) + if (fin) { + TRACE_STATE("reached stream fin", QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs); qcs->flags |= QC_SF_FIN_STREAM; + } if (ret || fin) { qcc_send_stream(qcs, 0); @@ -2668,21 +2670,30 @@ static int qc_wake(struct connection *conn) static void qc_shutw(struct stconn *sc, enum co_shw_mode mode) { struct qcs *qcs = __sc_mux_strm(sc); + struct qcc *qcc = qcs->qcc; - TRACE_ENTER(QMUX_EV_STRM_SHUT, qcs->qcc->conn, qcs); - - /* If QC_SF_FIN_STREAM is not set and stream is not closed locally, it - * means that upper layer reported an early closure. A RESET_STREAM is - * necessary if not already scheduled. - */ + TRACE_ENTER(QMUX_EV_STRM_SHUT, qcc->conn, qcs); + /* Early closure reported if QC_SF_FIN_STREAM not yet set. */ if (!qcs_is_close_local(qcs) && !(qcs->flags & (QC_SF_FIN_STREAM|QC_SF_TO_RESET))) { - qcc_reset_stream(qcs, 0); - se_fl_set_error(qcs->sd); + + if (qcs->flags & QC_SF_UNKNOWN_PL_LENGTH) { + /* Close stream with a FIN STREAM frame. */ + TRACE_STATE("set FIN STREAM", QMUX_EV_STRM_SHUT, qcc->conn, qcs); + qcs->flags |= QC_SF_FIN_STREAM; + qcc_send_stream(qcs, 0); + } + else { + /* RESET_STREAM necessary. */ + qcc_reset_stream(qcs, 0); + se_fl_set_error(qcs->sd); + } + + tasklet_wakeup(qcc->wait_event.tasklet); } - TRACE_LEAVE(QMUX_EV_STRM_SHUT, qcs->qcc->conn, qcs); + TRACE_LEAVE(QMUX_EV_STRM_SHUT, qcc->conn, qcs); } /* for debugging with CLI's "show sess" command. May emit multiple lines, each diff --git a/src/qmux_http.c b/src/qmux_http.c index 6eedf0c4a..73b00389c 100644 --- a/src/qmux_http.c +++ b/src/qmux_http.c @@ -78,6 +78,9 @@ size_t qcs_http_snd_buf(struct qcs *qcs, struct buffer *buf, size_t count, htx = htx_from_buf(buf); + if (htx->extra && htx->extra == HTX_UNKOWN_PAYLOAD_LENGTH) + qcs->flags |= QC_SF_UNKNOWN_PL_LENGTH; + ret = qcs->qcc->app_ops->snd_buf(qcs, htx, count); *fin = (htx->flags & HTX_FL_EOM) && htx_is_empty(htx);