BUG/MEDIUM: mux-h1: don't pass an upgraded connection to traces in h1s_destroy()

When an HTTP/1 frontend connection receives the HTTP/2 preface in two
parts, the first one (shorter than a full request) creates an h1s, then
the second one completes the preface and the connection is upgraded to
H2: h1_release() calls conn_upgrade_mux_fe(), h2_init() sets conn->ctx to
the new h2c, then h1_destroy() calls h1s_destroy(), which emits traces
with h1c->conn. h1_trace() and h1_trace_fill_ctx() then derive the h1c
from conn->ctx, which is now the h2c, and dereference it as an h1c,
crashing the worker. h1_release() already takes care of not using the
connection once it doesn't belong to the mux anymore, but h1s_destroy()
does not.

This only happens when h1 traces are enabled (level "state" or above),
and was reproduced with "-dt h1:state:minimal" and a client sending the
H2 preface in two segments split anywhere several ms apart. The process
then crashes in h1_trace().

The fix consists in only passing the connection to the traces in
h1s_destroy() when it still belongs to this h1c. The upgrade with an
existing h1s was made possible in 2.4 by commit 0f9395d81 ("BUG/MAJOR:
mux-h1: Properly handle TCP to H1 upgrades"), while the trace there comes
from commit 6b81df727 ("MINOR: mux-h1/trace: register a new trace source
with its events") in 2.1. This must be backported to all supported
versions.

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 8421aba7f787c9fa7a1827fbccaf252aa47826bd..6f9ac7eedff67e4eaffb949b5551e112c7b950fb 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -1071,8 +1071,12 @@ static void h1s_destroy(struct h1s *h1s)
 {
        if (h1s) {
                struct h1c *h1c = h1s->h1c;
+               /* after a mux upgrade (e.g. H1 to H2), the connection does not
+                * belong to us anymore and must not be passed to traces.
+                */
+               struct connection *conn = (h1c->conn && h1c->conn->ctx == h1c) ? h1c->conn : NULL;
 
-               TRACE_POINT(H1_EV_H1S_END, h1c->conn, h1s);
+               TRACE_POINT(H1_EV_H1S_END, conn, h1s);
                h1c->h1s = NULL;
 
                if (h1s->subs)
@@ -1101,11 +1105,11 @@ static void h1s_destroy(struct h1s *h1s)
                        h1c->state = H1_CS_IDLE;
                        h1c->flags |= H1C_F_WAIT_NEXT_REQ;
                        h1c->req_count++;
-                       TRACE_STATE("set idle mode on h1c, waiting for the next request", H1_EV_H1C_ERR, h1c->conn, h1s);
+                       TRACE_STATE("set idle mode on h1c, waiting for the next request", H1_EV_H1C_ERR, conn, h1s);
                }
                else {
                        h1_close(h1c);
-                       TRACE_STATE("close h1c", H1_EV_H1S_END, h1c->conn, h1s);
+                       TRACE_STATE("close h1c", H1_EV_H1S_END, conn, h1s);
                }
 
                HA_ATOMIC_DEC(&h1c->px_counters->open_streams);