BUG/MINOR: dict: fix refcount race on insert collision

In dict_insert(), when ebis_insert() returns an existing node n indicating
that another thread inserted the same key concurrently, the code freed its
own newly-allocated entry and returned the winner without bumping its
refcount. Both callers then held a reference with refcount=1 instead of 2,
so when one expires the other becomes a use-after-free or double-free.

The bug likely comes from the fact that new_dict_entry() creates an entry
with a refcount preset to 1 (saves an atomic op) and that because of this
there is no refcount increment upon a successful insertion in the tree,
resulting in requiring different code paths for collision and normal
insertion.

A simple fix consists in bumping the refcount under the lock and unlocking
only at the end, but this would mean performing two free() calls under a
lock, which we always try to avoid. The code was slightly rearranged so
that we can now bump the existing entry's refcount under the lock in case
of duplicate, or unlock immediately in the common case, so that the free()
call is done out of the lock.

The probably of the race is very low (at peers connection setup only),
reason why it's marked low. This should be backported to all versions.

diff --git a/src/dict.c b/src/dict.c
index 34689ef77401c5d2c5937b71f12ad066a270bb4f..fc6bdad22117649c116548282e45798cf1b539ed 100644 (file)
--- a/src/dict.c
+++ b/src/dict.c
@@ -79,7 +79,7 @@ static struct dict_entry *__dict_lookup(struct dict *d, const char *s)
  */
 struct dict_entry *dict_insert(struct dict *d, char *s)
 {
-       struct dict_entry *de;
+       struct dict_entry *de, *tree_de;
        struct ebpt_node *n;
 
        HA_RWLOCK_RDLOCK(DICT_LOCK, &d->rwlock);
@@ -97,13 +97,18 @@ struct dict_entry *dict_insert(struct dict *d, char *s)
 
        HA_RWLOCK_WRLOCK(DICT_LOCK, &d->rwlock);
        n = ebis_insert(&d->values, &de->value);
-       HA_RWLOCK_WRUNLOCK(DICT_LOCK, &d->rwlock);
-       if (n != &de->value) {
+       tree_de = container_of(n, struct dict_entry, value);
+       if (tree_de == de)
+               HA_RWLOCK_WRUNLOCK(DICT_LOCK, &d->rwlock);
+       else {
+               /* another entry was already there, we'll return it, kill
+                * ours and bump the other's refcount before returning it.
+                */
+               HA_ATOMIC_INC(&tree_de->refcount);
+               HA_RWLOCK_WRUNLOCK(DICT_LOCK, &d->rwlock);
                free_dict_entry(de);
-               de = container_of(n, struct dict_entry, value);
        }
-
-       return de;
+       return tree_de;
 }