RepoMirrors/haproxy
1
0
Fork 0
mirror of http://git.haproxy.org/git/haproxy.git/ synced 2024-12-15 07:54:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Olivier Houchard 2020-07-02 11:58:05 +02:00 committed by Willy Tarreau
parent 54d31170a9
commit 48ce6a3ab1
3 changed files with 31 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
src/mux_fcgi.c
View File

@ -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)); TRACE_ENTER(FCGI_EV_FCONN_WAKE, (fconn ? fconn->conn : NULL));
if (fconn) { 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) { if (!expired) {
HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
TRACE_DEVEL("leaving (not expired)", FCGI_EV_FCONN_WAKE, fconn->conn); TRACE_DEVEL("leaving (not expired)", FCGI_EV_FCONN_WAKE, fconn->conn);
return t; 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 /* We're about to destroy the connection, so make sure nobody attempts
* to steal it from us. * to steal it from us.
*/ */
HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
if (fconn->conn->flags & CO_FL_LIST_MASK) if (fconn->conn->flags & CO_FL_LIST_MASK)
MT_LIST_DEL(&fconn->conn->list); 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); HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
} }
do_leave:
task_destroy(t); task_destroy(t);
if (!fconn) { if (!fconn) {

5
src/mux_h1.c
View File

@ -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); 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 /* Somebody already stole the connection from us, so we should not
* free it, we just have to free the task. * free it, we just have to free the task.
*/ */
if (!t->context) if (!t->context)
h1c = NULL; 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); HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
} }

24
src/mux_h2.c
View File

@ -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); TRACE_ENTER(H2_EV_H2C_WAKE, h2c ? h2c->conn : NULL);
if (h2c) { 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) { if (!expired) {
HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
TRACE_DEVEL("leaving (not expired)", H2_EV_H2C_WAKE, h2c->conn); TRACE_DEVEL("leaving (not expired)", H2_EV_H2C_WAKE, h2c->conn);
return t; 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 /* we do still have streams but all of them are idle, waiting
* for the data layer, so we must not enforce the timeout here. * 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; t->expire = TICK_ETERNITY;
return t; 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 /* We're about to destroy the connection, so make sure nobody attempts
* to steal it from us. * to steal it from us.
*/ */
HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
if (h2c->conn->flags & CO_FL_LIST_MASK) if (h2c->conn->flags & CO_FL_LIST_MASK)
MT_LIST_DEL(&h2c->conn->list); 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); HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].takeover_lock);
} }
do_leave:
task_destroy(t); task_destroy(t);
if (!h2c) { if (!h2c) {