BUG/MEDIUM: muxes: Make sure nobody stole the connection before using it.

In the various timeout functions, make sure nobody stole the connection from
us before attempting to doing anything with it, there's a very small race
condition between the time we access the task context, and the time we
actually check it again with the lock, where it could have been free'd.

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index 1cf8a33b4137f8131fbd2aee6a5fd604602812b8..c4c7ce6421ceb517085125ab7d864c34d5063b57 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3085,7 +3085,19 @@ static struct task *fcgi_timeout_task(struct task *t, void *context, unsigned sh
        TRACE_ENTER(FCGI_EV_FCONN_WAKE, (fconn ? fconn->conn : NULL));
 
        if (fconn) {
+               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
+
+               /* Somebody already stole the connection from us, so we should not
+                * free it, we just have to free the task.
+                */
+               if (!t->context) {
+                       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
+                       fconn = NULL;
+                       goto do_leave;
+               }
+
                if (!expired) {
+                       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
                        TRACE_DEVEL("leaving (not expired)", FCGI_EV_FCONN_WAKE, fconn->conn);
                        return t;
                }
@@ -3093,20 +3105,13 @@ static struct task *fcgi_timeout_task(struct task *t, void *context, unsigned sh
                /* We're about to destroy the connection, so make sure nobody attempts
                 * to steal it from us.
                 */
-               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
-
                if (fconn->conn->flags & CO_FL_LIST_MASK)
                        MT_LIST_DEL(&fconn->conn->list);
 
-               /* Somebody already stole the connection from us, so we should not
-                * free it, we just have to free the task.
-                */
-               if (!t->context)
-                       fconn = NULL;
-
                HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
        }
 
+do_leave:
        task_destroy(t);
 
        if (!fconn) {

diff --git a/src/mux_h1.c b/src/mux_h1.c
index a294c6542cb4ebc986c372a0aec9af825d527993..89c55b4d46aabc305911713830c0618e7dfc6756 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -2311,14 +2311,13 @@ static struct task *h1_timeout_task(struct task *t, void *context, unsigned shor
                 */
                HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
 
-               if (h1c->conn->flags & CO_FL_LIST_MASK)
-                       MT_LIST_DEL(&h1c->conn->list);
-
                /* Somebody already stole the connection from us, so we should not
                 * free it, we just have to free the task.
                 */
                if (!t->context)
                        h1c = NULL;
+               else if (h1c->conn->flags & CO_FL_LIST_MASK)
+                       MT_LIST_DEL(&h1c->conn->list);
 
                HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
        }

diff --git a/src/mux_h2.c b/src/mux_h2.c
index adcc6c270dd7ee5c2632cdfee9dfa833cb917d11..827ff8e4f6ba99817d052176ab74ced617534c10 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3706,7 +3706,21 @@ static struct task *h2_timeout_task(struct task *t, void *context, unsigned shor
        TRACE_ENTER(H2_EV_H2C_WAKE, h2c ? h2c->conn : NULL);
 
        if (h2c) {
+                /* Make sure nobody stole the connection from us */
+               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
+
+               /* Somebody already stole the connection from us, so we should not
+                * free it, we just have to free the task.
+                */
+               if (!t->context) {
+                       h2c = NULL;
+                       HA_SPIN_UNLOCK(&OTHER_LOCK, &idle_conns[tid].takeover_lock);
+                       goto do_leave;
+               }
+
+
                if (!expired) {
+                       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
                        TRACE_DEVEL("leaving (not expired)", H2_EV_H2C_WAKE, h2c->conn);
                        return t;
                }
@@ -3715,6 +3729,7 @@ static struct task *h2_timeout_task(struct task *t, void *context, unsigned shor
                        /* we do still have streams but all of them are idle, waiting
                         * for the data layer, so we must not enforce the timeout here.
                         */
+                       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
                        t->expire = TICK_ETERNITY;
                        return t;
                }
@@ -3722,20 +3737,13 @@ static struct task *h2_timeout_task(struct task *t, void *context, unsigned shor
                /* We're about to destroy the connection, so make sure nobody attempts
                 * to steal it from us.
                 */
-               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
-
                if (h2c->conn->flags & CO_FL_LIST_MASK)
                        MT_LIST_DEL(&h2c->conn->list);
 
-               /* Somebody already stole the connection from us, so we should not
-                * free it, we just have to free the task.
-                */
-               if (!t->context)
-                       h2c = NULL;
-
                HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
        }
 
+do_leave:
        task_destroy(t);
 
        if (!h2c) {