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.

diff --git a/include/haproxy/stick_table.h b/include/haproxy/stick_table.h
index 2c5e7a223eb34d901c81e0b5e7f2352ef02464de..3e10db107087a6b40ae352a3c218b9b409a791a4 100644 (file)
--- 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 ff19709d631e2064717abfcbb9f3b89053fffdb0..2b5eddd7b65dbf5a1192b1983be3ec63a656589b 100644 (file)
--- 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;