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.

diff --git a/include/haproxy/mux_quic-t.h b/include/haproxy/mux_quic-t.h
index abf00bfc30d5248a24993a6af615fc91e713a95b..1670262a1b3d4228c6ca59ede646e423845b4e50 100644 (file)
--- 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 6c6764851420d70407d9c5e8890359cc06c69b80..8d13f5d09c0496c3e3eb3333896cc3470c954bc7 100644 (file)
--- 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 bf91750c5eefe6c6b8568abfe1e75035313383a1..08db5387ee1ca178079b59ad29a3002e9be18c55 100644 (file)
--- 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) {