8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM #24236

plummercj · 2025-03-25T20:36:28Z

Calling ThreadGroupReference.groups() from an event handler can cause a deadlock. Details in first comment. Tested with :jdk_lang on all supported platforms and tier1, tier2, tier3, and tier5 svc testing.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM (Bug - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
Jaikiran Pai (@jaikiran - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24236/head:pull/24236
$ git checkout pull/24236

Update a local copy of the PR:
$ git checkout pull/24236
$ git pull https://git.openjdk.org/jdk.git pull/24236/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24236

View PR using the GUI difftool:
$ git pr show -t 24236

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24236.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-25T20:37:13Z

👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-25T20:38:26Z

@plummercj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM

Reviewed-by: alanb, jpai, sspitsyn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 273 new commits pushed to the master branch:

15d36ee: 8353330: Test runtime/cds/appcds/SignedJar.java fails in CDSHeapVerifier
e6fe249: 8323100: com/sun/tools/attach/StartManagementAgent.java failed with "WaitForSingleObject failed"
096e70d: 8352437: Support --add-exports with -XX:+AOTClassLinking
6970cf6: 8352775: JVM crashes with -XX:AOTMode=create -XX:+UseZGC
afcad8c: 5043343: FileImageInputStream and FileImageOutputStream do not properly track streamPos for RandomAccessFile
6891490: 8353324: Clean up of comments and import after 8319192
07fd666: 8342984: Bump minimum boot jdk to JDK 24
6a46d55: 8353129: CDS ArchiveRelocation tests fail after JDK-8325132
19eabaf: 8353227: JFR: Prepare tests for strong parser validation
564066d: 8353118: Deprecate the use of java.locale.useOldISOCodes system property
... and 263 more: https://git.openjdk.org/jdk/compare/a347ecdedc098bd23598ba6acf28d77db01be066...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-03-25T20:39:14Z

@plummercj The following labels will be automatically applied to this pull request:

core-libs
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

plummercj · 2025-03-25T22:39:23Z

The reason is because this JDI API eventually ends up with JVMTI doing a java upcall to ThreadGroup.subgroupsAsArray(), which can trigger class loading the first time it is called. Thjis results in a ClassPrepareEvent that the debugger will get, but not process because its event handler thread is blocked on the ThreadGroupReference.groups(), so we have a deadlock.

The workaround is to make ThreadGroupReference.groups() not trigger any class loading. The fix is subtle. Before the fix, the java stack trace at the time of the ClasPrepareEvent looks like:

java.util.Arrays.copyOf(java.lang.Object[], int, java.lang.Class) bci:18 line:3513
java.util.ArrayList.toArray(java.lang.Object[]) bci:21 line:401
java.lang.ThreadGroup.subgroupsAsArray() bci:10 line:751

This is ThreadGroup.subgroupsAsArray:

private ThreadGroup[] subgroupsAsArray() {
List groups = synchronizedSubgroups();
return groups.toArray(new ThreadGroup[0]);
}

This is ArrayList.toArray():

public T[] toArray(T[] a) {
if (a.length < size)
// Make a new array of a's runtime type, but my contents:
return (T[]) Arrays.copyOf(elementData, size, a.getClass()); <--- Line 401
System.arraycopy(elementData, 0, a, 0, size);
if (a.length > size)
a[size] = null;
return a;
}

And this is Arrays.copyOf():

public static <T,U> T[] copyOf(U[] original, int newLength, Class<? extends T[]> newType) {
@SuppressWarnings("unchecked")
T[] copy = ((Object)newType == (Object)Object[].class)
? (T[]) new Object[newLength]
: (T[]) Array.newInstance(newType.getComponentType(), newLength);
System.arraycopy(original, 0, copy, 0,
Math.min(original.length, newLength));
return copy;
}

Line 3513 is actually the return statement, so this seems incorrect of by a bit. Adding some tracing of ClassPrepareEvents shows that we are currently handling the following:

cbClassPrepare: java.lang.reflect.Array

So it looks like we took the Array.newInstance() path, which triggered class loading of java.lang.reflect.Array. After the fix we never end up in Arrays.copyOf(). Instead ArrayList.toArray() calls System.arraycopy() directly, avoiding any class loading..

mlbridge · 2025-03-26T00:04:44Z

Webrevs

plummercj · 2025-03-26T00:06:40Z

Sorry, I used the wrong bugID for this PR

plummercj · 2025-03-26T00:08:42Z

Ok, bugID is correct now.

jaikiran · 2025-03-26T01:57:41Z

src/java.base/share/classes/java/lang/ThreadGroup.java

@@ -661,7 +661,7 @@ private List<ThreadGroup> synchronizedSubgroups() {
     */
    private ThreadGroup[] subgroupsAsArray() {
        List<ThreadGroup> groups = synchronizedSubgroups();
-        return groups.toArray(new ThreadGroup[0]);
+        return groups.toArray(new ThreadGroup[groups.size()]);


Hello Chris, would a comment on this line be necessary to prevent future (IDE encouraged [1]) refactoring of this code back to toArray(new ThreadGroup[0])?

[1]

What would you think about using the simple code + loop from the JBS comment? That would remove any discussion about toArray and any concerns that it would load classes.

Regarding adding a comment, I wasn't too sure what the comment should say because once you start down that path, it's hard to not end up with too much detail, and then things just get very wordy. Since we have a test that will fail if this is ever modified to cause classloading again, at least that should help prevent undoing this fix. However, the one downside of the test is that I can only get it to fail with non-debug builds, so it may not be caught early.

Regarding using the loop, that's probably a more bullet proof option, although there is still no guarantee that someday someone won't look at it and say "I can just use toArray() here". There's probably no escaping an AI code cleaning bot coming to that conclusion sometime in the near future. Then we're back to adding a comment to make sure it isn't changed.

I just noticed that this subgroupsAsArray() private method is only used by JVMTI and a comment exists about that on that method:

/** * Returns a snapshot of the subgroups as an array, used by JVMTI. */

So, irrespective of what solution we choose, perhaps we could add another brief sentence to that comment, something like the following?

/** * Returns a snapshot of the subgroups as an array, used by JVMTI. * Care should be taken not to trigger any classloading in this method. */

I switched to the code that does the array copy inline rather than rely on toArray(). I also added a comment. I intentionally kept is short and simple. I was tempted to say something about toArray(), reference the CR, mention JDI and the debug agent, etc, but that's heading down the "too wordy" path that I wanted to avoid. Readers can "git blame" to find out why it was done.

AlanBateman · 2025-03-26T18:58:59Z

src/java.base/share/classes/java/lang/ThreadGroup.java

@@ -658,10 +658,16 @@ private List<ThreadGroup> synchronizedSubgroups() {

    /**
     * Returns a snapshot of the subgroups as an array, used by JVMTI.
+     * WARNING: Make sure this API does not trigger any class loading.


Might be better to say "method" rather than "API" as this is all internal.

AlanBateman · 2025-03-26T19:01:13Z

test/jdk/com/sun/jdi/EarlyThreadGroupChildrenTest.java

+
+public class EarlyThreadGroupChildrenTest extends TestScaffold {
+    ClassType targetClass;
+    ThreadReference mainThread;


Are these used? Maybe copied over from another test?

Yes, copied from another test. I'll remove them.

AlanBateman · 2025-03-26T19:11:01Z

test/jdk/com/sun/jdi/EarlyThreadGroupChildrenTest.java

+    @Override
+    public void classPrepared(ClassPrepareEvent event) {
+        try {
+            ++classPreparedCount;


It's a long time since I looked at at test based on TestScaffold. Would I be correct to say that it creates an event handler thread to remove events from the event queue and dispatch them to the registered listeners? Only asking as it's not immediately clear if the increment of the event count is correct. I think TestScaffold uses one thread but would need to dig into the test infra to be sure.

Yes, one EventHandler thread dispatching to all listeners. More details below:

There is an interface called TargetListener which declares all the event callbacks. TargetAdapter implements TargetListener and provides and empty implementation for each callback. TestScaffold extends TargetAdapter, and each test extends TestScaffold. So every test has default empty implementations of all the callbacks, and can override some (there are a couple of overrides in this tests).

A TargetListener instance gets events by adding itself as a listener using TestScaffold.addListener(). You can see that this test is doing that early on so it gets events.

In addition, some of the TestScaffold APIs will create an instance of a TargetAdapter with one or more event callback overrides, and register that listener. For example, see waitForRequestedEvent(). So it is possible to have 2 or more listeners registered, although as I just pointed out in JDK-8352759, that can be problematic at times.

TestScaffold always has an EventHandler thread active (and only one). It sits in a loop calling EventQueue.remove(). For any each EventSet received it will iterate over each Event in the EventSet, and for each registered listener, it will call the listener's event callback for that Event.

BTW, classPreparedCount is somewhat unneeded legacy code. Previously I had limited it to 50 events to avoid the VMDisconnected exception, but I got rid of the count check and instead now catch the exception.

Thanks for the expanded comment, it just means that classPreparedCount doesn't need to be volatile, it's only accessed by one thread. For this test it doesn't matter of course.

jaikiran

Thank you Chris for adding the code comment. The source change and the code comment look good to me.

coleenp · 2025-03-27T11:07:31Z

src/java.base/share/classes/java/lang/ThreadGroup.java

@@ -658,10 +658,16 @@ private List<ThreadGroup> synchronizedSubgroups() {

    /**
     * Returns a snapshot of the subgroups as an array, used by JVMTI.
+     * WARNING: Make sure this method does not trigger any class loading.


Can you say why briefly here? Because it's called during thread startup and may deadlock with ClassLoad events. I see the comment in the test but not sure if someone will find it, except through git blame, which might be enough.

I was trying to avoid getting too much into the weeds with the explanation. There's always that one extra bit of information that would be useful, but after adding that then there's another, and you need to decide when to stop. How about the following (and I really don't want to go any further than this):

* WARNING: Make sure this method does not trigger any class loading, * because a ClassPrepareEvent can deadlock the debugger and debug agent.

sspitsyn

The fix looks okay to me. I guess new test fails with a deadlock without your fix.

plummercj · 2025-03-27T21:44:03Z

The fix looks okay to me. I guess new test fails with a deadlock without your fix.

Yes. That is the failure mode. There is no detecting the failure other than a timeout.

jaikiran

Still looks good.

AlanBateman · 2025-03-29T08:55:43Z

src/java.base/share/classes/java/lang/ThreadGroup.java

@@ -658,10 +658,17 @@ private List<ThreadGroup> synchronizedSubgroups() {

    /**
     * Returns a snapshot of the subgroups as an array, used by JVMTI.
+     * WARNING: Make sure this method does not trigger any class loading,
+     * because a ClassPrepareEvent can deadlock the debugger and debug agent.


The comment is okay but if you are doing any further edits then you could change this to say that a callback for the ClassPrepare event can deadlock (the event is "ClassPrepare" in the JVM TI spec, I realize the "ClassPrepareEvent" may have come from JDI).

I'd rather not use "callback" here. Even the JVMTI spec mostly documents in terms of events, not callbacks. For example, for the ClassLoad callback it says "For most purposes the ClassPrepare event will be more useful". Maybe as a compromise I can change "ClassPrepareEvent" to "ClassPrepare event".

plummercj · 2025-04-02T17:04:10Z

Thanks reviews Sergei, Jai, and Alan

/integrate

openjdk · 2025-04-02T17:04:40Z

Going to push as commit cc870d4.
Since your change was applied there have been 287 commits pushed to the master branch:

d979bd8: 8344671: Few JFR streaming tests fail with application not alive error on MacOS 15
49cb7aa: 8339114: DaCapo xalan performance with -XX:+UseObjectMonitorTable
d32ff13: 8353117: Crash: assert(id >= ThreadIdentifier::initial() && id < ThreadIdentifier::current()) failed: must be reasonable)
a0677d9: 8353263: Parallel: Remove locking in PSOldGen::resize
8608b16: 8348887: Create IR framework test for JDK-8347997
23eb648: 8353545: Improve debug info for StartOptionTest
4f97c4c: 8349211: Add support for intrusive trees to the utilities red-black tree
c9baa8a: 8352418: Add verification code to check that the associated loop nodes of useless Template Assertion Predicates are dead
b80b04d: 8353329: Small memory leak when create GrowableArray with initial size 0
4a50778: 8353458: Don't pass -Wno-format-nonliteral to CFLAGS
... and 277 more: https://git.openjdk.org/jdk/compare/a347ecdedc098bd23598ba6acf28d77db01be066...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-02T17:04:47Z

@plummercj Pushed as commit cc870d4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Avoid class loading in ThreadGroup.subgroupsAsArray()

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

4c65325

openjdk bot changed the title ~~8352773~~ 8352773: JVMTI should disable events during java upcalls Mar 25, 2025

openjdk bot added serviceability core-libs labels Mar 25, 2025

plummercj added 3 commits March 25, 2025 15:07

fix jcheck errors

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

8d9917a

fix more jcheck errors

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

be21380

get rid of unused breakpoint event code

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

18a255c

plummercj marked this pull request as ready for review March 26, 2025 00:00

openjdk bot added the rfr label Mar 26, 2025

plummercj changed the title ~~8352773: JVMTI should disable events during java upcalls~~ 8352088 Mar 26, 2025

openjdk bot changed the title ~~8352088~~ 8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM Mar 26, 2025

jaikiran reviewed Mar 26, 2025

View reviewed changes

Use explicit array copying code. Better comment.

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

3bc43a2

AlanBateman approved these changes Mar 26, 2025

View reviewed changes

openjdk bot added the ready label Mar 26, 2025

minor comment fix.

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

ea44034

openjdk bot removed the ready label Mar 26, 2025

remove unused locals.

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

42fd32e

jaikiran approved these changes Mar 27, 2025

View reviewed changes

openjdk bot added the ready label Mar 27, 2025

AlanBateman approved these changes Mar 27, 2025

View reviewed changes

coleenp reviewed Mar 27, 2025

View reviewed changes

sspitsyn approved these changes Mar 27, 2025

View reviewed changes

update comment.

Verified

This commit was signed with the committer’s verified signature.

minborg Per-Ake Minborg

GPG key ID: 556C11200043FB1B

Verified
Learn about vigilant mode

3d24174

openjdk bot removed ready rfr labels Mar 28, 2025

undo change that wasn't suppose to be committed.

977ecf1

openjdk bot added the rfr label Mar 28, 2025

sspitsyn approved these changes Mar 29, 2025

View reviewed changes

openjdk bot added the ready label Mar 29, 2025

jaikiran approved these changes Mar 29, 2025

View reviewed changes

AlanBateman reviewed Mar 29, 2025

View reviewed changes

minor comment update

80f75c6

openjdk bot removed the ready label Apr 1, 2025

sspitsyn approved these changes Apr 1, 2025

View reviewed changes

openjdk bot added the ready label Apr 1, 2025

jaikiran approved these changes Apr 2, 2025

View reviewed changes

AlanBateman approved these changes Apr 2, 2025

View reviewed changes

openjdk bot added the integrated label Apr 2, 2025

openjdk bot closed this Apr 2, 2025

openjdk bot removed ready rfr labels Apr 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM #24236

8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM #24236

plummercj commented Mar 25, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025 •

edited

Loading

openjdk bot commented Mar 25, 2025

plummercj commented Mar 25, 2025

mlbridge bot commented Mar 26, 2025 •

edited

Loading

plummercj commented Mar 26, 2025

plummercj commented Mar 26, 2025

jaikiran Mar 26, 2025

AlanBateman Mar 26, 2025

plummercj Mar 26, 2025

jaikiran Mar 26, 2025

plummercj Mar 26, 2025

AlanBateman Mar 26, 2025

AlanBateman Mar 26, 2025

plummercj Mar 26, 2025

AlanBateman Mar 26, 2025 •

edited

Loading

plummercj Mar 26, 2025

plummercj Mar 26, 2025

AlanBateman Mar 27, 2025

jaikiran left a comment

coleenp Mar 27, 2025

plummercj Mar 27, 2025

sspitsyn left a comment

plummercj commented Mar 27, 2025

jaikiran left a comment

AlanBateman Mar 29, 2025

plummercj Mar 31, 2025

plummercj commented Apr 2, 2025

openjdk bot commented Apr 2, 2025

openjdk bot commented Apr 2, 2025

8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM #24236

8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM #24236

Conversation

plummercj commented Mar 25, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025 • edited Loading

openjdk bot commented Mar 25, 2025

plummercj commented Mar 25, 2025

mlbridge bot commented Mar 26, 2025 • edited Loading

Webrevs

plummercj commented Mar 26, 2025

plummercj commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

plummercj commented Mar 27, 2025

jaikiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plummercj commented Apr 2, 2025

openjdk bot commented Apr 2, 2025

openjdk bot commented Apr 2, 2025

plummercj commented Mar 25, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 25, 2025 •

edited

Loading

mlbridge bot commented Mar 26, 2025 •

edited

Loading

AlanBateman Mar 26, 2025 •

edited

Loading