[Oilpan] Add use-after-free detector in Member<>

third_party/WebKit/Source/platform/heap/Handle.h

Index: third_party/WebKit/Source/platform/heap/Handle.h
diff --git a/third_party/WebKit/Source/platform/heap/Handle.h b/third_party/WebKit/Source/platform/heap/Handle.h
index 1c723468434d49f130be1f9c04b6e6c655227fa8..e2f3424a30ebea14f25c2ed07d7dbd5ea6f57922 100644
--- a/third_party/WebKit/Source/platform/heap/Handle.h
+++ b/third_party/WebKit/Source/platform/heap/Handle.h
@@ -774,11 +774,30 @@ public:
     T* get() const
     {
         static_assert(IsFullyDefined<T>::value, "T is not fully defined.");
+#if ENABLE(ASSERT) && defined(ADDRESS_SANITIZER)
+        // Verify that this handle points to a live on-heap object, if a pointer
+        // has been assigned to this handle out of GarbageCollectedMixin constructions.
+        if (m_raw && m_gcGeneration != gcGenerationUnchecked && !ThreadState::current()->isConstructingGCMixin()) {
+            HeapObjectHeader* header = HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw);
+            ASSERT(m_gcGeneration == header->gcGeneration());
+        }
+#endif
         return m_raw;
     }
 
-    void clear() { m_raw = nullptr; }
+    // This method is an accessor without any security checks, and it is
+    // expected to be used under some limited condition.
+    // So do NOT use it, if you do not know what it means.
+    // TODO(peria): We should remove this method.
+    T* unsafeGet() const
+    {
+        return m_raw;
+    }
 
+    void clear()
+    {
+        m_raw = nullptr;
+    }
 
 protected:
     void checkPointer()

[haraken, 2015/11/20 01:59:06]

In a follow-up CL, can you rename this to setPointer(void* raw)? I think it's
better to redirect all pointer assignments to the setPointer, instead of calling
checkPointer() at every assignment.

[peria, 2015/11/20 02:27:00]

Acknowledged.

@@ -795,26 +814,31 @@ protected:
         if (reinterpret_cast<intptr_t>(m_raw) % allocationGranularity)
             return;
 
-        // TODO(haraken): What we really want to check here is that the pointer
-        // is a traceable object. In other words, the pointer is either of:
-        //
-        //   (a) a pointer to the head of an on-heap object.
-        //   (b) a pointer to the head of an on-heap mixin object.
-        //
-        // We can check it by calling Heap::isHeapObjectAlive(m_raw),
-        // but we cannot call it here because it requires to include T.h.
-        // So we currently only try to implement the check for (a), but do
-        // not insist that T's definition is in scope.
-        if (IsFullyDefined<T>::value && !IsGarbageCollectedMixin<T>::value)
-            ASSERT(HeapObjectHeader::fromPayload(m_raw)->checkHeader());
+        // m_gcGeneration verifies use-after-free of this Member handle.
+        // If m_gcGeneration is gcGenerationForFreeListEntry or m_raw is
+        // nullptr, the verification is skipped.

[haraken, 2015/11/20 01:59:06]

gcGenerationForFreeListEntry => gcGenerationUnchecked

[peria, 2015/11/20 02:27:00]

Done.

+        // For technical reason, we set gcGenerationForFreeListEntry in
+        // m_gcGeneration in case that this method is called in a constructor
+        // of a GCMixin object.

[haraken, 2015/11/20 01:59:06]

We set gcGenerationUnchecked while constructing a GC mixin object (and skip the
verification unfortunately) because HeapObjectHeaderTrait<T>::heapObjectHeader
doesn't work before the vtable of the object is complete.

[peria, 2015/11/20 04:14:50]

Done.

+        // TODO(peria): Set m_gcGeneration even if this is called in a
+        // GCMixin constructor.
+        m_gcGeneration = gcGenerationUnchecked;
+        if (IsFullyDefined<T>::value && !ThreadState::current()->isConstructingGCMixin()) {

[haraken, 2015/11/20 01:59:06]

Remove the IsFullyDefined<T>::value check. As we discussed, it is wrong to use
IsFullyDefined<T>::value dynamically.

[peria, 2015/11/20 02:27:00]

Done.
And it is checked statically at the beginning of this method.

+            HeapObjectHeader* header = HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw);
+            m_gcGeneration = header->gcGeneration();
+            ASSERT(m_gcGeneration != gcGenerationForFreeListEntry);
+        }
 #endif
     }
 
     T* m_raw;
 
+#if ENABLE(ASSERT) && defined(ADDRESS_SANITIZER)
+    uint32_t m_gcGeneration;
+#endif
+
     template<bool x, WTF::WeakHandlingFlag y, WTF::ShouldWeakPointersBeMarkedStrongly z, typename U, typename V> friend struct CollectionBackingTraceTrait;
     friend class Visitor;
-
 };
 
 // WeakMember is similar to Member in that it is used to point to other oilpan
@@ -878,6 +902,14 @@ public:
         return *this;
     }
 
+    // TODO(peria): Remove this get() and use unsafeGet() at only
+    // where it is required.
+    T* get() const
+    {
+        // WeakMember may point to a dead object, so we skip the verification.
+        return Member<T>::unsafeGet();
+    }
+
 private:
     T** cell() const { return const_cast<T**>(&this->m_raw); }
 
@@ -952,15 +984,15 @@ public:
 };
 
 // Comparison operators between (Weak)Members, Persistents, and UntracedMembers.
-template<typename T, typename U> inline bool operator==(const Member<T>& a, const Member<U>& b) { return a.get() == b.get(); }
-template<typename T, typename U> inline bool operator!=(const Member<T>& a, const Member<U>& b) { return a.get() != b.get(); }
+template<typename T, typename U> inline bool operator==(const Member<T>& a, const Member<U>& b) { return a.unsafeGet() == b.unsafeGet(); }
+template<typename T, typename U> inline bool operator!=(const Member<T>& a, const Member<U>& b) { return a.unsafeGet() != b.unsafeGet(); }
 template<typename T, typename U> inline bool operator==(const Persistent<T>& a, const Persistent<U>& b) { return a.get() == b.get(); }
 template<typename T, typename U> inline bool operator!=(const Persistent<T>& a, const Persistent<U>& b) { return a.get() != b.get(); }
 
-template<typename T, typename U> inline bool operator==(const Member<T>& a, const Persistent<U>& b) { return a.get() == b.get(); }
-template<typename T, typename U> inline bool operator!=(const Member<T>& a, const Persistent<U>& b) { return a.get() != b.get(); }
-template<typename T, typename U> inline bool operator==(const Persistent<T>& a, const Member<U>& b) { return a.get() == b.get(); }
-template<typename T, typename U> inline bool operator!=(const Persistent<T>& a, const Member<U>& b) { return a.get() != b.get(); }
+template<typename T, typename U> inline bool operator==(const Member<T>& a, const Persistent<U>& b) { return a.unsafeGet() == b.get(); }
+template<typename T, typename U> inline bool operator!=(const Member<T>& a, const Persistent<U>& b) { return a.unsafeGet() != b.get(); }
+template<typename T, typename U> inline bool operator==(const Persistent<T>& a, const Member<U>& b) { return a.get() == b.unsafeGet(); }
+template<typename T, typename U> inline bool operator!=(const Persistent<T>& a, const Member<U>& b) { return a.get() != b.unsafeGet(); }
 
 template<typename T>
 class DummyBase {
@@ -1420,11 +1452,9 @@ template<typename T> struct PtrHash<blink::Member<T>> : PtrHash<T*> {
     static bool equal(const U& a, const V& b) { return a == b; }
 };
 
-template<typename T> struct PtrHash<blink::WeakMember<T>> : PtrHash<blink::Member<T>> {
-};
+template<typename T> struct PtrHash<blink::WeakMember<T>> : PtrHash<blink::Member<T>> { };
 
-template<typename T> struct PtrHash<blink::UntracedMember<T>> : PtrHash<blink::Member<T>> {
-};
+template<typename T> struct PtrHash<blink::UntracedMember<T>> : PtrHash<blink::Member<T>> { };
 
 // PtrHash is the default hash for hash tables with members.
 template<typename T> struct DefaultHash<blink::Member<T>> {