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 abf00bf..1670262 100644
--- a/include/haproxy/mux_quic-t.h
+++ b/include/haproxy/mux_quic-t.h
@@ -188,6 +188,7 @@
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 6c67648..8d13f5d 100644
--- a/src/h3.c
+++ b/src/h3.c
@@ -1203,7 +1203,8 @@
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 @@
* 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 @@
.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 bf91750..08db538 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -1796,11 +1796,11 @@
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 @@
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 @@
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) {