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

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

 /*
  * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 753 matching lines @@
         m_raw = nullptr;
         return *this;
     }
 
     void swap(Member<T>& other)
     {
         std::swap(m_raw, other.m_raw);
         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()
     {
 #if ENABLE(ASSERT) && defined(ADDRESS_SANITIZER)
         if (!m_raw)
             return;
         // HashTable can store a special value (which is not aligned to the
         // allocation granularity) to Member<> to represent a deleted entry.
         // Thus we treat a pointer that is not aligned to the granularity
         // as a valid pointer.
         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.

[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
 // heap allocated objects.
 // However instead of creating a strong pointer to the object, the WeakMember creates
 // a weak pointer, which does not keep the pointee alive. Hence if all pointers to
 // to a heap allocated object are weak the object will be garbage collected. At the
 // time of GC the weak pointers will automatically be set to null.
 template<typename T>
 class WeakMember : public Member<T> {
@@ skipping 43 matching lines @@
         this->checkPointer();
         return *this;
     }
 
     WeakMember& operator=(std::nullptr_t)
     {
         this->m_raw = nullptr;
         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); }
 
     template<typename Derived> friend class VisitorHelper;
 };
 
 // UntracedMember is a pointer to an on-heap object that is not traced for some
 // reason. Please don't use this unless you understand what you're doing.
 // Basically, all pointers to on-heap objects must be stored in either of
 // Persistent, Member or WeakMember. It is not allowed to leave raw pointers to
@@ skipping 54 matching lines @@
     }
 
     UntracedMember& operator=(std::nullptr_t)
     {
         this->m_raw = nullptr;
         return *this;
     }
 };
 
 // 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 {
 public:
     DummyBase() { }
     ~DummyBase() { }
 };
 
 // We need this explicit instantiation for component build on Windows.
 template<>
@@ skipping 439 matching lines @@
 
 template<typename T> struct PtrHash<blink::Member<T>> : PtrHash<T*> {
     template<typename U>
     static unsigned hash(const U& key) { return PtrHash<T*>::hash(key); }
     static bool equal(T* a, const blink::Member<T>& b) { return a == b; }
     static bool equal(const blink::Member<T>& a, T* b) { return a == b; }
     template<typename U, typename V>
     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>> {
     using Hash = PtrHash<blink::Member<T>>;
 };
 
 template<typename T> struct DefaultHash<blink::WeakMember<T>> {
     using Hash = PtrHash<blink::WeakMember<T>>;
 };
 
@@ skipping 72 matching lines @@
     // TODO(sof): extend WTF::FunctionWrapper call overloading to also handle (CrossThread)WeakPersistent.
     static T* unwrap(const StorageType& value) { return value.get(); }
 };
 
 template<typename T>
 PassRefPtr<T> adoptRef(blink::RefCountedGarbageCollected<T>*) = delete;
 
 } // namespace WTF
 
 #endif