From 48ce6a3ab181bf7b90376008d7fb077df57387c0 Mon Sep 17 00:00:00 2001 From: Olivier Houchard Date: Thu, 2 Jul 2020 11:58:05 +0200 Subject: [PATCH] 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. --- src/mux_fcgi.c | 21 +++++++++++++-------- src/mux_h1.c | 5 ++--- src/mux_h2.c | 24 ++++++++++++++++-------- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c index 1cf8a33b41..c4c7ce6421 100644 --- 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 a294c6542c..89c55b4d46 100644 --- 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 adcc6c270d..827ff8e4f6 100644 --- 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) { -- 2.39.5