[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 64e43fd71243011277fb2052e26dfe146148b0dc..fa9c03acb5830cc03d2cc5b7a77def93bde52320 100644
--- a/third_party/WebKit/Source/platform/heap/Handle.h
+++ b/third_party/WebKit/Source/platform/heap/Handle.h
@@ -771,10 +771,30 @@ public:
         checkPointer();
     }
 
-    T* get() const { return m_raw; }
+    T* get() const
+    {
+#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) {

[haraken, 2015/11/16 02:47:49]

This needs to be changed to:

  if (m_raw && m_gcGeneration &&
!ThreadState::current()->isConstructingGCMixin())

[peria, 2015/11/16 05:33:26]

It will make this part less safety.
We can check Member<GCedClass>::get() while we're constructing a GCMixin class
object.

+            ASSERT(m_gcGeneration == HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw)->gcGeneration());

[haraken, 2015/11/16 06:20:30]

Why is this safe? The behavior of
HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw) should be undefined if this is
called while constructing a mixin object.

[peria, 2015/11/16 07:04:42]

Please consider the following code.

We cannot refer HeapObjectHeader with m_a at (1), as you expect.  It fails with
a pure virtual function call.
And we can refer m_x correctly at (2), because its construction (of class Y) has
been completed.
Why can we refer m_a safely at (3)?  It is because its m_gcGeneration must be 0
and it does not call the ASSERT().


struct X : public GarbageCollectedMixin { ... };
struct Y : public GarbageCollected<Y>, public X { ... };

struct A : public GarbageCollectedMixin {
    A() : m_x(new Y), m_a(this) {
        // HeapObjectHeaderTrait<T>::heapObjectHeader(m_a); // ... (1)
        m_x.get(); // ... (2)
        m_a.get(); // ... (3)
    }
    Member<X> m_x;
    Member<A> m_a;
};
struct B : public GarbageCollected<B>, public A {
    B() : A() { }
}

+        }
+#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.
+    T* unsafeGet() const
+    {
+        return m_raw;
+    }
 
+    void clear()
+    {
+        m_raw = nullptr;
+    }
 
 protected:
     void checkPointer()
@@ -799,16 +819,18 @@ protected:
         // 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.

[haraken, 2015/11/16 02:47:49]

Update this comment.

[peria, 2015/11/16 05:33:26]

Done.

-        if (IsFullyDefined<T>::value && !IsGarbageCollectedMixin<T>::value)
-            ASSERT(HeapObjectHeader::fromPayload(m_raw)->checkHeader());
+        m_gcGeneration = HeapObjectHeaderTrait<T>::gcGeneration(m_raw);

[haraken, 2015/11/16 02:47:49]

Can we change this to:

  if (ThreadState::current()->isConstructingGCMixin())
    m_gcGeneration = 0;
  else
    m_gcGeneration =
HeapObjectHeaderTrait<T>::heapObjectHeader()->gcGeneration();

?

Then we can completely remove HeapObjectHeaderTrait<T>::gcGeneration.

[peria, 2015/11/16 05:33:26]

We can call HOHTrait<GCedClass>::gcGeneration() correctly while we're
constructing a GCMixin class object.
If we want to remove Trait<>::gcGeneration(), we have to change this line to

  if (ThreadState::current()->isConstructingGCMixin() &&
IsGarabgeCollectedMixin<T>::value)
    m_gcGeneration = 0;
  else
    m_gcGeneration =
HeapObjectHeaderTrait<T>::heapObjectHeader()->gcGeneration();

and need similar work in get().

 #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
@@ -872,6 +894,12 @@ public:
         return *this;
     }
 
+    T* get() const
+    {
+        // WeakMember may point to a dead object, so we skip the verification.

[haraken, 2015/11/16 02:47:49]

I wonder why this happens. It seems this CL is using too many unsafeGet()s than
needed.

I'm fine with landing these unsafeGet()s in this CL but most of them should be
replaced with get()s in follow-up CLs.

[peria, 2015/11/16 05:33:26]

It happens in weak processing of HeapHashSet<WeakMember<>>,
and I agree this CL uses unsafeGet() more generally than needed.
 
I'll do it.

+        return Member<T>::unsafeGet();
+    }
+
 private:
     T** cell() const { return const_cast<T**>(&this->m_raw); }
 
@@ -946,15 +974,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 {
@@ -1414,11 +1442,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>> {