From f8aaf8bdfa40e21b1a2f600c3ed6455bf9b6a763 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Wed, 14 Sep 2022 16:23:47 +0200 Subject: [PATCH] BUG/MEDIUM: mux-quic: fix crash on early app-ops release H3 SETTINGS emission has recently been delayed. The idea is to send it with the first STREAM to reduce sendto syscall invocation. This was implemented in the following patch : 3dd79d378c86b3ebf60e029f518add5f1ed54815 MINOR: h3: Send the h3 settings with others streams (requests) This patch works fine under nominal conditions. However, it will cause a crash if a HTTP/3 connection is released before having sent any data, for example when receiving an invalid first request. In this case, qc_release will first free qcc.app_ops HTTP/3 application protocol layer via release callback. Then qc_send is called to emit any closing frames built by app_ops release invocation. However, in qc_send, as no data has been sent, it will try to complete application layer protocol intialization, with a SETTINGS emission for HTTP/3. Thus, qcc.app_ops is reused, which is invalid as it has been just freed. This will cause a crash with h3_finalize in the call stack. This bug can be reproduced artificially by generating incomplete HTTP/3 requests. This will in time trigger http-request timeout without any data send. This is done by editing qc_handle_strm_frm function. - ret = qcc_recv(qc->qcc, strm_frm->id, strm_frm->len, + ret = qcc_recv(qc->qcc, strm_frm->id, strm_frm->len - 1, strm_frm->offset.key, strm_frm->fin, (char *)strm_frm->data); To fix this, application layer closing API has been adjusted to be done in two-steps. A new shutdown callback is implemented : it is used by the HTTP/3 layer to generate GOAWAY frame in qc_release prologue. Application layer context qcc.app_ops is then freed later in qc_release via the release operation which is now only used to liberate app layer ressources. This fixes the problem as the intermediary qc_send invocation will be able to reuse app_ops before it is freed. This patch fixes the crash, but it would be better to adjust H3 SETTINGS emission in case of early connection closing : in this case, there is no need to send it. This should be implemented in a future patch. This should fix the crash recently experienced by Tristan in github issue #1801. This must be backported up to 2.6. --- include/haproxy/mux_quic-t.h | 1 + src/h3.c | 10 ++++++++-- src/mux_quic.c | 9 ++++++--- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/include/haproxy/mux_quic-t.h b/include/haproxy/mux_quic-t.h index abf00bfc3..1670262a1 100644 --- a/include/haproxy/mux_quic-t.h +++ b/include/haproxy/mux_quic-t.h @@ -188,6 +188,7 @@ struct qcc_app_ops { size_t (*snd_buf)(struct stconn *sc, struct buffer *buf, size_t count, int flags); void (*detach)(struct qcs *qcs); int (*finalize)(void *ctx); + void (*shutdown)(void *ctx); /* Close a connection. */ void (*release)(void *ctx); void (*inc_err_cnt)(void *ctx, int err_code); }; diff --git a/src/h3.c b/src/h3.c index 6c6764851..8d13f5d09 100644 --- a/src/h3.c +++ b/src/h3.c @@ -1203,7 +1203,8 @@ static int h3_init(struct qcc *qcc) return 0; } -static void h3_release(void *ctx) +/* Send a HTTP/3 GOAWAY followed by a CONNECTION_CLOSE_APP. */ +static void h3_shutdown(void *ctx) { struct h3c *h3c = ctx; @@ -1223,7 +1224,11 @@ static void h3_release(void *ctx) * the connection. */ qcc_emit_cc_app(h3c->qcc, H3_NO_ERROR, 0); +} +static void h3_release(void *ctx) +{ + struct h3c *h3c = ctx; pool_free(pool_head_h3c, h3c); } @@ -1266,6 +1271,7 @@ const struct qcc_app_ops h3_ops = { .snd_buf = h3_snd_buf, .detach = h3_detach, .finalize = h3_finalize, - .release = h3_release, + .shutdown = h3_shutdown, .inc_err_cnt = h3_stats_inc_err_cnt, + .release = h3_release, }; diff --git a/src/mux_quic.c b/src/mux_quic.c index bf91750c5..08db5387e 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -1796,11 +1796,11 @@ static void qc_release(struct qcc *qcc) TRACE_ENTER(QMUX_EV_QCC_END, conn); - if (qcc->app_ops && qcc->app_ops->release) { + if (qcc->app_ops && qcc->app_ops->shutdown) { /* Application protocol with dedicated connection closing * procedure. */ - qcc->app_ops->release(qcc->ctx); + qcc->app_ops->shutdown(qcc->ctx); /* useful if application protocol should emit some closing * frames. For example HTTP/3 GOAWAY frame. @@ -1810,7 +1810,6 @@ static void qc_release(struct qcc *qcc) else { qcc_emit_cc_app(qcc, QC_ERR_NO_ERROR, 0); } - TRACE_PROTO("application layer released", QMUX_EV_QCC_END, conn); if (qcc->task) { task_destroy(qcc->task); @@ -1839,6 +1838,10 @@ static void qc_release(struct qcc *qcc) pool_free(pool_head_quic_frame, frm); } + if (qcc->app_ops && qcc->app_ops->release) + qcc->app_ops->release(qcc->ctx); + TRACE_PROTO("application layer released", QMUX_EV_QCC_END, conn); + pool_free(pool_head_qcc, qcc); if (conn) {