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 7968fe3 ("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.

(cherry picked from commit 9357873)
Signed-off-by: Christopher Faulet <[email protected]>

Author: capflam

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 */

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;