From 9357873641c5de29b169848fc1c808747818a1eb Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Tue, 25 Jun 2024 08:22:58 +0200 Subject: [PATCH] BUG/MEDIUM: stick-table: Decrement the ref count inside lock to kill a session When we try to kill a session, the shard must be locked before decrementing the ref count on the session. Otherwise, the ref count can fall to 0 and a purge task (stktable_trash_oldest or process_table_expire) may release the session before we have the opportunity to acquire the lock on the shard to effectively kill the session. This could lead to a double free. Here is the scenario: Thread 1 Thread 2 sktsess_kill(ts) if (ATOMIC_DEC(&ts->ref_cnt) != 0) return /* here the ref count is 0 */ stktable_trash_oldest() LOCK(&sh_lock) if (!ATOMIC_LOAD(&ts->ref_cnf)) __stksess_free(ts) UNLOCK(&sh_lock) /* here the session was released */ LOCK(&sh_lock) __stksess_free(ts) <--- double free UNLOCK(&sh_lock) The bug was introduced in 2.9 by the commit 7968fe3889 ("MEDIUM: stick-table: change the ref_cnt atomically"). The ref count must be decremented inside the lock for stksess_kill() and sktsess_kill_if_expired() function. This patch should fix the issue #2611. It must be backported as far as 2.9. On the 2.9, there is no sharding. All the table is locked. The patch will have to be adapted. --- include/haproxy/stick_table.h | 8 ++++---- src/stick_table.c | 8 +++----- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/include/haproxy/stick_table.h b/include/haproxy/stick_table.h index 2c5e7a223..3e10db107 100644 --- a/include/haproxy/stick_table.h +++ b/include/haproxy/stick_table.h @@ -220,9 +220,6 @@ static inline void stksess_kill_if_expired(struct stktable *t, struct stksess *t uint shard; size_t len; - if (decrefcnt && HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1) != 0) - return; - if (t->expire != TICK_ETERNITY && tick_is_expired(ts->expire, now_ms)) { if (t->type == SMP_T_STR) len = strlen((const char *)ts->key.key); @@ -235,9 +232,12 @@ static inline void stksess_kill_if_expired(struct stktable *t, struct stksess *t ALREADY_CHECKED(shard); HA_RWLOCK_WRLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); - __stksess_kill_if_expired(t, ts); + if (!decrefcnt || !HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1)) + __stksess_kill_if_expired(t, ts); HA_RWLOCK_WRUNLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); } + else if (decrefcnt) + HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1); } /* sets the stick counter's entry pointer */ diff --git a/src/stick_table.c b/src/stick_table.c index ff19709d6..2b5eddd7b 100644 --- a/src/stick_table.c +++ b/src/stick_table.c @@ -170,10 +170,7 @@ int stksess_kill(struct stktable *t, struct stksess *ts, int decrefcnt) { uint shard; size_t len; - int ret; - - if (decrefcnt && HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1) != 0) - return 0; + int ret = 0; if (t->type == SMP_T_STR) len = strlen((const char *)ts->key.key); @@ -186,7 +183,8 @@ int stksess_kill(struct stktable *t, struct stksess *ts, int decrefcnt) ALREADY_CHECKED(shard); HA_RWLOCK_WRLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); - ret = __stksess_kill(t, ts); + if (!decrefcnt || !HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1)) + ret = __stksess_kill(t, ts); HA_RWLOCK_WRUNLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); return ret;