From ca3ddeddba1b819b91ea9a62c2b2446c8929666f Mon Sep 17 00:00:00 2001 From: Roland Levillain Date: Fri, 10 Aug 2018 15:35:17 +0100 Subject: Remove region space memory protection before logging heap corruption. During heap corruption logging, before reading memory from the region space (to produce debugging information), make sure all of the space's pages are readable to prevent a memory protection fault (which would interrupt debug logging. Test: art/test.py Bug: 74064045 Change-Id: I6c804abef7fae602b80c0a2d1020ce870fa6b681 --- runtime/gc/collector/concurrent_copying-inl.h | 4 +++- runtime/gc/collector/concurrent_copying.cc | 19 +++++++++++++++---- runtime/gc/space/region_space.cc | 6 ++++++ runtime/gc/space/region_space.h | 6 ++++++ 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/runtime/gc/collector/concurrent_copying-inl.h b/runtime/gc/collector/concurrent_copying-inl.h index 36fefbdbc3..a6577dfe26 100644 --- a/runtime/gc/collector/concurrent_copying-inl.h +++ b/runtime/gc/collector/concurrent_copying-inl.h @@ -149,7 +149,9 @@ inline mirror::Object* ConcurrentCopying::Mark(Thread* const self, case space::RegionSpace::RegionType::kRegionTypeUnevacFromSpace: return MarkUnevacFromSpaceRegion(self, from_ref, region_space_bitmap_); default: - // The reference is in an unused region. + // The reference is in an unused region. Remove memory protection from + // the region space and log debugging information. + region_space_->Unprotect(); LOG(FATAL_WITHOUT_ABORT) << DumpHeapReference(holder, offset, from_ref); region_space_->DumpNonFreeRegions(LOG_STREAM(FATAL_WITHOUT_ABORT)); heap_->GetVerification()->LogHeapCorruption(holder, offset, from_ref, /* fatal */ true); diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc index b03f67152b..bc6305f171 100644 --- a/runtime/gc/collector/concurrent_copying.cc +++ b/runtime/gc/collector/concurrent_copying.cc @@ -1871,17 +1871,20 @@ void ConcurrentCopying::AssertToSpaceInvariant(mirror::Object* obj, } else if (type == RegionType::kRegionTypeUnevacFromSpace) { if (!IsMarkedInUnevacFromSpace(ref)) { LOG(FATAL_WITHOUT_ABORT) << "Found unmarked reference in unevac from-space:"; + // Remove memory protection from the region space and log debugging information. + region_space_->Unprotect(); LOG(FATAL_WITHOUT_ABORT) << DumpHeapReference(obj, offset, ref); } CHECK(IsMarkedInUnevacFromSpace(ref)) << ref; } else { // Not OK: either a from-space ref or a reference in an unused region. - // Do extra logging. if (type == RegionType::kRegionTypeFromSpace) { LOG(FATAL_WITHOUT_ABORT) << "Found from-space reference:"; } else { LOG(FATAL_WITHOUT_ABORT) << "Found reference in region with type " << type << ":"; } + // Remove memory protection from the region space and log debugging information. + region_space_->Unprotect(); LOG(FATAL_WITHOUT_ABORT) << DumpHeapReference(obj, offset, ref); if (obj != nullptr) { LogFromSpaceRefHolder(obj, offset); @@ -1949,17 +1952,20 @@ void ConcurrentCopying::AssertToSpaceInvariant(GcRootSource* gc_root_source, } else if (type == RegionType::kRegionTypeUnevacFromSpace) { if (!IsMarkedInUnevacFromSpace(ref)) { LOG(FATAL_WITHOUT_ABORT) << "Found unmarked reference in unevac from-space:"; + // Remove memory protection from the region space and log debugging information. + region_space_->Unprotect(); LOG(FATAL_WITHOUT_ABORT) << DumpGcRoot(ref); } CHECK(IsMarkedInUnevacFromSpace(ref)) << ref; } else { // Not OK: either a from-space ref or a reference in an unused region. - // Do extra logging. if (type == RegionType::kRegionTypeFromSpace) { LOG(FATAL_WITHOUT_ABORT) << "Found from-space reference:"; } else { LOG(FATAL_WITHOUT_ABORT) << "Found reference in region with type " << type << ":"; } + // Remove memory protection from the region space and log debugging information. + region_space_->Unprotect(); LOG(FATAL_WITHOUT_ABORT) << DumpGcRoot(ref); if (gc_root_source == nullptr) { // No info. @@ -2359,6 +2365,8 @@ mirror::Object* ConcurrentCopying::Copy(Thread* const self, // from a previous GC that is either inside or outside the allocated region. mirror::Class* klass = from_ref->GetClass(); if (UNLIKELY(klass == nullptr)) { + // Remove memory protection from the region space and log debugging information. + region_space_->Unprotect(); heap_->GetVerification()->LogHeapCorruption(holder, offset, from_ref, /* fatal */ true); } // There must not be a read barrier to avoid nested RB that might violate the to-space invariant. @@ -2638,8 +2646,11 @@ mirror::Object* ConcurrentCopying::MarkNonMoving(Thread* const self, } } if (is_los && !IsAligned(ref)) { - // Ref is a large object that is not aligned, it must be heap corruption. Dump data before - // AtomicSetReadBarrierState since it will fault if the address is not valid. + // Ref is a large object that is not aligned, it must be heap + // corruption. Remove memory protection and dump data before + // AtomicSetReadBarrierState since it will fault if the address is not + // valid. + region_space_->Unprotect(); heap_->GetVerification()->LogHeapCorruption(holder, offset, ref, /* fatal */ true); } // Not marked or on the allocation stack. Try to mark it. diff --git a/runtime/gc/space/region_space.cc b/runtime/gc/space/region_space.cc index 0569092bcd..db8253ca65 100644 --- a/runtime/gc/space/region_space.cc +++ b/runtime/gc/space/region_space.cc @@ -552,6 +552,12 @@ void RegionSpace::Clear() { evac_region_ = &full_region_; } +void RegionSpace::Unprotect() { + if (kProtectClearedRegions) { + CheckedCall(mprotect, __FUNCTION__, Begin(), Size(), PROT_READ | PROT_WRITE); + } +} + void RegionSpace::ClampGrowthLimit(size_t new_capacity) { MutexLock mu(Thread::Current(), region_lock_); CHECK_LE(new_capacity, NonGrowthLimitCapacity()); diff --git a/runtime/gc/space/region_space.h b/runtime/gc/space/region_space.h index 90f1f1dd2a..eb570d219a 100644 --- a/runtime/gc/space/region_space.h +++ b/runtime/gc/space/region_space.h @@ -108,6 +108,12 @@ class RegionSpace FINAL : public ContinuousMemMapAllocSpace { void Clear() OVERRIDE REQUIRES(!region_lock_); + // Remove memory protection from the whole region space, i.e. make memory + // pages backing the region area readable and writable. This method is useful + // to avoid page protection faults when dumping information about an invalid + // reference. + void Unprotect(); + // Change the non growth limit capacity to new capacity by shrinking or expanding the map. // Currently, only shrinking is supported. // Unlike implementations of this function in other spaces, we need to pass -- cgit v1.2.3-59-g8ed1b