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
