[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 && !ThreadState::current()->isConstructingGCMixin()) {

[haraken, 2015/11/16 09:38:20]

I begin to think that we should introduce gcGenerationUnchecked. In other words,
we should distinguish the fact that the Member is pointing to an already-freed
object (<== This is a use-after-free bug!) from the fact that the gcGeneration
doesn't need to be checked (e.g., cannot be checked while constructing an
object).

Then this check should be:

  m_gcGeneration != gcGenerationUnchecked

[peria, 2015/11/16 12:28:45]

Done.

+            HeapObjectHeader* header = HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw);
+            ASSERT(!header || (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.

[haraken, 2015/11/16 09:38:20]

I believe that unsafeGet() should not be needed. You don't need to address it in
this CL but let's add a TODO.

// TODO: We should remove this.

[peria, 2015/11/16 12:28:45]

Yes, let's.

+    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.
-        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.
+        // For technical reason, we set gcGenerationForFreeListEntry in
+        // m_gcGeneration in case that this method is called in a constructor
+        // of a GCMixin object.
+        // TODO(peria): Set m_gcGeneration even if this is called in a
+        // GCMixin constructor.
+        if (!ThreadState::current()->isConstructingGCMixin()) {
+            HeapObjectHeader* header = HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw);
+            if (header)
+                m_gcGeneration = header->gcGeneration();

[haraken, 2015/11/16 09:38:20]

Who assigns 0 to m_gcGeneration when we fail at getting a valid gcGeneration
(e.g., m_raw == 0, ThreadState::current()->isConstructingGCMixin() is true etc)?

I think the invariant should be:

  m_raw == 0 or we don't want to check the gcGeneration for some reason iff.
m_gcGeneration == gcGenerationUnchecked

  use-after-free iff. m_gcGeneration == gcGenerationForFreeListEntry

  valid otherwise

[peria, 2015/11/16 12:28:45]

Oops.
Set it by default.
It should be

  if m_raw == 0 or m_gcGeneration == gcGenerationUnchecked:
      skip verification
  if m_gcGeneration == gcGenerationForFreeListEntry:
      invalid
  if m_gcGeneration == HeapObjectHeader::gcGeneration:
      valid
  otherwise:
      invalid

'otherwise' includes that a new object of the same class is allocated
on the same address.

+        }
 #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;
     }
 
+    // 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); }
 
     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