Only visit pointer arrays once during image relocation

Previously they could get visited twice, this caused corruption of
the app image if a pointer was updated twice.

Bug: 22858531
Change-Id: I1f1ba1ba5dc205be07dba51bc6ce7a825c82b33a

diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc
index 0c06c38..894ce9a 100644 (file)
--- a/runtime/gc/space/image_space.cc
+++ b/runtime/gc/space/image_space.cc
@@ -676,13 +676,17 @@ class RelocationRange {
         dest_(dest),
         length_(length) {}
 
-  bool ContainsSource(uintptr_t address) const {
+  bool InSource(uintptr_t address) const {
     return address - source_ < length_;
   }
 
+  bool InDest(uintptr_t address) const {
+    return address - dest_ < length_;
+  }
+
   // Translate a source address to the destination space.
   uintptr_t ToDest(uintptr_t address) const {
-    DCHECK(ContainsSource(address));
+    DCHECK(InSource(address));
     return address + Delta();
   }
 
@@ -724,24 +728,28 @@ class FixupVisitor : public ValueObject {
   template <typename T>
   ALWAYS_INLINE T* ForwardObject(T* src) const {
     const uintptr_t uint_src = reinterpret_cast<uintptr_t>(src);
-    if (boot_image_.ContainsSource(uint_src)) {
+    if (boot_image_.InSource(uint_src)) {
       return reinterpret_cast<T*>(boot_image_.ToDest(uint_src));
     }
-    if (app_image_.ContainsSource(uint_src)) {
+    if (app_image_.InSource(uint_src)) {
       return reinterpret_cast<T*>(app_image_.ToDest(uint_src));
     }
+    // Since we are fixing up the app image, there should only be pointers to the app image and
+    // boot image.
+    DCHECK(src == nullptr) << reinterpret_cast<const void*>(src);
     return src;
   }
 
   // Return the relocated address of a code pointer (contained by an oat file).
   ALWAYS_INLINE const void* ForwardCode(const void* src) const {
     const uintptr_t uint_src = reinterpret_cast<uintptr_t>(src);
-    if (boot_oat_.ContainsSource(uint_src)) {
+    if (boot_oat_.InSource(uint_src)) {
      return reinterpret_cast<const void*>(boot_oat_.ToDest(uint_src));
     }
-    if (app_oat_.ContainsSource(uint_src)) {
+    if (app_oat_.InSource(uint_src)) {
       return reinterpret_cast<const void*>(app_oat_.ToDest(uint_src));
     }
+    DCHECK(src == nullptr) << src;
     return src;
   }
 
@@ -766,6 +774,11 @@ class FixupObjectAdapter : public FixupVisitor {
   template<typename... Args>
   explicit FixupObjectAdapter(Args... args) : FixupVisitor(args...) {}
 
+  // Must be called on pointers that already have been relocated to the destination relocation.
+  ALWAYS_INLINE bool IsInAppImage(mirror::Object* object) const {
+    return app_image_.InDest(reinterpret_cast<uintptr_t>(object));
+  }
+
   template <typename T>
   T* operator()(T* obj) const {
     return ForwardObject(obj);
@@ -816,7 +829,10 @@ class FixupRootVisitor : public FixupVisitor {
 class FixupObjectVisitor : public FixupVisitor {
  public:
   template<typename... Args>
-  explicit FixupObjectVisitor(Args... args) : FixupVisitor(args...) {}
+  explicit FixupObjectVisitor(gc::accounting::ContinuousSpaceBitmap* pointer_array_visited,
+                              Args... args)
+      : FixupVisitor(args...),
+        pointer_array_visited_(pointer_array_visited) {}
 
   // Fix up separately since we also need to fix up method entrypoints.
   ALWAYS_INLINE void VisitRootIfNonNull(
@@ -841,6 +857,19 @@ class FixupObjectVisitor : public FixupVisitor {
     }
   }
 
+  // Visit a pointer array and forward corresponding native data. Ignores pointer arrays in the
+  // boot image. Uses the bitmap to ensure the same array is not visited multiple times.
+  template <typename Visitor>
+  void VisitPointerArray(mirror::PointerArray* array, const Visitor& visitor) const
+      NO_THREAD_SAFETY_ANALYSIS {
+    if (array != nullptr &&
+        visitor.IsInAppImage(array) &&
+        !pointer_array_visited_->Test(array)) {
+      array->Fixup<kVerifyNone, kWithoutReadBarrier>(array, sizeof(void*), visitor);
+      pointer_array_visited_->Set(array);
+    }
+  }
+
   // java.lang.ref.Reference visitor.
   void operator()(mirror::Class* klass ATTRIBUTE_UNUSED, mirror::Reference* ref) const
       SHARED_REQUIRES(Locks::mutator_lock_) REQUIRES(Locks::heap_bitmap_lock_) {
@@ -859,11 +888,9 @@ class FixupObjectVisitor : public FixupVisitor {
       mirror::Class* klass = obj->AsClass<kVerifyNone, kWithoutReadBarrier>();
       FixupObjectAdapter visitor(boot_image_, boot_oat_, app_image_, app_oat_);
       klass->FixupNativePointers<kVerifyNone, kWithoutReadBarrier>(klass, sizeof(void*), visitor);
-      // Deal with the arrays.
-      mirror::PointerArray* vtable = klass->GetVTable<kVerifyNone, kWithoutReadBarrier>();
-      if (vtable != nullptr) {
-        vtable->Fixup<kVerifyNone, kWithoutReadBarrier>(vtable, sizeof(void*), visitor);
-      }
+      // Deal with the pointer arrays. Use the helper function since multiple classes can reference
+      // the same arrays.
+      VisitPointerArray(klass->GetVTable<kVerifyNone, kWithoutReadBarrier>(), visitor);
       mirror::IfTable* iftable = klass->GetIfTable<kVerifyNone, kWithoutReadBarrier>();
       if (iftable != nullptr) {
         for (int32_t i = 0, count = iftable->Count(); i < count; ++i) {
@@ -871,12 +898,15 @@ class FixupObjectVisitor : public FixupVisitor {
             mirror::PointerArray* methods =
                 iftable->GetMethodArray<kVerifyNone, kWithoutReadBarrier>(i);
             DCHECK(methods != nullptr);
-            methods->Fixup<kVerifyNone, kWithoutReadBarrier>(methods, sizeof(void*), visitor);
+            VisitPointerArray(methods, visitor);
           }
         }
       }
     }
   }
+
+ private:
+  gc::accounting::ContinuousSpaceBitmap* const pointer_array_visited_;
 };
 
 class ForwardObjectAdapter {
@@ -1010,9 +1040,18 @@ static bool RelocateInPlace(ImageHeader& image_header,
   const ImageSection& objects_section = image_header.GetImageSection(ImageHeader::kSectionObjects);
   uintptr_t objects_begin = reinterpret_cast<uintptr_t>(target_base + objects_section.Offset());
   uintptr_t objects_end = reinterpret_cast<uintptr_t>(target_base + objects_section.End());
-  // Two pass approach, fix up all classes first, then fix up non class-objects.
-  FixupObjectVisitor fixup_object_visitor(boot_image, boot_oat, app_image, app_oat);
   if (fixup_image) {
+    // Two pass approach, fix up all classes first, then fix up non class-objects.
+    // The visited bitmap is used to ensure that pointer arrays are not forwarded twice.
+    std::unique_ptr<gc::accounting::ContinuousSpaceBitmap> visited_bitmap(
+        gc::accounting::ContinuousSpaceBitmap::Create("Pointer array bitmap",
+                                                      target_base,
+                                                      image_header.GetImageSize()));
+    FixupObjectVisitor fixup_object_visitor(visited_bitmap.get(),
+                                            boot_image,
+                                            boot_oat,
+                                            app_image,
+                                            app_oat);
     TimingLogger::ScopedTiming timing("Fixup classes", &logger);
     // Fixup class only touches app image classes, don't need the mutator lock since the space is
     // not yet visible to the GC.
@@ -1025,7 +1064,7 @@ static bool RelocateInPlace(ImageHeader& image_header,
     bitmap->VisitMarkedRange(objects_begin, objects_end, fixup_object_visitor);
     FixupObjectAdapter fixup_adapter(boot_image, boot_oat, app_image, app_oat);
     // Fixup image roots.
-    CHECK(app_image.ContainsSource(reinterpret_cast<uintptr_t>(
+    CHECK(app_image.InSource(reinterpret_cast<uintptr_t>(
         image_header.GetImageRoots<kWithoutReadBarrier>())));
     image_header.RelocateImageObjects(app_image.Delta());
     CHECK_EQ(image_header.GetImageBegin(), target_base);