DBusHash: Recalculate bucket used if the table is rebuilt

Hash buckets are simply entries in an array owned by the hash table,
so every time the hash table's array of buckets is reallocated, we must
invalidate all pointers to buckets and recalculate them to point into
the new array of buckets. This was not always done. Luckily, we appear
to have avoided causing any actual memory corruption like this.

The only place where we reallocate the array of buckets is in
rebuild_table(), which is only called by add_allocated_entry(), which
is only called by add_entry(), which is only called by
find_generic_function() when create_if_not_found is true.
find_generic_function(), in turn, is only called by the
table->find_function() implementations.

The table->find_function() implementations have an optional "out"
parameter which returns a pointer to the hash bucket in which the returned
entry would be found. It is set in find_generic_function() for existing
entries, or in add_allocated_entry() if a new entry is created; after
that it is returned through callers unchanged until the caller of
table->find_function() is reached. The only callers that make use of the
"out" parameter in practice are _dbus_hash_iter_lookup(), to populate
a DBusHashIter, and the _dbus_hash_table_remove_TYPE() family, to pass
it to remove_entry().

We can ignore the _dbus_hash_table_remove_TYPE() family for two
reasons: they call the find function with create_if_not_found set to
FALSE, which never reallocates the hash table, and they do not store
the pointer to the bucket in the long-term. So we only need to consider
_dbus_hash_iter_lookup().

It is documented to be unsafe to add hash entries while a DBusHashIter
is open, and only adding a hash entry can trigger rebuild_table();
so we can assume that if _dbus_hash_iter_lookup() returns a valid
bucket, it remains valid forever.

The remaining case that must be considered is whether reallocation
can occur after setting the "out" parameter for the bucket, but before
returning it to _dbus_hash_iter_lookup(). We can see that it can: we
call rebuild_table() after recalculating the correct bucket. If we do,
and it actually causes a rebuild, then we must recalculate the bucket
accordingly.

Looking at the worst-case impact of this bug, if it is going to cause
any problem, it would only be when _dbus_hash_iter_lookup() is called
with create_if_not_found set true. This makes three uses of the bucket:
it stores it in the DBusHashTableIter, it calculates the next bucket
by finding the offset of the bucket in table->buckets and advancing
by one pointer, and it makes an assertion that should be tautologous,
enforcing that the next bucket corresponds to what it should.

When running under the AddressSanitizer, which makes allocations in
widely spaced regions of memory, on a 32-bit platform, we could (and
indeed do) find that the tautologous assertion fails. The current
bucket returned from the "out" parameter is a pointer into the old
value of table->buckets. If it's far enough before or after the new
table->buckets in the address space, then the offset in next_bucket
could overflow a 32-bit integer, resulting in the assertion no longer
being true.

The next commit will add extra assertions, which reproduce the bug
even without AddressSanitizer.

In production code without assertions, the impact is that
the ->bucket and ->next_bucket members of the DBusHashIter can be
invalid. They are used in _dbus_hash_iter_next() and
_dbus_hash_iter_remove_entry(). However, the only callers of
_dbus_hash_iter_lookup() outside test code are in bus/containers.c,
and neither calls either of those functions, so we dodge that bullet.

Signed-off-by: Simon McVittie <smcv@collabora.com>

diff --git a/dbus/dbus-hash.c b/dbus/dbus-hash.c
index 1192c6c49e8b1cba188ae666e4be681c5119f16b..b438a2e02ee6a7697393c6b7f954e88d38bfcb6b 100644 (file)
--- a/dbus/dbus-hash.c
+++ b/dbus/dbus-hash.c
@@ -236,7 +236,7 @@ static DBusHashEntry* find_string_function      (DBusHashTable          *table,
                                                  DBusHashEntry        ***bucket,
                                                  DBusPreallocatedHash   *preallocated);
 static unsigned int   string_hash               (const char             *str);
-static void           rebuild_table             (DBusHashTable          *table);
+static dbus_bool_t    rebuild_table             (DBusHashTable          *table);
 static DBusHashEntry* alloc_entry               (DBusHashTable          *table);
 static void           remove_entry              (DBusHashTable          *table,
                                                  DBusHashEntry         **bucket,
@@ -802,7 +802,33 @@ add_allocated_entry (DBusHashTable   *table,
    */
   if (table->n_entries >= table->hi_rebuild_size ||
       table->n_entries < table->lo_rebuild_size)
-    rebuild_table (table);
+    {
+      if (!rebuild_table (table))
+        return;
+
+      if (bucket)
+        {
+          /* Recalculate hash for the new table size */
+          switch (table->key_type)
+            {
+            case DBUS_HASH_STRING:
+              idx = string_hash (entry->key) & table->mask;
+              break;
+
+            case DBUS_HASH_INT:
+            case DBUS_HASH_UINTPTR:
+              idx = RANDOM_INDEX (table, entry->key);
+              break;
+
+            default:
+              idx = 0;
+              _dbus_assert_not_reached ("Unknown hash table type");
+              break;
+            }
+
+          *bucket = &(table->buckets[idx]);
+        }
+    }
 }
 
 static DBusHashEntry*
@@ -927,7 +953,8 @@ find_direct_function (DBusHashTable        *table,
                                 preallocated);
 }
 
-static void
+/* Return FALSE if nothing happened. */
+static dbus_bool_t
 rebuild_table (DBusHashTable *table)
 {
   int old_size;
@@ -954,13 +981,13 @@ rebuild_table (DBusHashTable *table)
           table->down_shift >= 2)
         new_buckets = table->n_buckets * 4;
       else
-        return; /* can't grow anymore */
+        return FALSE;   /* can't grow any more */
     }
   else
     {
       new_buckets = table->n_buckets / 4;
       if (new_buckets < DBUS_SMALL_HASH_TABLE)
-        return; /* don't bother shrinking this far */
+        return FALSE;   /* don't bother shrinking this far */
     }
 
   table->buckets = dbus_new0 (DBusHashEntry*, new_buckets);
@@ -970,7 +997,7 @@ rebuild_table (DBusHashTable *table)
        * still work, albeit more slowly.
        */
       table->buckets = old_buckets;
-      return;
+      return FALSE;
     }
 
   table->n_buckets = new_buckets;
@@ -1045,6 +1072,8 @@ rebuild_table (DBusHashTable *table)
 
   if (old_buckets != table->static_buckets)
     dbus_free (old_buckets);
+
+  return TRUE;
 }
 
 /**