[WTF] HashTable's rehash is not compatible to Ref<T> and ASan
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Sep 2016 05:11:25 +0000 (05:11 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Sep 2016 05:11:25 +0000 (05:11 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@205694 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/HashTable.h
Source/WebCore/ChangeLog
Source/WebCore/loader/ResourceLoadStatistics.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp
Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp

index 8d8508d..84d67ef 100644 (file)
@@ -1,3 +1,23 @@
+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>
 
         Heap::isMarked() shouldn't pay the price of concurrent lazy flipping
index db4c56f..bce7511 100644 (file)
@@ -1153,7 +1153,7 @@ namespace WTF {
     void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::deallocateTable(ValueType* table, unsigned size)
     {
         for (unsigned i = 0; i < size; ++i) {
-            if (!isDeletedBucket(table[i]))
+            if (!isEmptyOrDeletedBucket(table[i]))
                 table[i].~ValueType();
         }
         fastFree(table);
@@ -1203,6 +1203,7 @@ namespace WTF {
             }
 
             Value* reinsertedEntry = reinsert(WTFMove(oldTable[i]));
+            oldTable[i].~ValueType();
             if (std::addressof(oldTable[i]) == entry) {
                 ASSERT(!newEntry);
                 newEntry = reinsertedEntry;
@@ -1211,7 +1212,7 @@ namespace WTF {
 
         m_deletedCount = 0;
 
-        deallocateTable(oldTable, oldTableSize);
+        fastFree(oldTable);
 
         internalCheckTableConsistency();
         return newEntry;
index f372e75..ff70613 100644 (file)
@@ -1,3 +1,14 @@
+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>
 
         HTMLObjectElement.hspace / vspace attributes should be unsigned
index 4d4aa44..290758f 100644 (file)
@@ -27,6 +27,7 @@
 #define ResourceLoadStatistics_h
 
 #include <wtf/HashCountedSet.h>
+#include <wtf/text/StringHash.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
index 9af57d0..14ca6b7 100644 (file)
@@ -1,3 +1,18 @@
+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>
 
         URLParser: Handle \ in paths of special URLs according to spec
index 42a9b78..a109d3f 100644 (file)
@@ -789,6 +789,18 @@ TEST(WTF_HashMap, Ref_Key)
     }
 
     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());
 }
 
 TEST(WTF_HashMap, Ref_Value)
@@ -890,6 +902,17 @@ TEST(WTF_HashMap, Ref_Value)
     }
 
     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());
 }
 
 TEST(WTF_HashMap, DeletedAddressOfOperator)
index 922c16c..3d25057 100644 (file)
@@ -428,6 +428,17 @@ TEST(WTF_HashSet, Ref)
     }
 
     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());
 }
 
 TEST(WTF_HashSet, DeletedAddressOfOperator)