Changeset 205694 in webkit

Timestamp:

Sep 8, 2016 10:11:25 PM (8 years ago)

Author:

Yusuke Suzuki

Message:

[WTF] HashTable's rehash is not compatible to Ref<T> and ASan
​https://bugs.webkit.org/show_bug.cgi?id=161763

Reviewed by Mark Lam.

Source/WebCore:

Include wtf/text/StringHash.h to avoid linking errors in EFL port.

• loader/ResourceLoadStatistics.h:

Source/WTF:

If we move an object, the location which the moved object used should not be touched anymore.
HashTable::rehash performs WTFMove for the object that resides in the old table.
However, after moving it, we accidentally touch this location by using !isEmptyOrDeletedBucket(table[i])
in HashTable::deallocateTable. And it causes ASan crashing if we use Ref<> for HashTable's key or value.

In this patch, we call the destructor right after moving the object. And HashTable::rehash just calls
fastFree since all the objects in the old table are already moved and destructed.
And we also change HashTable::deallocate to destruct only live objects. Calling destructors for empty objects
is meaningless. And according to the Ref<>'s comment, empty object is not designed to be destructed.

• wtf/HashTable.h:

(WTF::KeyTraits>::deallocateTable):

Tools:

Add tests that inserts many Ref<>s. It incurs HashTable::rehash, and we can ensure
that ASan crash does not occur with this patch.

• TestWebKitAPI/Tests/WTF/HashMap.cpp:

(TestWebKitAPI::TEST):

• TestWebKitAPI/Tests/WTF/HashSet.cpp:

(TestWebKitAPI::TEST):

Location:

trunk

Files:

• Source/WTF/ChangeLog (modified) (1 diff)
• Source/WTF/wtf/HashTable.h (modified) (3 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/ResourceLoadStatistics.h (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp (modified) (2 diffs)
• Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp (modified) (1 diff)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-09-08  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [WTF] HashTable's rehash is not compatible to Ref<T> and ASan
+        https://bugs.webkit.org/show_bug.cgi?id=161763
+
+        Reviewed by Mark Lam.
+
+        If we move an object, the location which the moved object used should not be touched anymore.
+        HashTable::rehash performs WTFMove for the object that resides in the old table.
+        However, after moving it, we accidentally touch this location by using `!isEmptyOrDeletedBucket(table[i])`
+        in HashTable::deallocateTable. And it causes ASan crashing if we use Ref<> for HashTable's key or value.
+
+        In this patch, we call the destructor right after moving the object. And HashTable::rehash just calls
+        fastFree since all the objects in the old table are already moved and destructed.
+        And we also change HashTable::deallocate to destruct only live objects. Calling destructors for empty objects
+        is meaningless. And according to the Ref<>'s comment, empty object is not designed to be destructed.
+
+        * wtf/HashTable.h:
+        (WTF::KeyTraits>::deallocateTable):
+
 2016-09-08  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/WTF/wtf/HashTable.h

@@ -1154 +1154 @@
     {
         for (unsigned i = 0; i < size; ++i) {
-            if (!isDeletedBucket(table[i]))
+            if (!isEmptyOrDeletedBucket(table[i]))
                 table[i].~ValueType();
         }
@@ -1204 +1204 @@
 
             Value* reinsertedEntry = reinsert(WTFMove(oldTable[i]));
+            oldTable[i].~ValueType();
             if (std::addressof(oldTable[i]) == entry) {
                 ASSERT(!newEntry);
@@ -1212 +1213 @@
         m_deletedCount = 0;
 
-        deallocateTable(oldTable, oldTableSize);
+        fastFree(oldTable);
 
         internalCheckTableConsistency();

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-09-08  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [WTF] HashTable's rehash is not compatible to Ref<T> and ASan
+        https://bugs.webkit.org/show_bug.cgi?id=161763
+
+        Reviewed by Mark Lam.
+
+        Include wtf/text/StringHash.h to avoid linking errors in EFL port.
+
+        * loader/ResourceLoadStatistics.h:
+
 2016-09-08  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/loader/ResourceLoadStatistics.h

@@ -28 +28 @@
 
 #include <wtf/HashCountedSet.h>
+#include <wtf/text/StringHash.h>
 #include <wtf/text/WTFString.h>
 

trunk/Tools/ChangeLog

@@ +1 @@
+2016-09-08  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [WTF] HashTable's rehash is not compatible to Ref<T> and ASan
+        https://bugs.webkit.org/show_bug.cgi?id=161763
+
+        Reviewed by Mark Lam.
+
+        Add tests that inserts many Ref<>s. It incurs HashTable::rehash, and we can ensure
+        that ASan crash does not occur with this patch.
+
+        * TestWebKitAPI/Tests/WTF/HashMap.cpp:
+        (TestWebKitAPI::TEST):
+        * TestWebKitAPI/Tests/WTF/HashSet.cpp:
+        (TestWebKitAPI::TEST):
+
 2016-09-08  Alex Christensen  <achristensen@webkit.org>
 

trunk/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp

@@ -790 +790 @@
 
     ASSERT_STREQ("ref(a) deref(a) ", takeLogStr().c_str());
+
+    {
+        HashMap<Ref<RefLogger>, int> map;
+        for (int i = 0; i < 64; ++i) {
+            Ref<RefLogger> ref = adoptRef(*new RefLogger("a"));
+            auto* pointer = ref.ptr();
+            map.add(WTFMove(ref), i + 1);
+            ASSERT_TRUE(map.contains(pointer));
+        }
+    }
+
+    ASSERT_STREQ("deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) ", takeLogStr().c_str());
 }
 
@@ -891 +903 @@
 
     ASSERT_STREQ("ref(a) deref(a) ", takeLogStr().c_str());
+
+    {
+        HashMap<int, Ref<RefLogger>> map;
+        for (int i = 0; i < 64; ++i) {
+            Ref<RefLogger> ref = adoptRef(*new RefLogger("a"));
+            map.add(i + 1, WTFMove(ref));
+            ASSERT_TRUE(map.contains(i + 1));
+        }
+    }
+
+    ASSERT_STREQ("deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) ", takeLogStr().c_str());
 }
 

trunk/Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp

@@ -429 +429 @@
 
     ASSERT_STREQ("ref(a) deref(a) ", takeLogStr().c_str());
+
+    {
+        HashSet<Ref<RefLogger>> set;
+        for (int i = 0; i < 64; ++i) {
+            Ref<RefLogger> ref = adoptRef(*new RefLogger("a"));
+            auto* pointer = ref.ptr();
+            set.add(WTFMove(ref));
+            ASSERT_TRUE(set.contains(pointer));
+        }
+    }
+    ASSERT_STREQ("deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) deref(a) ", takeLogStr().c_str());
 }