On Mon, 14 Feb 2022 14:27:45 GMT, Zhengyu Gu <z...@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 Hi Zhengyu, I don't think the fix is correct - see below. Thanks, David 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. src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c line 215: > 213: classTrack_activate(JNIEnv *env) > 214: { > 215: struct bag* new_bag = bagCreateBag(sizeof(char*), 1000); I don't think there can be any race during activation but this change is harmless. src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c line 240: > 238: if (deletedSignatures != NULL) { > 239: bagEnumerateOver(deletedSignatures, cleanDeleted, NULL); > 240: to_delete = deletedSignatures; `cleanDeleted` also calls `jvmtiDeallocate`, so either both need to be outside the lock or neither. It is really unclear to me how many threads can be involved here and which functions can be called when. ------------- Changes requested by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7461