[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 638 matching lines @@
 public:
     PersistentHeapDeque() { }
 
     template<size_t otherCapacity>
     PersistentHeapDeque(const HeapDeque<T, otherCapacity>& other)
         : PersistentHeapCollectionBase<HeapDeque<T, inlineCapacity>>(other)
     {
     }
 };
 
+template<typename T, bool = IsGarbageCollectedMixin<T>::value> class GarbageCollectedMixinTrait;
+template<typename T>
+class GarbageCollectedMixinTrait<T, true> {
+public:
+    static HeapObjectHeader* heapObjectHeader(T* obj)
+    {
+        return obj->heapObjectHeader();
+    }
+    static uint16_t gcGeneration(T* obj)
+    {
+        // While constructing GarbageCollectedMixin classes, we skip looking
+        // for gcGeneration, because the object may not be fully allocated.

[haraken, 2015/11/10 16:37:05]

Sorry, I'm getting confused. Why can't we call
heapObjectHeader(obj)->gcGeneration() against a GarbageCollectedMixin object
that is being constructed? heapObjectHeader(obj)->gcGeneration() shouldn't
depend on the vtable of the mixin object, so I think we can just call it... (I
may be missing something.)

[peria, 2015/11/11 02:46:34]

obj->heapObjectHeader() (on line#665) depends on the vtable of GCMixin.

What we want to do here is to get the head address of the object, that |obj| is
pointing to.  Because 'T*' is a pointer for GCMixin class, |obj| does not point
to the head of the actual object.
I think using a virtual method 'heapObjectHeader()' is the only solution, but if
we can get the head address without virtual methods, I'm happy to update this.

[haraken, 2015/11/11 04:19:29]

Thanks for the clarification; now I understand the issue.

Maybe can we introduce HeapObjectHeaderTrait like this?

template<typename T>
class HeapObjectHeaderTrait<T, true> {
public:
  static HeapObjectHeader* heapObjectHeader(T* obj) {
    ASSERT(!ThreadState::current()->inMixinConstructor());
    return obj->heapObjectHeader();
  }
  static HeapObjectHeader* heapObjectHeaderIfExists(T* obj) {
    if (ThreadState::current()->inMixinConstructor())
      return nullptr;
    return obj->heapObjectHeader();
  }
};

If we have the HeapObjectHeaderTrait, we can remove ObjectAliveTrait (in a
follow-up CL).

[peria, 2015/11/11 05:51:51]

SGTM
I moved this Trait to Heap.h in PS6.

+        if (ThreadState::current()->isGCForbidden())

[haraken, 2015/11/10 16:37:05]

Let's rename isGCForbidden() to isConstructingGCMixin().

[peria, 2015/11/11 05:51:51]

Renaming it looks confusing in other places, so I made it as
a new method in PS5.

+            return 0;
+        return heapObjectHeader(obj)->gcGeneration();
+    }
+};
+template<typename T>
+class GarbageCollectedMixinTrait<T, false> {
+public:
+    static HeapObjectHeader* heapObjectHeader(T* obj)
+    {
+        return HeapObjectHeader::fromPayload(obj);
+    }
+    static uint16_t gcGeneration(T* obj)
+    {
+        return heapObjectHeader(obj)->gcGeneration();
+    }
+};
+
 // Members are used in classes to contain strong pointers to other oilpan heap
 // allocated objects.
 // All Member fields of a class must be traced in the class' trace method.
 // During the mark phase of the GC all live objects are marked as live and
 // all Member fields of a live object will be traced marked as live as well.
 template<typename T>
 class Member {
 public:
     Member() : m_raw(nullptr)
     {
+        checkPointer();
     }
 
     Member(std::nullptr_t) : m_raw(nullptr)
     {
+        checkPointer();
     }
 
     Member(T* raw) : m_raw(raw)
     {
         checkPointer();
     }
 
     explicit Member(T& raw) : m_raw(&raw)
     {
         checkPointer();
@@ skipping 72 matching lines @@
     Member& operator=(RawPtr<U> other)
     {
         m_raw = other;
         checkPointer();
         return *this;
     }
 
     Member& operator=(std::nullptr_t)
     {
         m_raw = nullptr;
+        checkPointer();
         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)
+        // Verify that this handle points a live on-heap object, if a pointer

[haraken, 2015/11/10 16:37:05]

points to

[peria, 2015/11/11 02:46:34]

Done.

+        // has been assigned to this handle out of GarbageCollectedMixin constructions.
+        if (m_raw && m_gcGeneration) {
+            ASSERT(m_gcGeneration == GarbageCollectedMixinTrait<T>::heapObjectHeader(m_raw)->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.
+    T* unsafeGet() const
+    {
+        return m_raw;
+    }
 
+    void clear()
+    {
+        m_raw = nullptr;
+        checkPointer();
+    }
 
 protected:
     void checkPointer()
     {
 #if ENABLE(ASSERT)
-        if (!m_raw)
+        if (!m_raw) {
+            m_gcGeneration = 0;

[haraken, 2015/11/10 16:37:05]

You won't need this. Given that you're checking if(m_raw && m_gcGeneration) at
line 812, you don't need to reset m_gcGeneration when m_raw becomes 0. This also
means that you don't need to call checkPointer() when assigning nullptr.

[peria, 2015/11/11 02:46:34]

Agree. Done.

             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.
-        if (IsFullyDefined<T>::value && !IsGarbageCollectedMixin<T>::value)
-            ASSERT(HeapObjectHeader::fromPayload(m_raw)->checkHeader());
+        m_gcGeneration = GarbageCollectedMixinTrait<T>::gcGeneration(m_raw);

[haraken, 2015/11/11 04:19:29]

So you can update the code to:

  HeapObjectHeader* header = 
HeapObjectHeaderTrait<T>::heapObjectHeaderIfExists(m_raw);
  if (!header)
    return;
  ASSERT(header->checkHeader());
  m_gcGeneration = header->gcGeneration();
  ASSERT(!m_gcGeneration);

My question is if the code compiles without including T.h... (The above comment
implies that the code won't compile without including T.h.)

[peria, 2015/11/11 05:51:51]

Acknowledged.
IIUC, in all files using Member<T>, we need to include T.h to judge
if T is a GCed class.  Otherwise, GC plugin should stop compiling.

 #endif
     }
 
     T* m_raw;
 
+#if ENABLE(ASSERT)
+    uint16_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 40 matching lines @@
     WeakMember& operator=(const RawPtr<U>& other)
     {
         this->m_raw = other;
         this->checkPointer();
         return *this;
     }
 
     WeakMember& operator=(std::nullptr_t)
     {
         this->m_raw = nullptr;
+        this->checkPointer();
         return *this;
     }
 
 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
@@ skipping 52 matching lines @@
     UntracedMember& operator=(const RawPtr<U>& other)
     {
         this->m_raw = other;
         this->checkPointer();
         return *this;
     }
 
     UntracedMember& operator=(std::nullptr_t)
     {
         this->m_raw = nullptr;
+        this->checkPointer();
         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>
 class DummyBase {
@@ skipping 446 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