From 58721f2192e0ff2cf8c2cd84cddd83d4de717314 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 9 May 2023 18:01:09 +0200 Subject: [PATCH] BUG/MINOR: mux-quic: fix transport VS app CONNECTION_CLOSE A recent series of patch were introduced to streamline error generation by QUIC MUX. However, a regression was introduced : every error generated by the MUX was built as CONNECTION_CLOSE_APP frame, whereas it should be only for H3/QPACK errors. Fix this by adding an argument in qcc_set_error. When false, a standard CONNECTION_CLOSE is used as error. This bug was detected by QUIC tracker with the following tests "stop_sending" and "server_flow_control" which requires a CONNECTION_CLOSE frame. This must be backported up to 2.7. --- include/haproxy/mux_quic.h | 2 +- src/h3.c | 18 +++++++++--------- src/mux_quic.c | 37 +++++++++++++++++++------------------ src/qpack-dec.c | 4 ++-- 4 files changed, 31 insertions(+), 30 deletions(-) diff --git a/include/haproxy/mux_quic.h b/include/haproxy/mux_quic.h index e425e61d2..0e408bd18 100644 --- a/include/haproxy/mux_quic.h +++ b/include/haproxy/mux_quic.h @@ -12,7 +12,7 @@ #include #include -void qcc_set_error(struct qcc *qcc, int err); +void qcc_set_error(struct qcc *qcc, int err, int app); struct qcs *qcc_init_stream_local(struct qcc *qcc, int bidi); struct buffer *qc_get_buf(struct qcs *qcs, struct buffer *bptr); diff --git a/src/h3.c b/src/h3.c index a364212f5..e686f0de0 100644 --- a/src/h3.c +++ b/src/h3.c @@ -191,7 +191,7 @@ static ssize_t h3_init_uni_stream(struct h3c *h3c, struct qcs *qcs, case H3_UNI_S_T_CTRL: if (h3c->flags & H3_CF_UNI_CTRL_SET) { TRACE_ERROR("duplicated control stream", H3_EV_H3S_NEW, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, H3_STREAM_CREATION_ERROR); + qcc_set_error(qcs->qcc, H3_STREAM_CREATION_ERROR, 1); goto err; } h3c->flags |= H3_CF_UNI_CTRL_SET; @@ -206,7 +206,7 @@ static ssize_t h3_init_uni_stream(struct h3c *h3c, struct qcs *qcs, case H3_UNI_S_T_QPACK_DEC: if (h3c->flags & H3_CF_UNI_QPACK_DEC_SET) { TRACE_ERROR("duplicated qpack decoder stream", H3_EV_H3S_NEW, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, H3_STREAM_CREATION_ERROR); + qcc_set_error(qcs->qcc, H3_STREAM_CREATION_ERROR, 1); goto err; } h3c->flags |= H3_CF_UNI_QPACK_DEC_SET; @@ -217,7 +217,7 @@ static ssize_t h3_init_uni_stream(struct h3c *h3c, struct qcs *qcs, case H3_UNI_S_T_QPACK_ENC: if (h3c->flags & H3_CF_UNI_QPACK_ENC_SET) { TRACE_ERROR("duplicated qpack encoder stream", H3_EV_H3S_NEW, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, H3_STREAM_CREATION_ERROR); + qcc_set_error(qcs->qcc, H3_STREAM_CREATION_ERROR, 1); goto err; } h3c->flags |= H3_CF_UNI_QPACK_ENC_SET; @@ -1031,7 +1031,7 @@ static ssize_t h3_decode_qcs(struct qcs *qcs, struct buffer *b, int fin) */ if (h3s->type == H3S_T_CTRL && fin) { TRACE_ERROR("control stream closed by remote peer", H3_EV_RX_FRAME, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM); + qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM, 1); goto err; } @@ -1067,7 +1067,7 @@ static ssize_t h3_decode_qcs(struct qcs *qcs, struct buffer *b, int fin) if (!h3_is_frame_valid(h3c, qcs, ftype)) { TRACE_ERROR("received an invalid frame", H3_EV_RX_FRAME, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, H3_FRAME_UNEXPECTED); + qcc_set_error(qcs->qcc, H3_FRAME_UNEXPECTED, 1); goto err; } @@ -1090,7 +1090,7 @@ static ssize_t h3_decode_qcs(struct qcs *qcs, struct buffer *b, int fin) */ if (flen > QC_S_RX_BUF_SZ) { TRACE_ERROR("received a too big frame", H3_EV_RX_FRAME, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, H3_EXCESSIVE_LOAD); + qcc_set_error(qcs->qcc, H3_EXCESSIVE_LOAD, 1); goto err; } break; @@ -1129,7 +1129,7 @@ static ssize_t h3_decode_qcs(struct qcs *qcs, struct buffer *b, int fin) ret = h3_parse_settings_frm(qcs->qcc->ctx, b, flen); if (ret < 0) { TRACE_ERROR("error on SETTINGS parsing", H3_EV_RX_FRAME, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, h3c->err); + qcc_set_error(qcs->qcc, h3c->err, 1); goto err; } h3c->flags |= H3_CF_SETTINGS_RECV; @@ -1163,7 +1163,7 @@ static ssize_t h3_decode_qcs(struct qcs *qcs, struct buffer *b, int fin) return b_data(b); } else if (h3c->err) { - qcc_set_error(qcs->qcc, h3c->err); + qcc_set_error(qcs->qcc, h3c->err, 1); return b_data(b); } @@ -1670,7 +1670,7 @@ static int h3_close(struct qcs *qcs, enum qcc_app_ops_close_side side) */ if (qcs == h3c->ctrl_strm || h3s->type == H3S_T_CTRL) { TRACE_ERROR("closure detected on control stream", H3_EV_H3S_END, qcs->qcc->conn, qcs); - qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM); + qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM, 1); return 1; } diff --git a/src/mux_quic.c b/src/mux_quic.c index e816613e8..705008872 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -497,10 +497,11 @@ void qcs_notify_send(struct qcs *qcs) } /* A fatal error is detected locally for connection. It should be closed - * with a CONNECTION_CLOSE using code. This function must not be called - * more than once by connection. + * with a CONNECTION_CLOSE using code. Set to true to indicate that + * the code must be considered as an application level error. This function + * must not be called more than once by connection. */ -void qcc_set_error(struct qcc *qcc, int err) +void qcc_set_error(struct qcc *qcc, int err, int app) { /* This must not be called multiple times per connection. */ BUG_ON(qcc->flags & QC_CF_ERRL); @@ -508,7 +509,7 @@ void qcc_set_error(struct qcc *qcc, int err) TRACE_STATE("connection on error", QMUX_EV_QCC_ERR, qcc->conn); qcc->flags |= QC_CF_ERRL; - qcc->err = quic_err_app(err); + qcc->err = app ? quic_err_app(err) : quic_err_transport(err); } /* Open a locally initiated stream for the connection . Set for a @@ -541,7 +542,7 @@ struct qcs *qcc_init_stream_local(struct qcc *qcc, int bidi) qcs = qcs_new(qcc, *next, type); if (!qcs) { TRACE_LEAVE(QMUX_EV_QCS_NEW, qcc->conn); - qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR); + qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR, 0); return NULL; } @@ -588,7 +589,7 @@ static struct qcs *qcc_init_stream_remote(struct qcc *qcc, uint64_t id) qcc->lfctl.ms_uni * 4; if (id >= max_id) { TRACE_ERROR("flow control error", QMUX_EV_QCS_NEW|QMUX_EV_PROTO_ERR, qcc->conn); - qcc_set_error(qcc, QC_ERR_STREAM_LIMIT_ERROR); + qcc_set_error(qcc, QC_ERR_STREAM_LIMIT_ERROR, 0); goto err; } @@ -602,7 +603,7 @@ static struct qcs *qcc_init_stream_remote(struct qcc *qcc, uint64_t id) qcs = qcs_new(qcc, *largest, type); if (!qcs) { TRACE_ERROR("stream fallocation failure", QMUX_EV_QCS_NEW, qcc->conn); - qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR); + qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR, 0); goto err; } @@ -661,13 +662,13 @@ int qcc_get_qcs(struct qcc *qcc, uint64_t id, int receive_only, int send_only, if (!receive_only && quic_stream_is_uni(id) && quic_stream_is_remote(qcc, id)) { TRACE_ERROR("receive-only stream not allowed", QMUX_EV_QCC_RECV|QMUX_EV_QCC_NQCS|QMUX_EV_PROTO_ERR, qcc->conn, NULL, &id); - qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR); + qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR, 0); goto err; } if (!send_only && quic_stream_is_uni(id) && quic_stream_is_local(qcc, id)) { TRACE_ERROR("send-only stream not allowed", QMUX_EV_QCC_RECV|QMUX_EV_QCC_NQCS|QMUX_EV_PROTO_ERR, qcc->conn, NULL, &id); - qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR); + qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR, 0); goto err; } @@ -699,7 +700,7 @@ int qcc_get_qcs(struct qcc *qcc, uint64_t id, int receive_only, int send_only, * stream. */ TRACE_ERROR("locally initiated stream not yet created", QMUX_EV_QCC_RECV|QMUX_EV_QCC_NQCS|QMUX_EV_PROTO_ERR, qcc->conn, NULL, &id); - qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR); + qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR, 0); goto err; } else { @@ -755,7 +756,7 @@ static void qcs_consume(struct qcs *qcs, uint64_t bytes) TRACE_DATA("increase stream credit via MAX_STREAM_DATA", QMUX_EV_QCS_RECV, qcc->conn, qcs); frm = qc_frm_alloc(QUIC_FT_MAX_STREAM_DATA); if (!frm) { - qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR); + qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR, 0); return; } @@ -774,7 +775,7 @@ static void qcs_consume(struct qcs *qcs, uint64_t bytes) TRACE_DATA("increase conn credit via MAX_DATA", QMUX_EV_QCS_RECV, qcc->conn, qcs); frm = qc_frm_alloc(QUIC_FT_MAX_DATA); if (!frm) { - qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR); + qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR, 0); return; } @@ -989,7 +990,7 @@ int qcc_recv(struct qcc *qcc, uint64_t id, uint64_t len, uint64_t offset, if (qcs->flags & QC_SF_SIZE_KNOWN && (offset + len > qcs->rx.offset_max || (fin && offset + len < qcs->rx.offset_max))) { TRACE_ERROR("final size error", QMUX_EV_QCC_RECV|QMUX_EV_QCS_RECV|QMUX_EV_PROTO_ERR, qcc->conn, qcs); - qcc_set_error(qcc, QC_ERR_FINAL_SIZE_ERROR); + qcc_set_error(qcc, QC_ERR_FINAL_SIZE_ERROR, 0); goto err; } @@ -1022,7 +1023,7 @@ int qcc_recv(struct qcc *qcc, uint64_t id, uint64_t len, uint64_t offset, */ TRACE_ERROR("flow control error", QMUX_EV_QCC_RECV|QMUX_EV_QCS_RECV|QMUX_EV_PROTO_ERR, qcc->conn, qcs); - qcc_set_error(qcc, QC_ERR_FLOW_CONTROL_ERROR); + qcc_set_error(qcc, QC_ERR_FLOW_CONTROL_ERROR, 0); goto err; } } @@ -1061,7 +1062,7 @@ int qcc_recv(struct qcc *qcc, uint64_t id, uint64_t len, uint64_t offset, */ TRACE_ERROR("overlapping data rejected", QMUX_EV_QCC_RECV|QMUX_EV_QCS_RECV|QMUX_EV_PROTO_ERR, qcc->conn, qcs); - qcc_set_error(qcc, QC_ERR_PROTOCOL_VIOLATION); + qcc_set_error(qcc, QC_ERR_PROTOCOL_VIOLATION, 0); return 1; case NCB_RET_GAP_SIZE: @@ -1194,7 +1195,7 @@ int qcc_recv_reset_stream(struct qcc *qcc, uint64_t id, uint64_t err, uint64_t f */ if (qcc_get_qcs(qcc, id, 1, 0, &qcs)) { TRACE_ERROR("RESET_STREAM for send-only stream received", QMUX_EV_QCC_RECV|QMUX_EV_QCS_RECV, qcc->conn, qcs); - qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR); + qcc_set_error(qcc, QC_ERR_STREAM_STATE_ERROR, 0); goto err; } @@ -1214,7 +1215,7 @@ int qcc_recv_reset_stream(struct qcc *qcc, uint64_t id, uint64_t err, uint64_t f if (qcs->rx.offset_max > final_size || ((qcs->flags & QC_SF_SIZE_KNOWN) && qcs->rx.offset_max != final_size)) { TRACE_ERROR("final size error on RESET_STREAM", QMUX_EV_QCC_RECV|QMUX_EV_QCS_RECV, qcc->conn, qcs); - qcc_set_error(qcc, QC_ERR_FINAL_SIZE_ERROR); + qcc_set_error(qcc, QC_ERR_FINAL_SIZE_ERROR, 0); goto err; } @@ -1341,7 +1342,7 @@ static int qcc_release_remote_stream(struct qcc *qcc, uint64_t id) TRACE_DATA("increase max stream limit with MAX_STREAMS_BIDI", QMUX_EV_QCC_SEND, qcc->conn); frm = qc_frm_alloc(QUIC_FT_MAX_STREAMS_BIDI); if (!frm) { - qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR); + qcc_set_error(qcc, QC_ERR_INTERNAL_ERROR, 0); goto err; } diff --git a/src/qpack-dec.c b/src/qpack-dec.c index 1ae2ef35d..97392bbc3 100644 --- a/src/qpack-dec.c +++ b/src/qpack-dec.c @@ -111,7 +111,7 @@ int qpack_decode_enc(struct buffer *buf, int fin, void *ctx) * connection error of type H3_CLOSED_CRITICAL_STREAM. */ if (fin) { - qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM); + qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM, 1); return -1; } @@ -158,7 +158,7 @@ int qpack_decode_dec(struct buffer *buf, int fin, void *ctx) * connection error of type H3_CLOSED_CRITICAL_STREAM. */ if (fin) { - qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM); + qcc_set_error(qcs->qcc, H3_CLOSED_CRITICAL_STREAM, 1); return -1; }