From 235e8f1afd7e9753a26051b30c47ecc398ccfd12 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 19 Dec 2023 11:25:18 +0100 Subject: [PATCH] MEDIUM: mux-quic: add BUG_ON if sending on locally closed QCS Previously, if snd_buf operation was conducted despite QCS already locally closed, the input buffer was silently dropped. This situation could happen if a RESET_STREAM was emitted butemission not reported to the stream layer. Resetting silently the buffer ensure QUIC MUX remain compliant with RFC 9000 which forbid emission after RESET_STREAM. Since previous commit, it is now ensured that RESET_STREAM sending will always be reported to stream-layer. Thus, there is no need anymore to silently reset the buffer. A BUG_ON() statement is added to ensure this assumption will remain valid. The new code is deemed cleaner as it does not hide a missing error notification on the stconn-layer. Previously, if an error was missing, sending would continue unnecessarily with a false success status reported for the stream. Note that the BUG_ON() statement was also added into nego_ff callback. This is necessary to ensure both sending path remains consistent. This patch is labelled as MEDIUM as issues were already encountered in snd_buf/nego_ff implementation and it's not easy to cover all occurences during test. If the BUG_ON() is triggered without any apparent stream-layer issue, this commit should be reverted. --- include/haproxy/qmux_http.h | 1 - src/mux_quic.c | 11 ++++++----- src/qmux_http.c | 20 -------------------- 3 files changed, 6 insertions(+), 26 deletions(-) diff --git a/include/haproxy/qmux_http.h b/include/haproxy/qmux_http.h index a7dbe7cc3..4a7711401 100644 --- a/include/haproxy/qmux_http.h +++ b/include/haproxy/qmux_http.h @@ -10,7 +10,6 @@ size_t qcs_http_rcv_buf(struct qcs *qcs, struct buffer *buf, size_t count, char *fin); size_t qcs_http_snd_buf(struct qcs *qcs, struct buffer *buf, size_t count, char *fin); -size_t qcs_http_reset_buf(struct qcs *qcs, struct buffer *buf, size_t count); #endif /* USE_QUIC */ diff --git a/src/mux_quic.c b/src/mux_quic.c index bf3616e0c..21b1cdec7 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -2811,6 +2811,9 @@ static size_t qmux_strm_snd_buf(struct stconn *sc, struct buffer *buf, TRACE_ENTER(QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs); + /* Sending forbidden if QCS is locally closed (FIN or RESET_STREAM sent). */ + BUG_ON(qcs_is_close_local(qcs) || (qcs->flags & QC_SF_TO_RESET)); + /* stream layer has been detached so no transfer must occur after. */ BUG_ON_HOT(qcs->flags & QC_SF_DETACH); @@ -2821,11 +2824,6 @@ static size_t qmux_strm_snd_buf(struct stconn *sc, struct buffer *buf, goto end; } - if (qcs_is_close_local(qcs) || (qcs->flags & QC_SF_TO_RESET)) { - ret = qcs_http_reset_buf(qcs, buf, count); - goto end; - } - ret = qcs_http_snd_buf(qcs, buf, count, &fin); if (fin) { TRACE_STATE("reached stream fin", QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs); @@ -2853,6 +2851,9 @@ static size_t qmux_strm_nego_ff(struct stconn *sc, struct buffer *input, TRACE_ENTER(QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs); + /* Sending forbidden if QCS is locally closed (FIN or RESET_STREAM sent). */ + BUG_ON(qcs_is_close_local(qcs) || (qcs->flags & QC_SF_TO_RESET)); + /* stream layer has been detached so no transfer must occur after. */ BUG_ON_HOT(qcs->flags & QC_SF_DETACH); diff --git a/src/qmux_http.c b/src/qmux_http.c index 3c5f3917c..092eb15da 100644 --- a/src/qmux_http.c +++ b/src/qmux_http.c @@ -95,23 +95,3 @@ size_t qcs_http_snd_buf(struct qcs *qcs, struct buffer *buf, size_t count, return ret; } - -/* QUIC MUX snd_buf reset. HTX data stored in of length will be - * cleared. This can be used when data should not be transmitted any longer. - * - * Return the size in bytes of cleared data. - */ -size_t qcs_http_reset_buf(struct qcs *qcs, struct buffer *buf, size_t count) -{ - struct htx *htx; - - TRACE_ENTER(QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs); - - htx = htx_from_buf(buf); - htx_reset(htx); - htx_to_buf(htx, buf); - - TRACE_LEAVE(QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs); - - return count; -}