Fix for potential moving GC bugs around proxy class.
- Handlerize proxy_class which is live across multiple allocation
points in ClassLinker::CreateProxyClass().
- In ClassLinker::CreateProxyClass(), insert a proxy class into the
class table before creating ArtFields for it (and update it later in
LinkClass()) because the field roots (ArtField::declaring_class_)
won't be updated by GC unless the class is in the class table. If GC
happens before they are updated by FixupTemporaryDeclaringClass()
from LinkClass(), FixupTemporaryDeclaringClass() may not update the
field roots correctly because the old class may already be moved but
the fields roots may not. Reduce a window of time where the fields
roots could be stale.
- In ClassLinker::LinkClass(), directly wrap a new class in a handle
to avoid a window of time where new_class may be potentially stale.
- Print more diagnostic info about the holder of the field upon a mark
sweep invalid ref crash.
- Add an additional sanity check in Field::GetArtField().
(cherry pick commit 08d1b5f2296c0f51507b8b443f4e39dfc161572c)
Bug: 20557050
Change-Id: I9ad32d304922da96b7e1fad262d97de21cbac776
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 292f830..ef94a30 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -1815,8 +1815,8 @@
// TODO: Use fast jobjects?
auto interfaces = hs.NewHandle<mirror::ObjectArray<mirror::Class>>(nullptr);
- mirror::Class* new_class = nullptr;
- if (!LinkClass(self, descriptor, klass, interfaces, &new_class)) {
+ MutableHandle<mirror::Class> h_new_class = hs.NewHandle<mirror::Class>(nullptr);
+ if (!LinkClass(self, descriptor, klass, interfaces, &h_new_class)) {
// Linking failed.
if (!klass->IsErroneous()) {
mirror::Class::SetStatus(klass, mirror::Class::kStatusError, self);
@@ -1824,10 +1824,8 @@
return nullptr;
}
self->AssertNoPendingException();
- CHECK(new_class != nullptr) << descriptor;
- CHECK(new_class->IsResolved()) << descriptor;
-
- Handle<mirror::Class> new_class_h(hs.NewHandle(new_class));
+ CHECK(h_new_class.Get() != nullptr) << descriptor;
+ CHECK(h_new_class->IsResolved()) << descriptor;
// Instrumentation may have updated entrypoints for all methods of all
// classes. However it could not update methods of this class while we
@@ -1838,7 +1836,7 @@
// suspending all threads to update entrypoints while we are doing it
// for this class.
DCHECK_EQ(self->GetState(), kRunnable);
- Runtime::Current()->GetInstrumentation()->InstallStubsForClass(new_class_h.Get());
+ Runtime::Current()->GetInstrumentation()->InstallStubsForClass(h_new_class.Get());
}
/*
@@ -1852,9 +1850,9 @@
* The class has been prepared and resolved but possibly not yet verified
* at this point.
*/
- Dbg::PostClassPrepare(new_class_h.Get());
+ Dbg::PostClassPrepare(h_new_class.Get());
- return new_class_h.Get();
+ return h_new_class.Get();
}
uint32_t ClassLinker::SizeOfClassWithoutEmbeddedTables(const DexFile& dex_file,
@@ -2738,11 +2736,7 @@
WriterMutexLock mu(Thread::Current(), *Locks::classlinker_classes_lock_);
auto existing_it = class_table_.FindWithHash(std::make_pair(descriptor, klass->GetClassLoader()),
hash);
- if (existing_it == class_table_.end()) {
- CHECK(klass->IsProxyClass());
- return nullptr;
- }
-
+ CHECK(existing_it != class_table_.end());
mirror::Class* existing = existing_it->Read();
CHECK_NE(existing, klass) << descriptor;
CHECK(!existing->IsResolved()) << descriptor;
@@ -3214,7 +3208,7 @@
jobjectArray interfaces, jobject loader,
jobjectArray methods, jobjectArray throws) {
Thread* self = soa.Self();
- StackHandleScope<9> hs(self);
+ StackHandleScope<10> hs(self);
MutableHandle<mirror::Class> klass(hs.NewHandle(
AllocClass(self, GetClassRoot(kJavaLangClass), sizeof(mirror::Class))));
if (klass.Get() == nullptr) {
@@ -3228,9 +3222,17 @@
klass->SetClassLoader(soa.Decode<mirror::ClassLoader*>(loader));
DCHECK_EQ(klass->GetPrimitiveType(), Primitive::kPrimNot);
klass->SetName(soa.Decode<mirror::String*>(name));
- mirror::Class* proxy_class = GetClassRoot(kJavaLangReflectProxy);
- klass->SetDexCache(proxy_class->GetDexCache());
+ klass->SetDexCache(GetClassRoot(kJavaLangReflectProxy)->GetDexCache());
mirror::Class::SetStatus(klass, mirror::Class::kStatusIdx, self);
+ std::string descriptor(GetDescriptorForProxy(klass.Get()));
+ size_t hash = ComputeModifiedUtf8Hash(descriptor.c_str());
+
+ // Insert the class before loading the fields as the field roots
+ // (ArtField::declaring_class_) are only visited from the class
+ // table. There can't be any suspend points between inserting the
+ // class and setting the field arrays below.
+ mirror::Class* existing = InsertClass(descriptor.c_str(), klass.Get(), hash);
+ CHECK(existing == nullptr);
// Instance fields are inherited, but we add a couple of static fields...
const size_t num_fields = 2;
@@ -3253,18 +3255,21 @@
// Proxies have 1 direct method, the constructor
{
- mirror::ObjectArray<mirror::ArtMethod>* directs = AllocArtMethodArray(self, 1);
- if (UNLIKELY(directs == nullptr)) {
+ StackHandleScope<2> hs2(self);
+ Handle<mirror::ObjectArray<mirror::ArtMethod>> directs =
+ hs2.NewHandle(AllocArtMethodArray(self, 1));
+ if (UNLIKELY(directs.Get() == nullptr)) {
CHECK(self->IsExceptionPending()); // OOME.
return nullptr;
}
- klass->SetDirectMethods(directs);
- mirror::ArtMethod* constructor = CreateProxyConstructor(self, klass, proxy_class);
- if (UNLIKELY(constructor == nullptr)) {
+ klass->SetDirectMethods(directs.Get());
+ Handle<mirror::ArtMethod> constructor =
+ hs2.NewHandle(CreateProxyConstructor(self, klass));
+ if (UNLIKELY(constructor.Get() == nullptr)) {
CHECK(self->IsExceptionPending()); // OOME.
return nullptr;
}
- klass->SetDirectMethod(0, constructor);
+ klass->SetDirectMethod(0, constructor.Get());
}
// Create virtual method using specified prototypes.
@@ -3273,35 +3278,38 @@
<< PrettyClass(h_methods->GetClass());
const size_t num_virtual_methods = h_methods->GetLength();
{
- mirror::ObjectArray<mirror::ArtMethod>* virtuals = AllocArtMethodArray(self,
- num_virtual_methods);
- if (UNLIKELY(virtuals == nullptr)) {
+ StackHandleScope<1> hs2(self);
+ Handle<mirror::ObjectArray<mirror::ArtMethod>> virtuals =
+ hs2.NewHandle(AllocArtMethodArray(self, num_virtual_methods));
+ if (UNLIKELY(virtuals.Get() == nullptr)) {
CHECK(self->IsExceptionPending()); // OOME.
return nullptr;
}
- klass->SetVirtualMethods(virtuals);
+ klass->SetVirtualMethods(virtuals.Get());
}
for (size_t i = 0; i < num_virtual_methods; ++i) {
- StackHandleScope<1> hs2(self);
+ StackHandleScope<2> hs2(self);
Handle<mirror::ArtMethod> prototype(hs2.NewHandle(h_methods->Get(i)->GetArtMethod()));
- mirror::ArtMethod* clone = CreateProxyMethod(self, klass, prototype);
- if (UNLIKELY(clone == nullptr)) {
+ Handle<mirror::ArtMethod> clone(hs2.NewHandle(CreateProxyMethod(self, klass, prototype)));
+ if (UNLIKELY(clone.Get() == nullptr)) {
CHECK(self->IsExceptionPending()); // OOME.
return nullptr;
}
- klass->SetVirtualMethod(i, clone);
+ klass->SetVirtualMethod(i, clone.Get());
}
- klass->SetSuperClass(proxy_class); // The super class is java.lang.reflect.Proxy
- mirror::Class::SetStatus(klass, mirror::Class::kStatusLoaded, self); // Now effectively in the loaded state.
+ // The super class is java.lang.reflect.Proxy
+ klass->SetSuperClass(GetClassRoot(kJavaLangReflectProxy));
+ // Now effectively in the loaded state.
+ mirror::Class::SetStatus(klass, mirror::Class::kStatusLoaded, self);
self->AssertNoPendingException();
- std::string descriptor(GetDescriptorForProxy(klass.Get()));
- mirror::Class* new_class = nullptr;
+ MutableHandle<mirror::Class> new_class = hs.NewHandle<mirror::Class>(nullptr);
{
// Must hold lock on object when resolved.
ObjectLock<mirror::Class> resolution_lock(self, klass);
- // Link the fields and virtual methods, creating vtable and iftables
+ // Link the fields and virtual methods, creating vtable and iftables.
+ // The new class will replace the old one in the class table.
Handle<mirror::ObjectArray<mirror::Class> > h_interfaces(
hs.NewHandle(soa.Decode<mirror::ObjectArray<mirror::Class>*>(interfaces)));
if (!LinkClass(self, descriptor.c_str(), klass, h_interfaces, &new_class)) {
@@ -3309,15 +3317,14 @@
return nullptr;
}
}
-
CHECK(klass->IsRetired());
- CHECK_NE(klass.Get(), new_class);
- klass.Assign(new_class);
+ CHECK_NE(klass.Get(), new_class.Get());
+ klass.Assign(new_class.Get());
- CHECK_EQ(interfaces_sfield->GetDeclaringClass(), new_class);
+ CHECK_EQ(interfaces_sfield->GetDeclaringClass(), klass.Get());
interfaces_sfield->SetObject<false>(klass.Get(),
soa.Decode<mirror::ObjectArray<mirror::Class>*>(interfaces));
- CHECK_EQ(throws_sfield->GetDeclaringClass(), new_class);
+ CHECK_EQ(throws_sfield->GetDeclaringClass(), klass.Get());
throws_sfield->SetObject<false>(klass.Get(),
soa.Decode<mirror::ObjectArray<mirror::ObjectArray<mirror::Class> >*>(throws));
@@ -3338,7 +3345,8 @@
CheckProxyMethod(virtual_method, prototype);
}
- mirror::String* decoded_name = soa.Decode<mirror::String*>(name);
+ StackHandleScope<1> hs2(self);
+ Handle<mirror::String> decoded_name = hs2.NewHandle(soa.Decode<mirror::String*>(name));
std::string interfaces_field_name(StringPrintf("java.lang.Class[] %s.interfaces",
decoded_name->ToModifiedUtf8().c_str()));
CHECK_EQ(PrettyField(klass->GetStaticField(0)), interfaces_field_name);
@@ -3352,9 +3360,6 @@
CHECK_EQ(klass.Get()->GetThrows(),
soa.Decode<mirror::ObjectArray<mirror::ObjectArray<mirror::Class>>*>(throws));
}
- mirror::Class* existing = InsertClass(descriptor.c_str(), klass.Get(),
- ComputeModifiedUtf8Hash(descriptor.c_str()));
- CHECK(existing == nullptr);
return klass.Get();
}
@@ -3389,17 +3394,16 @@
mirror::ArtMethod* ClassLinker::CreateProxyConstructor(Thread* self,
- Handle<mirror::Class> klass,
- mirror::Class* proxy_class) {
+ Handle<mirror::Class> klass) {
// Create constructor for Proxy that must initialize h
mirror::ObjectArray<mirror::ArtMethod>* proxy_direct_methods =
- proxy_class->GetDirectMethods();
+ GetClassRoot(kJavaLangReflectProxy)->GetDirectMethods();
CHECK_EQ(proxy_direct_methods->GetLength(), 16);
mirror::ArtMethod* proxy_constructor = proxy_direct_methods->Get(2);
// Ensure constructor is in dex cache so that we can use the dex cache to look up the overridden
// constructor method.
- proxy_class->GetDexCache()->SetResolvedMethod(proxy_constructor->GetDexMethodIndex(),
- proxy_constructor);
+ GetClassRoot(kJavaLangReflectProxy)->GetDexCache()->SetResolvedMethod(
+ proxy_constructor->GetDexMethodIndex(), proxy_constructor);
// Clone the existing constructor of Proxy (our constructor would just invoke it so steal its
// code_ too)
mirror::ArtMethod* constructor = down_cast<mirror::ArtMethod*>(proxy_constructor->Clone(self));
@@ -3923,7 +3927,7 @@
bool ClassLinker::LinkClass(Thread* self, const char* descriptor, Handle<mirror::Class> klass,
Handle<mirror::ObjectArray<mirror::Class>> interfaces,
- mirror::Class** new_class) {
+ MutableHandle<mirror::Class>* h_new_class_out) {
CHECK_EQ(mirror::Class::kStatusLoaded, klass->GetStatus());
if (!LinkSuperClass(klass)) {
@@ -3956,25 +3960,23 @@
// This will notify waiters on klass that saw the not yet resolved
// class in the class_table_ during EnsureResolved.
mirror::Class::SetStatus(klass, mirror::Class::kStatusResolved, self);
- *new_class = klass.Get();
+ h_new_class_out->Assign(klass.Get());
} else {
CHECK(!klass->IsResolved());
// Retire the temporary class and create the correctly sized resolved class.
- *new_class = klass->CopyOf(self, class_size, &imt_handle_scope);
- if (UNLIKELY(*new_class == nullptr)) {
+ StackHandleScope<1> hs(self);
+ auto h_new_class = hs.NewHandle<mirror::Class>(
+ klass->CopyOf(self, class_size, &imt_handle_scope));
+ if (UNLIKELY(h_new_class.Get() == nullptr)) {
CHECK(self->IsExceptionPending()); // Expect an OOME.
mirror::Class::SetStatus(klass, mirror::Class::kStatusError, self);
return false;
}
- CHECK_EQ((*new_class)->GetClassSize(), class_size);
- StackHandleScope<1> hs(self);
- auto new_class_h = hs.NewHandleWrapper<mirror::Class>(new_class);
- ObjectLock<mirror::Class> lock(self, new_class_h);
-
- FixupTemporaryDeclaringClass(klass.Get(), new_class_h.Get());
-
- mirror::Class* existing = UpdateClass(descriptor, new_class_h.Get(),
+ CHECK_EQ(h_new_class->GetClassSize(), class_size);
+ ObjectLock<mirror::Class> lock(self, h_new_class);
+ FixupTemporaryDeclaringClass(klass.Get(), h_new_class.Get());
+ mirror::Class* existing = UpdateClass(descriptor, h_new_class.Get(),
ComputeModifiedUtf8Hash(descriptor));
CHECK(existing == nullptr || existing == klass.Get());
@@ -3982,10 +3984,12 @@
// class_table_ during EnsureResolved.
mirror::Class::SetStatus(klass, mirror::Class::kStatusRetired, self);
- CHECK_EQ(new_class_h->GetStatus(), mirror::Class::kStatusResolving);
+ CHECK_EQ(h_new_class->GetStatus(), mirror::Class::kStatusResolving);
// This will notify waiters on new_class that saw the not yet resolved
// class in the class_table_ during EnsureResolved.
- mirror::Class::SetStatus(new_class_h, mirror::Class::kStatusResolved, self);
+ mirror::Class::SetStatus(h_new_class, mirror::Class::kStatusResolved, self);
+ // Return the new class.
+ h_new_class_out->Assign(h_new_class.Get());
}
return true;
}
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index 95c8aa0..947e152 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -49,6 +49,7 @@
} // namespace mirror
template<class T> class Handle;
+template<class T> class MutableHandle;
class InternTable;
template<class T> class ObjectLock;
class Runtime;
@@ -572,7 +573,7 @@
bool LinkClass(Thread* self, const char* descriptor, Handle<mirror::Class> klass,
Handle<mirror::ObjectArray<mirror::Class>> interfaces,
- mirror::Class** new_class)
+ MutableHandle<mirror::Class>* h_new_class_out)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
bool LinkSuperClass(Handle<mirror::Class> klass)
@@ -622,8 +623,7 @@
// Returns the boot image oat file.
const OatFile* GetBootOatFile() SHARED_LOCKS_REQUIRED(dex_lock_);
- mirror::ArtMethod* CreateProxyConstructor(Thread* self, Handle<mirror::Class> klass,
- mirror::Class* proxy_class)
+ mirror::ArtMethod* CreateProxyConstructor(Thread* self, Handle<mirror::Class> klass)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
mirror::ArtMethod* CreateProxyMethod(Thread* self, Handle<mirror::Class> klass,
Handle<mirror::ArtMethod> prototype)
diff --git a/runtime/gc/collector/mark_sweep.cc b/runtime/gc/collector/mark_sweep.cc
index 5401b56..2db5650 100644
--- a/runtime/gc/collector/mark_sweep.cc
+++ b/runtime/gc/collector/mark_sweep.cc
@@ -389,6 +389,9 @@
ArtField* field = holder_->FindFieldByOffset(offset_);
LOG(INTERNAL_FATAL) << "Field info: "
<< " holder=" << holder_
+ << " holder is "
+ << (mark_sweep_->GetHeap()->IsLiveObjectLocked(holder_)
+ ? "alive" : "dead")
<< " holder_size=" << holder_size
<< " holder_type=" << PrettyTypeOf(holder_)
<< " offset=" << offset_.Uint32Value()
@@ -404,6 +407,12 @@
? holder_->AsClass()->NumReferenceStaticFields()
: holder_->GetClass()->NumReferenceInstanceFields())
<< "\n";
+ // Print the memory content of the holder.
+ for (size_t i = 0; i < holder_size / sizeof(uint32_t); ++i) {
+ uint32_t* p = reinterpret_cast<uint32_t*>(holder_);
+ LOG(INTERNAL_FATAL) << &p[i] << ": " << "holder+" << (i * sizeof(uint32_t)) << " = "
+ << std::hex << p[i];
+ }
}
PrintFileToLog("/proc/self/maps", LogSeverity::INTERNAL_FATAL);
MemMap::DumpMaps(LOG(INTERNAL_FATAL), true);
diff --git a/runtime/mirror/field.cc b/runtime/mirror/field.cc
index 933784e..ac56129 100644
--- a/runtime/mirror/field.cc
+++ b/runtime/mirror/field.cc
@@ -69,6 +69,7 @@
mirror::DexCache* const dex_cache = declaring_class->GetDexCache();
ArtField* const art_field = dex_cache->GetResolvedField(GetDexFieldIndex(), sizeof(void*));
CHECK(art_field != nullptr);
+ CHECK_EQ(declaring_class, art_field->GetDeclaringClass());
return art_field;
}
