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

When a connection is removed from the safe list or the idle list,
CO_FL_SAFE_LIST and CO_FL_IDLE_LIST flags must be cleared. It is performed
when the connection is reused. But not when it is moved into the
toremove_conns list. It may be an issue because the multiplexer owning the
connection may be woken up before the connection is really removed. If the
connection flags are not sanitized, it may think the connection is idle and
reinsert it in the corresponding list. From this point, we can imagine
several bugs. An UAF or a connection reused with an invalid state for
instance.

To avoid any issue, the connection flags are sanitized when an idle
connection is moved into the toremove_conns list. The same is performed at
right places in the multiplexers. Especially because the connection release
may be delayed (for h2 and fcgi connections).

This patch shoudld fix the issue #2057. It must carefully be backported as
far as 2.2. Especially on the 2.2 where the code is really different. But
some conflicts should be expected on the 2.4 too.

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index 9b48271740d96f3945cbb33e7a7b91cb18520e8d..fd74b56af78afde8b818438306a1efb5d30ef3c9 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3140,8 +3140,10 @@ 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 f9bd1d88b9a180fb67e400c427c62f375bf76ea4..455f62384a24f2f9166a7f013eae1bad3e3de4ed 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -3297,8 +3297,10 @@ 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 4ac7fc5da2cab07a8c35b26fe8507e177a00438a..293c41447d57a791416a49a53d0ad7cd026b16a7 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4031,6 +4031,7 @@ 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);
                }
        }
@@ -4039,6 +4040,7 @@ 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);
                }
        }
@@ -4118,8 +4120,10 @@ 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);
        }
@@ -4172,6 +4176,7 @@ 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 6e9d5113fed0ae407a1e47e7549af11c6e42d9e5..45fc33fa308588cb412202aa86756864a5e90d09 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -5828,6 +5828,7 @@ 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++;
 
@@ -5885,6 +5886,7 @@ void srv_release_conn(struct server *srv, struct connection *conn)
        /* Remove the connection from any tree (safe, idle or available) */
        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);
 }