On Mon, 14 Feb 2022 23:11:34 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> There are scenarios that JDWP agent can deadlock on `classTrackLock` >> monitor. Following is the scenario in bug report. >> >> **Java Thread** >> - loads a class and post `JVMTI_EVENT_CLASS_PREPARE` event >> - JDWP event callback handler calls `classTrack_processUnloads()` to handle >> the event. >> - `classTrack_processUnloads()` takes `classTrackLock` lock, then tries to >> allocate a new bag under the lock. >> - bag allocation code calls` jvmtiAllocate()`, which may be blocked by >> ongoing safepoint due to state transition. >> >> If the safepoint is GC safepoint (prior to JDK16) or >> `VM_JvmtiPostObjectFree` safepoint (JDK16 or later) >> >> **VM Thread** >> - post `JVMTI_EVENT_OBJECT_FREE` >> - JDWP event callback handler calls `cbTrackingObjectFree()` to handle the >> event >> - `cbTrackingObjectFree()` tries to acquire `classTrackLock` lock, leads to >> deadlock >> >> From my research, there are three events that may be posted at safepoints, >> `JVMTI_EVENT_GARBAGE_COLLECTION_START`, >> `JVMTI_EVENT_GARBAGE_COLLECTION_FINISH` and `JVMTI_EVENT_OBJECT_FREE`, but >> only `JVMTI_EVENT_OBJECT_FREE` is relevant to JDWP agent. >> >> The solution I purpose here, is simply move allocation/deallocation code >> outside of `classTrackLock` lock. >> >> >> Test: >> - [x] tier1 >> - [x] vmTestbase_nsk_jdi >> - [x] vmTestbase_nsk_jdwp >> - [x] vmTestbase_nsk_jvmti > > src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c line 100: > >> 98: struct bag* deleted = deletedSignatures; >> 99: deletedSignatures = NULL; >> 100: debugMonitorExit(classTrackLock); > > This looks risky as the critical section is broken and the NULL deleted > signatures exposed. If `cbTrackingObjectFree` occurs while this is true then > you will lose the record of the deleted signature. > > Alternatively you could allow for lock-free reading of `deletedSignatures`, > preemptively allocate a new bad if needed then take the lock. Or even use the > lock to read `deletedSignatures` to determine if a new bag is needed, then > drop the lock, create the bag, take the lock and re-check everything. Agreed. ------------- PR: https://git.openjdk.java.net/jdk/pull/7461
