JDWP: fix thread_list deadlock
Limits the scope of Locks::thread_list_lock_ locking in the debugger
so we do not try to lock it twice when creating a JDWP id (because
calling Object::IdentityHashCode may need to take the lock).
Bug: 20048099
Change-Id: I305dd72ccc4d2d007d1603b0d52bcfa94b6842a7
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 3f67f9e..cfcdf4c 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -415,9 +415,8 @@
static Thread* DecodeThread(ScopedObjectAccessUnchecked& soa, JDWP::ObjectId thread_id,
JDWP::JdwpError* error)
- EXCLUSIVE_LOCKS_REQUIRED(Locks::thread_list_lock_)
- LOCKS_EXCLUDED(Locks::thread_suspend_count_lock_)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+ LOCKS_EXCLUDED(Locks::thread_list_lock_, Locks::thread_suspend_count_lock_) {
mirror::Object* thread_peer = Dbg::GetObjectRegistry()->Get<mirror::Object*>(thread_id, error);
if (thread_peer == nullptr) {
// This isn't even an object.
@@ -432,6 +431,7 @@
return nullptr;
}
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
Thread* thread = Thread::FromManagedThread(soa, thread_peer);
// If thread is null then this a java.lang.Thread without a Thread*. Must be a un-started or a
// zombie.
@@ -864,17 +864,13 @@
};
ScopedObjectAccessUnchecked soa(Thread::Current());
- Thread* thread;
- {
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- JDWP::JdwpError error;
- thread = DecodeThread(soa, thread_id, &error);
- if (thread == nullptr) {
- return error;
- }
- if (!IsSuspendedForDebugger(soa, thread)) {
- return JDWP::ERR_THREAD_NOT_SUSPENDED;
- }
+ JDWP::JdwpError error;
+ Thread* thread = DecodeThread(soa, thread_id, &error);
+ if (thread == nullptr) {
+ return error;
+ }
+ if (!IsSuspendedForDebugger(soa, thread)) {
+ return JDWP::ERR_THREAD_NOT_SUSPENDED;
}
std::unique_ptr<Context> context(Context::Create());
OwnedMonitorVisitor visitor(thread, context.get(), monitors, stack_depths);
@@ -884,21 +880,17 @@
JDWP::JdwpError Dbg::GetContendedMonitor(JDWP::ObjectId thread_id,
JDWP::ObjectId* contended_monitor) {
- mirror::Object* contended_monitor_obj;
ScopedObjectAccessUnchecked soa(Thread::Current());
*contended_monitor = 0;
- {
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- JDWP::JdwpError error;
- Thread* thread = DecodeThread(soa, thread_id, &error);
- if (thread == nullptr) {
- return error;
- }
- if (!IsSuspendedForDebugger(soa, thread)) {
- return JDWP::ERR_THREAD_NOT_SUSPENDED;
- }
- contended_monitor_obj = Monitor::GetContendedMonitor(thread);
+ JDWP::JdwpError error;
+ Thread* thread = DecodeThread(soa, thread_id, &error);
+ if (thread == nullptr) {
+ return error;
}
+ if (!IsSuspendedForDebugger(soa, thread)) {
+ return JDWP::ERR_THREAD_NOT_SUSPENDED;
+ }
+ mirror::Object* contended_monitor_obj = Monitor::GetContendedMonitor(thread);
// Add() requires the thread_list_lock_ not held to avoid the lock
// level violation.
*contended_monitor = gRegistry->Add(contended_monitor_obj);
@@ -1381,7 +1373,9 @@
}
void Dbg::SetJdwpLocation(JDWP::JdwpLocation* location, mirror::ArtMethod* m, uint32_t dex_pc)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+ LOCKS_EXCLUDED(Locks::thread_list_lock_,
+ Locks::thread_suspend_count_lock_) {
if (m == nullptr) {
memset(location, 0, sizeof(*location));
} else {
@@ -1872,7 +1866,6 @@
JDWP::JdwpError Dbg::GetThreadName(JDWP::ObjectId thread_id, std::string* name) {
ScopedObjectAccessUnchecked soa(Thread::Current());
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
Thread* thread = DecodeThread(soa, thread_id, &error);
UNUSED(thread);
@@ -1902,11 +1895,8 @@
}
ScopedAssertNoThreadSuspension ants(soa.Self(), "Debugger: GetThreadGroup");
// Okay, so it's an object, but is it actually a thread?
- {
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- Thread* thread = DecodeThread(soa, thread_id, &error);
- UNUSED(thread);
- }
+ Thread* thread = DecodeThread(soa, thread_id, &error);
+ UNUSED(thread);
if (error == JDWP::ERR_THREAD_NOT_ALIVE) {
// Zombie threads are in the null group.
expandBufAddObjectId(pReply, JDWP::ObjectId(0));
@@ -2094,7 +2084,6 @@
*pSuspendStatus = JDWP::SUSPEND_STATUS_NOT_SUSPENDED;
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
Thread* thread = DecodeThread(soa, thread_id, &error);
if (error != JDWP::ERR_NONE) {
@@ -2115,7 +2104,6 @@
JDWP::JdwpError Dbg::GetThreadDebugSuspendCount(JDWP::ObjectId thread_id, JDWP::ExpandBuf* pReply) {
ScopedObjectAccess soa(Thread::Current());
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
Thread* thread = DecodeThread(soa, thread_id, &error);
if (error != JDWP::ERR_NONE) {
@@ -2128,7 +2116,6 @@
JDWP::JdwpError Dbg::Interrupt(JDWP::ObjectId thread_id) {
ScopedObjectAccess soa(Thread::Current());
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
Thread* thread = DecodeThread(soa, thread_id, &error);
if (error != JDWP::ERR_NONE) {
@@ -2207,7 +2194,6 @@
JDWP::JdwpError Dbg::GetThreadFrameCount(JDWP::ObjectId thread_id, size_t* result) {
ScopedObjectAccess soa(Thread::Current());
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
*result = 0;
Thread* thread = DecodeThread(soa, thread_id, &error);
@@ -2233,9 +2219,7 @@
expandBufAdd4BE(buf_, frame_count_);
}
- // TODO: Enable annotalysis. We know lock is held in constructor, but abstraction confuses
- // annotalysis.
- virtual bool VisitFrame() NO_THREAD_SAFETY_ANALYSIS {
+ bool VisitFrame() OVERRIDE SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
if (GetMethod()->IsRuntimeMethod()) {
return true; // The debugger can't do anything useful with a frame that has no Method*.
}
@@ -2262,7 +2246,6 @@
};
ScopedObjectAccessUnchecked soa(Thread::Current());
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
Thread* thread = DecodeThread(soa, thread_id, &error);
if (error != JDWP::ERR_NONE) {
@@ -2369,17 +2352,13 @@
JDWP::JdwpError Dbg::GetThisObject(JDWP::ObjectId thread_id, JDWP::FrameId frame_id,
JDWP::ObjectId* result) {
ScopedObjectAccessUnchecked soa(Thread::Current());
- Thread* thread;
- {
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- JDWP::JdwpError error;
- thread = DecodeThread(soa, thread_id, &error);
- if (error != JDWP::ERR_NONE) {
- return error;
- }
- if (!IsSuspendedForDebugger(soa, thread)) {
- return JDWP::ERR_THREAD_NOT_SUSPENDED;
- }
+ JDWP::JdwpError error;
+ Thread* thread = DecodeThread(soa, thread_id, &error);
+ if (error != JDWP::ERR_NONE) {
+ return error;
+ }
+ if (!IsSuspendedForDebugger(soa, thread)) {
+ return JDWP::ERR_THREAD_NOT_SUSPENDED;
}
std::unique_ptr<Context> context(Context::Create());
GetThisVisitor visitor(thread, context.get(), frame_id);
@@ -2426,17 +2405,13 @@
JDWP::FrameId frame_id = request->ReadFrameId();
ScopedObjectAccessUnchecked soa(Thread::Current());
- Thread* thread;
- {
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- JDWP::JdwpError error;
- thread = DecodeThread(soa, thread_id, &error);
- if (error != JDWP::ERR_NONE) {
- return error;
- }
- if (!IsSuspendedForDebugger(soa, thread)) {
- return JDWP::ERR_THREAD_NOT_SUSPENDED;
- }
+ JDWP::JdwpError error;
+ Thread* thread = DecodeThread(soa, thread_id, &error);
+ if (error != JDWP::ERR_NONE) {
+ return error;
+ }
+ if (!IsSuspendedForDebugger(soa, thread)) {
+ return JDWP::ERR_THREAD_NOT_SUSPENDED;
}
// Find the frame with the given frame_id.
std::unique_ptr<Context> context(Context::Create());
@@ -2457,7 +2432,7 @@
size_t width = Dbg::GetTagWidth(reqSigByte);
uint8_t* ptr = expandBufAddSpace(pReply, width + 1);
- JDWP::JdwpError error = Dbg::GetLocalValue(visitor, soa, slot, reqSigByte, ptr, width);
+ error = Dbg::GetLocalValue(visitor, soa, slot, reqSigByte, ptr, width);
if (error != JDWP::ERR_NONE) {
return error;
}
@@ -2601,17 +2576,13 @@
JDWP::FrameId frame_id = request->ReadFrameId();
ScopedObjectAccessUnchecked soa(Thread::Current());
- Thread* thread;
- {
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- JDWP::JdwpError error;
- thread = DecodeThread(soa, thread_id, &error);
- if (error != JDWP::ERR_NONE) {
- return error;
- }
- if (!IsSuspendedForDebugger(soa, thread)) {
- return JDWP::ERR_THREAD_NOT_SUSPENDED;
- }
+ JDWP::JdwpError error;
+ Thread* thread = DecodeThread(soa, thread_id, &error);
+ if (error != JDWP::ERR_NONE) {
+ return error;
+ }
+ if (!IsSuspendedForDebugger(soa, thread)) {
+ return JDWP::ERR_THREAD_NOT_SUSPENDED;
}
// Find the frame with the given frame_id.
std::unique_ptr<Context> context(Context::Create());
@@ -2630,7 +2601,7 @@
uint64_t value = request->ReadValue(width);
VLOG(jdwp) << " --> slot " << slot << " " << sigByte << " " << value;
- JDWP::JdwpError error = Dbg::SetLocalValue(visitor, slot, sigByte, value, width);
+ error = Dbg::SetLocalValue(visitor, slot, sigByte, value, width);
if (error != JDWP::ERR_NONE) {
return error;
}
@@ -3477,10 +3448,7 @@
self_suspend_(false),
other_suspend_(false) {
ScopedObjectAccessUnchecked soa(self);
- {
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- thread_ = DecodeThread(soa, thread_id, &error_);
- }
+ thread_ = DecodeThread(soa, thread_id, &error_);
if (error_ == JDWP::ERR_NONE) {
if (thread_ == soa.Self()) {
self_suspend_ = true;
@@ -3650,7 +3618,6 @@
void Dbg::UnconfigureStep(JDWP::ObjectId thread_id) {
ScopedObjectAccessUnchecked soa(Thread::Current());
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
Thread* thread = DecodeThread(soa, thread_id, &error);
if (error == JDWP::ERR_NONE) {
@@ -3700,7 +3667,6 @@
Thread* self = Thread::Current();
{
ScopedObjectAccessUnchecked soa(self);
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
JDWP::JdwpError error;
targetThread = DecodeThread(soa, thread_id, &error);
if (error != JDWP::ERR_NONE) {
diff --git a/runtime/jdwp/object_registry.cc b/runtime/jdwp/object_registry.cc
index 99a005d..a42a58f 100644
--- a/runtime/jdwp/object_registry.cc
+++ b/runtime/jdwp/object_registry.cc
@@ -50,6 +50,10 @@
Thread* const self = Thread::Current();
self->AssertNoPendingException();
+ // Object::IdentityHashCode may cause these locks to be held so check we do not already
+ // hold them.
+ Locks::thread_list_lock_->AssertNotHeld(self);
+ Locks::thread_suspend_count_lock_->AssertNotHeld(self);
StackHandleScope<1> hs(self);
Handle<mirror::Object> obj_h(hs.NewHandle(o));
diff --git a/runtime/jdwp/object_registry.h b/runtime/jdwp/object_registry.h
index 0693f33..27a4e55 100644
--- a/runtime/jdwp/object_registry.h
+++ b/runtime/jdwp/object_registry.h
@@ -62,9 +62,13 @@
ObjectRegistry();
JDWP::ObjectId Add(mirror::Object* o)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) LOCKS_EXCLUDED(Locks::thread_list_lock_);
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+ LOCKS_EXCLUDED(Locks::thread_list_lock_,
+ Locks::thread_suspend_count_lock_);
JDWP::RefTypeId AddRefType(mirror::Class* c)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) LOCKS_EXCLUDED(Locks::thread_list_lock_);
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+ LOCKS_EXCLUDED(Locks::thread_list_lock_,
+ Locks::thread_suspend_count_lock_);
template<typename T> T Get(JDWP::ObjectId id, JDWP::JdwpError* error)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
@@ -96,7 +100,9 @@
private:
JDWP::ObjectId InternalAdd(mirror::Object* o)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
- LOCKS_EXCLUDED(lock_, Locks::thread_list_lock_);
+ LOCKS_EXCLUDED(lock_,
+ Locks::thread_list_lock_,
+ Locks::thread_suspend_count_lock_);
mirror::Object* InternalGet(JDWP::ObjectId id, JDWP::JdwpError* error)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
diff --git a/runtime/mirror/object.h b/runtime/mirror/object.h
index cfc8549..e44a40a 100644
--- a/runtime/mirror/object.h
+++ b/runtime/mirror/object.h
@@ -111,7 +111,10 @@
Object* Clone(Thread* self) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- int32_t IdentityHashCode() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+ int32_t IdentityHashCode() const
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+ LOCKS_EXCLUDED(Locks::thread_list_lock_,
+ Locks::thread_suspend_count_lock_);
static MemberOffset MonitorOffset() {
return OFFSET_OF_OBJECT_MEMBER(Object, monitor_);