BUG/MAJOR: connection: make sure to always remove a connection from the tree

Since commit 5afcb686b ("MAJOR: connection: purge idle conn by last usage")
in 2.9-dev4, the test on conn->toremove_list added to conn_get_idle_flag()
in 2.8 by commit 3a7b539b1 ("BUG/MEDIUM: connection: Preserve flags when a
conn is removed from an idle list") becomes misleading. Indeed, now both
toremove_list and idle_list are shared by a union since the presence in
these lists is mutually exclusive. However, in conn_get_idle_flag() we
check for the presence in the toremove_list to decide whether or not to
delete the connection from the tree. This test now fails because instead
it sees the presence in the idle or safe list via the union, and concludes
the element must not be removed. Thus the element remains in the tree and
can be found later after the connection is released, causing crashes that
Tristan reported in issue #2292.

The following config is sufficient to reproduce it with 2 threads:

   defaults
        mode http
        timeout client 5s
        timeout server 5s
        timeout connect 1s

   listen front
        bind :8001
        server next 127.0.0.1:8002

   frontend next
        bind :8002
        timeout http-keep-alive 1
        http-request redirect location /

Sending traffic with a few concurrent connections and some short timeouts
suffices to instantly crash it after ~10k reqs:

   $ h2load -t 4 -c 16 -n 10000 -m 1 -w 1 http://0:8001/

With Amaury we analyzed the conditions in which the function is called
in order to figure a better condition for the test and concluded that
->toremove_list is never filled there so we can safely remove that part
from the test and just move the flag retrieval back to what it was prior
to the 2.8 patch above. Note that the patch is not reverted though, as
the parts that would drop the unexpected flags removal are unchanged.

This patch must NOT be backported. The code in 2.8 works correctly, it's
only the change in 2.9 that makes it misbehave.

diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index fb14eed1258b74057f61a2835a85be895ba22c45..98b84b0e0d9a479fbf900ffb9edcd9a20de3ff67 100644 (file)
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -321,16 +321,6 @@ static inline void conn_set_private(struct connection *conn)
        }
 }
 
-/* Used to know if a connection is in an idle list. It returns connection flag
- * corresponding to the idle list if the connection is idle (CO_FL_SAFE_LIST or
- * CO_FL_IDLE_LIST) or 0 otherwise. Note that if the connection is scheduled to
- * be removed, 0 is returned, regardless the connection flags.
- */
-static inline unsigned int conn_get_idle_flag(const struct connection *conn)
-{
-       return (!MT_LIST_INLIST(&conn->toremove_list) ? conn->flags & CO_FL_LIST_MASK : 0);
-}
-
 static inline void conn_force_unsubscribe(struct connection *conn)
 {
        if (!conn->subs)

diff --git a/src/connection.c b/src/connection.c
index 44cfea4c279eebe78fa3fae865d9d84d772122c3..a7ab6f6084f9f58d5d87a827ba2f78032b7d87bd 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -188,7 +188,7 @@ int conn_notify_mux(struct connection *conn, int old_flags, int forced_wake)
             ((conn->flags ^ old_flags) & CO_FL_NOTIFY_DONE) ||
             ((old_flags & CO_FL_WAIT_XPRT) && !(conn->flags & CO_FL_WAIT_XPRT))) &&
            conn->mux && conn->mux->wake) {
-               uint conn_in_list = conn_get_idle_flag(conn);
+               uint conn_in_list = conn->flags & CO_FL_LIST_MASK;
                struct server *srv = objt_server(conn->target);
 
                if (conn_in_list) {

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index 8a4e6c2a8f2044b52205209d11817810d202f408..bc438c659dc96fd941045d73d4deebb485c12c0b 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -2955,7 +2955,7 @@ struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int state)
                conn = fconn->conn;
                TRACE_POINT(FCGI_EV_FCONN_WAKE, conn);
 
-               conn_in_list = conn_get_idle_flag(conn);
+               conn_in_list = conn->flags & CO_FL_LIST_MASK;
                if (conn_in_list)
                        conn_delete_from_tree(conn);
 

diff --git a/src/mux_h1.c b/src/mux_h1.c
index a33a0a625696d76ddeb50859585fd6c4a643a60f..c01223a65b391f34994fc296317453b3e71fc85c 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -3717,7 +3717,7 @@ struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state)
                /* Remove the connection from the list, to be sure nobody attempts
                 * to use it while we handle the I/O events
                 */
-               conn_in_list = conn_get_idle_flag(conn);
+               conn_in_list = conn->flags & CO_FL_LIST_MASK;
                if (conn_in_list)
                        conn_delete_from_tree(conn);
 

diff --git a/src/mux_h2.c b/src/mux_h2.c
index ae7276423ebe71706f6b0543280dd6f77a577f84..956495e94bb66db11a6a79f4c4a72e0ef3ae0ef8 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4099,7 +4099,7 @@ struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state)
                /* Remove the connection from the list, to be sure nobody attempts
                 * to use it while we handle the I/O events
                 */
-               conn_in_list = conn_get_idle_flag(conn);
+               conn_in_list = conn->flags & CO_FL_LIST_MASK;
                if (conn_in_list)
                        conn_delete_from_tree(conn);
 

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 24d11bf1a2e61de632341084016e17cb6a755364..d2b98e2fff3a76ebca0327848665a980578ef117 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -6390,7 +6390,7 @@ struct task *ssl_sock_io_cb(struct task *t, void *context, unsigned int state)
                        return NULL;
                }
                conn = ctx->conn;
-               conn_in_list = conn_get_idle_flag(conn);
+               conn_in_list = conn->flags & CO_FL_LIST_MASK;
                if (conn_in_list)
                        conn_delete_from_tree(conn);
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);