commit 51eaa7f5547f9ddea92ce57c6c2df9534d5017b8
author Mathieu Chartier <mathieuc@google.com> Wed Oct 12 16:08:35 2016 -0700
committer Mathieu Chartier <mathieuc@google.com> Wed Oct 12 17:07:04 2016 -0700
tree 43456ed702c2ee5fe6ca1cbf7e9e53b2a4088109
parent c2f1fc70dba4db8751248c4ad15315946f6d13d0

Fix moving GC bug in cloning finalizable objects

It is not safe to have thread suspension in the PreFenceVisitor. The fix is
to add the finalizer reference in the caller.

Test: art/tools/run-libcore-tests.sh '--mode=host' '--variant=X32' --debug

Bug: 31113334

Change-Id: I1e4650f8b75408a3d07e2c51ac334ff98552cfb5

runtime/mirror/class.cc

diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index 7606915..689dd22 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -1002,7 +1002,7 @@
       REQUIRES_SHARED(Locks::mutator_lock_) {
     StackHandleScope<1> hs(self_);
     Handle<mirror::Class> h_new_class_obj(hs.NewHandle(obj->AsClass()));
-    mirror::Object::CopyObject(self_, h_new_class_obj.Get(), orig_->Get(), copy_bytes_);
+    mirror::Object::CopyObject(h_new_class_obj.Get(), orig_->Get(), copy_bytes_);
     mirror::Class::SetStatus(h_new_class_obj, Class::kStatusResolving, self_);
     h_new_class_obj->PopulateEmbeddedVTable(pointer_size_);
     h_new_class_obj->SetImt(imt_, pointer_size_);

runtime/mirror/object.cc

diff --git a/runtime/mirror/object.cc b/runtime/mirror/object.cc
index dbfe1d9..daee727 100644
--- a/runtime/mirror/object.cc
+++ b/runtime/mirror/object.cc
@@ -72,8 +72,7 @@
   ObjPtr<Object> const dest_obj_;
 };
 
-Object* Object::CopyObject(Thread* self,
-                           ObjPtr<mirror::Object> dest,
+Object* Object::CopyObject(ObjPtr<mirror::Object> dest,
                            ObjPtr<mirror::Object> src,
                            size_t num_bytes) {
   // Copy instance data.  Don't assume memcpy copies by words (b/32012820).
@@ -128,26 +127,21 @@
   } else {
     heap->WriteBarrierEveryFieldOf(dest);
   }
-  if (c->IsFinalizable()) {
-    heap->AddFinalizerReference(self, &dest);
-  }
   return dest.Ptr();
 }
 
 // An allocation pre-fence visitor that copies the object.
 class CopyObjectVisitor {
  public:
-  CopyObjectVisitor(Thread* self, Handle<Object>* orig, size_t num_bytes)
-      : self_(self), orig_(orig), num_bytes_(num_bytes) {
-  }
+  CopyObjectVisitor(Handle<Object>* orig, size_t num_bytes)
+      : orig_(orig), num_bytes_(num_bytes) {}
 
   void operator()(ObjPtr<Object> obj, size_t usable_size ATTRIBUTE_UNUSED) const
       REQUIRES_SHARED(Locks::mutator_lock_) {
-    Object::CopyObject(self_, obj, orig_->Get(), num_bytes_);
+    Object::CopyObject(obj, orig_->Get(), num_bytes_);
   }
 
  private:
-  Thread* const self_;
   Handle<Object>* const orig_;
   const size_t num_bytes_;
   DISALLOW_COPY_AND_ASSIGN(CopyObjectVisitor);
@@ -162,12 +156,15 @@
   StackHandleScope<1> hs(self);
   Handle<Object> this_object(hs.NewHandle(this));
   ObjPtr<Object> copy;
-  CopyObjectVisitor visitor(self, &this_object, num_bytes);
+  CopyObjectVisitor visitor(&this_object, num_bytes);
   if (heap->IsMovableObject(this)) {
     copy = heap->AllocObject<true>(self, GetClass(), num_bytes, visitor);
   } else {
     copy = heap->AllocNonMovableObject<true>(self, GetClass(), num_bytes, visitor);
   }
+  if (this_object->GetClass()->IsFinalizable()) {
+    heap->AddFinalizerReference(self, &copy);
+  }
   return copy.Ptr();
 }
 

runtime/mirror/object.h

diff --git a/runtime/mirror/object.h b/runtime/mirror/object.h
index 175b0c3..84aa96c 100644
--- a/runtime/mirror/object.h
+++ b/runtime/mirror/object.h
@@ -609,11 +609,10 @@
     }
   }
 
-  // A utility function that copies an object in a read barrier and
-  // write barrier-aware way. This is internally used by Clone() and
-  // Class::CopyOf().
-  static Object* CopyObject(Thread* self,
-                            ObjPtr<mirror::Object> dest,
+  // A utility function that copies an object in a read barrier and write barrier-aware way.
+  // This is internally used by Clone() and Class::CopyOf(). If the object is finalizable,
+  // it is the callers job to call Heap::AddFinalizerReference.
+  static Object* CopyObject(ObjPtr<mirror::Object> dest,
                             ObjPtr<mirror::Object> src,
                             size_t num_bytes)
       REQUIRES_SHARED(Locks::mutator_lock_);