BUG/MEDIUM: connection: Preserve flags when a conn is removed from an idle list

The commit 5e1b0e7bf ("BUG/MEDIUM: connection: Clear flags when a conn is
removed from an idle list") introduced a regression. CO_FL_SAFE_LIST and
CO_FL_IDLE_LIST flags are used when the connection is released to properly
decrement used/idle connection counters. if a connection is idle, these
flags must be preserved till the connection is really released. It may be
removed from the list but not immediately released. If these flags are lost
when it is finally released, the current number of used connections is
erroneously decremented. If means this counter may become negative and the
counters tracking the number of idle connecitons is not decremented,
suggesting a leak.

So, the above commit is reverted and instead we improve a bit the way to
detect an idle connection. The function conn_get_idle_flag() must now be
used to know if a connection is in an idle list. It returns the connection
flag corresponding to the idle list if the connection is idle
(CO_FL_SAFE_LIST or CO_FL_IDLE_LIST) or 0 otherwise. But if the connection
is scheduled to be removed, 0 is also returned, regardless the connection
flags.

This new function is used when the connection is temporarily removed from
the list to be used, mainly in muxes.

This patch should fix #2078 and #2057. It must be backported as far as 2.2.

diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index 4d289e7b347b5eb17fdc8367aff580094f34d0c5..8cf22ef4f55158c43f7f3124c391805d0d7a198f 100644 (file)
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -316,6 +316,16 @@ 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 97619ec260cebf929810be71de5b55ffe8ade128..29197ad9469977dcdf9f06fdb765fca163219316 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -146,7 +146,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->flags & CO_FL_LIST_MASK;
+               uint conn_in_list = conn_get_idle_flag(conn);
                struct server *srv = objt_server(conn->target);
 
                if (conn_in_list) {

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index fd74b56af78afde8b818438306a1efb5d30ef3c9..0fb4f869d9544091664c6007695e3aa530164233 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -2957,7 +2957,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->flags & CO_FL_LIST_MASK;
+               conn_in_list = conn_get_idle_flag(conn);
                if (conn_in_list)
                        conn_delete_from_tree(&conn->hash_node->node);
 
@@ -3140,10 +3140,8 @@ struct task *fcgi_timeout_task(struct task *t, void *context, unsigned int state
                /* We're about to destroy the connection, so make sure nobody attempts
                 * to steal it from us.
                 */
-               if (fconn->conn->flags & CO_FL_LIST_MASK) {
+               if (fconn->conn->flags & CO_FL_LIST_MASK)
                        conn_delete_from_tree(&fconn->conn->hash_node->node);
-                       fconn->conn->flags &= ~CO_FL_LIST_MASK;
-               }
 
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
        }

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 2390aad3af07408e63bc807fdb8d2de5c0840e6e..5b00b1bd799a56685caa606f2c837d6b6cd5aee1 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -3186,7 +3186,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->flags & CO_FL_LIST_MASK;
+               conn_in_list = conn_get_idle_flag(conn);
                if (conn_in_list)
                        conn_delete_from_tree(&conn->hash_node->node);
 
@@ -3309,10 +3309,8 @@ struct task *h1_timeout_task(struct task *t, void *context, unsigned int state)
                /* We're about to destroy the connection, so make sure nobody attempts
                 * to steal it from us.
                 */
-               if (h1c->conn->flags & CO_FL_LIST_MASK) {
+               if (h1c->conn->flags & CO_FL_LIST_MASK)
                        conn_delete_from_tree(&h1c->conn->hash_node->node);
-                       h1c->conn->flags &= ~CO_FL_LIST_MASK;
-               }
 
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
        }

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 293c41447d57a791416a49a53d0ad7cd026b16a7..4b426394c0880e6ccdf6c112de946ff3a5c2c10f 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3895,11 +3895,10 @@ struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state)
                conn = h2c->conn;
                TRACE_ENTER(H2_EV_H2C_WAKE, conn);
 
-               conn_in_list = conn->flags & CO_FL_LIST_MASK;
-
                /* 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);
                if (conn_in_list)
                        conn_delete_from_tree(&conn->hash_node->node);
 
@@ -4031,7 +4030,6 @@ static int h2_process(struct h2c *h2c)
                if (conn->flags & CO_FL_LIST_MASK) {
                        HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                        conn_delete_from_tree(&conn->hash_node->node);
-                       conn->flags &= ~CO_FL_LIST_MASK;
                        HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                }
        }
@@ -4040,7 +4038,6 @@ static int h2_process(struct h2c *h2c)
                if (conn->flags & CO_FL_LIST_MASK) {
                        HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                        conn_delete_from_tree(&conn->hash_node->node);
-                       conn->flags &= ~CO_FL_LIST_MASK;
                        HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                }
        }
@@ -4120,10 +4117,8 @@ struct task *h2_timeout_task(struct task *t, void *context, unsigned int state)
                /* We're about to destroy the connection, so make sure nobody attempts
                 * to steal it from us.
                 */
-               if (h2c->conn->flags & CO_FL_LIST_MASK) {
+               if (h2c->conn->flags & CO_FL_LIST_MASK)
                        conn_delete_from_tree(&h2c->conn->hash_node->node);
-                       h2c->conn->flags &= ~CO_FL_LIST_MASK;
-               }
 
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
        }
@@ -4176,7 +4171,6 @@ do_leave:
        if (h2c->conn->flags & CO_FL_LIST_MASK) {
                HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                conn_delete_from_tree(&h2c->conn->hash_node->node);
-               h2c->conn->flags &= ~CO_FL_LIST_MASK;
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
        }
 

diff --git a/src/server.c b/src/server.c
index 45fc33fa308588cb412202aa86756864a5e90d09..ade6aafd48af8373ee13ec6b97a228581a600940 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -5828,7 +5828,6 @@ static int srv_migrate_conns_to_remove(struct eb_root *idle_tree, struct mt_list
 
                hash_node = ebmb_entry(node, struct conn_hash_node, node);
                eb_delete(node);
-               hash_node->conn->flags &= ~CO_FL_LIST_MASK;
                MT_LIST_APPEND(toremove_list, &hash_node->conn->toremove_list);
                i++;
 

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