BUG/MEDIUM: servers: Fix a race condition with idle connections.

When we're purging idle connections, there's a race condition, when we're
removing the connection from the idle list, to add it to the list of
connections to free, if the thread owning the connection tries to free it
at the same time.
To fix this, simply add a per-thread lock, that has to be hold before
removing the connection from the idle list, and when, in conn_free(), we're
about to remove the connection from every list. That way, we know for sure
the connection will stay valid while we remove it from the idle list, to add
it to the list of connections to free.
This should happen rarely enough that it shouldn't have any impact on
performances.
This has not been reported yet, but could provoke random segfaults.

This should be backported to 2.0.

diff --git a/include/proto/connection.h b/include/proto/connection.h
index 88c7e50e52fa37d3edd4e18412e1a219c8df1f43..02f3234a8b99ecd00315285e51c1e431f4519871 100644 (file)
--- a/include/proto/connection.h
+++ b/include/proto/connection.h
@@ -64,6 +64,8 @@ int conn_sock_drain(struct connection *conn);
 int conn_send_socks4_proxy_request(struct connection *conn);
 int conn_recv_socks4_proxy_response(struct connection *conn);
 
+__decl_hathreads(extern HA_SPINLOCK_T toremove_lock[MAX_THREADS]);
+
 /* returns true is the transport layer is ready */
 static inline int conn_xprt_ready(const struct connection *conn)
 {
@@ -595,7 +597,9 @@ static inline void conn_free(struct connection *conn)
        }
 
        conn_force_unsubscribe(conn);
+       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
        LIST_DEL_LOCKED(&conn->list);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
        pool_free(pool_head_connection, conn);
 }
 

diff --git a/src/server.c b/src/server.c
index 02fa2a46cb91387c0a495f4b4a8864b3ed004be7..a815f40010b3c05e193bef2360c15d00887a6ad2 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -66,6 +66,7 @@ struct eb_root idle_conn_srv = EB_ROOT;
 struct task *idle_conn_task = NULL;
 struct task *idle_conn_cleanup[MAX_THREADS] = { NULL };
 struct list toremove_connections[MAX_THREADS];
+__decl_hathreads(HA_SPINLOCK_T toremove_lock[MAX_THREADS]);
 
 /* The server names dictionary */
 struct dict server_name_dict = {
@@ -5660,6 +5661,7 @@ struct task *srv_cleanup_idle_connections(struct task *task, void *context, unsi
                        int j;
                        int did_remove = 0;
 
+                       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[i]);
                        for (j = 0; j < max_conn; j++) {
                                struct connection *conn = LIST_POP_LOCKED(&srv->idle_orphan_conns[i], struct connection *, list);
                                if (!conn)
@@ -5667,6 +5669,7 @@ struct task *srv_cleanup_idle_connections(struct task *task, void *context, unsi
                                did_remove = 1;
                                LIST_ADDQ_LOCKED(&toremove_connections[i], &conn->list);
                        }
+                       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[i]);
                        if (did_remove && max_conn < srv->curr_idle_thr[i])
                                srv_is_empty = 0;
                        if (did_remove)