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 1cf8a33..c4c7ce6 100644
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3085,7 +3085,19 @@
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 @@
/* 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 a294c65..89c55b4 100644
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -2311,14 +2311,13 @@
*/
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 adcc6c2..827ff8e 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3706,7 +3706,21 @@
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 @@
/* 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 @@
/* 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) {