8277444: Data race between JvmtiClassFileReconstituter::copy_bytecodes and class linking #26863

eastig · 2025-08-20T15:36:14Z

There is a race between JvmtiClassFileReconstituter::copy_bytecodes and InstanceKlass::link_class_impl. InstanceKlass::link_class_impl can be rewriting bytecodes. JvmtiClassFileReconstituter::copy_bytecodes will not restore them to the original ones because the flag rewritten is false. This will result in invalid bytecode.

This PR adds linking a class before the copy_bytecodes method is called.
The PR also adds a regression test.

Tested fastdebug and release builds: Linux x86_64 and arm64

The reproducer from JDK-8277444 passed.
The regression test passed.
Tier1 - tier3 passed.

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-8277444: Data race between JvmtiClassFileReconstituter::copy_bytecodes and class linking (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Alex Menkov (@alexmenkov - Reviewer) Review applies to 293d81ff
Coleen Phillimore (@coleenp - Reviewer) Review applies to 05207c6c

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26863

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…lass linking

bridgekeeper · 2025-08-20T15:37:21Z

👋 Welcome back eastigeevich! 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-08-20T15:38:45Z

@eastig 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:

8277444: Data race between JvmtiClassFileReconstituter::copy_bytecodes and class linking

Reviewed-by: dholmes, amenkov, coleenp

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 136 new commits pushed to the master branch:

0aee7bf: 8367048: RISC-V: Correct pipeline descriptions of the architecture
4ec63e8: 8366850: Test com/sun/jdi/JdbStopInNotificationThreadTest.java failed
81a1e8e: 8364936: Shenandoah: Switch nmethod entry barriers to conc_instruction_and_data_patch
... and 133 more: https://git.openjdk.org/jdk/compare/d4ce630cea267e746f7feb5124fe2ecd39d7e13a...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-08-20T15:40:13Z

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

hotspot
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.

eastig · 2025-08-20T15:43:03Z

Hi @coleenp,
Could you please take a look?

mlbridge · 2025-08-20T15:46:43Z

Webrevs

dholmes-ora · 2025-08-21T00:02:54Z

@eastig I am not sure about this one. Can you clarify please how you can be transforming a class that has not yet been linked? If this is possible then it seems to me we are missing a call to ensure linkage.

eastig · 2025-08-21T23:34:13Z

@eastig I am not sure about this one. Can you clarify please how you can be transforming a class that has not yet been linked? If this is possible then it seems to me we are missing a call to ensure linkage.

Hi @dholmes-ora,

I checked what was happening.

The reproducer from JDK-8277444 simplified:

Thread 1 does:

bigClass = Class.forName();
consumer.accept(bigClass); // Puts bigClass in QUEUE
final Object o = bigClass.getConstructor().newInstance();
System.out.println(o.hashCode());

Thread 2 does:

final Class<?> aClass = QUEUE.take();
Instrumentation.retransformClasses(aClass);

Class.forName does not link bigClass. So an unlinked class is put in a queue. The linking process starts when we start using bigClass.
Thread 2 gets unlinked bigClass from the queue. It can request to retransform before we start using it and the linking process starts.

So we can have the linking process and the retransforming process running in parallel. There is no synchronization between them. We get a race condition. bigClass is big enough to make the linking process running long.

I think Class.forName does not do linking intentionally, for performance reasons.

I hope I've got everything correctly from logs and sources.

dholmes-ora · 2025-08-22T06:42:48Z

@eastig Thank you very much for that detailed analysis. An interesting scenario. I still find it somewhat suspect that we can transform an unlinked class and wonder if we should instead ensure it is linked first, rather than trying to coordinate the two pieces of code via the use of the init_lock. ?

eastig · 2025-08-22T16:29:58Z

@dholmes-ora

According to

they allow flexibility in an implementation of the linking process. If I am correct, we have a "lazy" linkage strategy in Hotspot. I don't think we want to change anything here.

copy_bytecodes is only used by JVMTI:

JvmtiEnv::RetransformClasses
JvmtiEnv::GetBytecodes

I still find it somewhat suspect that we can transform an unlinked class and wonder if we should instead ensure it is linked first,

JvmtiEnv::RetransformClasses does not need a class to be linked. It needs the initial class file bytes which are the bytes passed to ClassLoader.defineClass or RedefineClasses.
JvmtiEnv::GetBytecodes returns the bytecodes that implement the method. It's not clear from the JVMTI specification whether the returned bytecodes must be the initial class bytes. The current implementation returns the initial bytecodes the same used in JvmtiEnv::RetransformClasses.

As we don't keep the initial bytecodes (do we?), copy_bytecodes cannot be called during linking, linking changes bytecodes. It can only be called for a class in the unlinked and linked states. When copy_bytecodes is called for a linked class, it restores bytecodes to the initial ones. Linking cannot be done whilst copy_bytecodes is working.

If we use init_lock, copy_bytecodes will never see a class in the linking state. Linking will never see copy_bytecodes. As linking is blocked while we are copying bytecodes, the linking time will increase. This might have negative performance impact. Retrasforming obsoletes an original version of a method, if a new version of the method is installed. So we might not notice the performance impact.

What other options do we have which don't require many changes? We have two mutually exclusive processes.

dean-long · 2025-08-22T20:28:08Z

This seems like the correct fix. Forcing these APIs to link the class first would be a change in behavior and I assume it could cause linking-related exceptions to be thrown that the client might not expect.

dholmes-ora · 2025-08-25T05:48:43Z

This seems like the correct fix. Forcing these APIs to link the class first would be a change in behavior and I assume it could cause linking-related exceptions to be thrown that the client might not expect.

@dean-long retransformClasses is specified to to be able to throw LinkageErrors.

dholmes-ora · 2025-08-25T06:43:50Z

Class.forName does not link bigClass.

Just to be clear Class.forName(name, false, loader) would not initialize the class; the default form would initialize and thus link the class. The Class.forName spec does not state whether or not it actually performs full linking in the absence of initialization, though the hotspot implementation does not. Note that the preparation phase of linking must have occurred to get the Class object.

The retransformation API's are somewhat vague about the exact state of a class to be retransformed - the class must be "modifiable" but that seems to be treated as a static rather than dynamic property ie. primitives, arrays, hidden classes are not modifiable - any other type of class instance is.

Bottom line is that I don't think any of the specifications help us here, so we need to look at implementation practicalities.

The linking code, uses the init_lock and assumes it cannot be interfered with in any way. I don't know either pieces of code well enough to say whether we could devise a lock-free protocol that would handle the current case; or whether we could use a VM mutex around the critical part of the linking code?

I don't like JVM TI having to know anything about the init_lock and I don't like seeing another place where we acquire the init_lock via an ObjectLocker, as that does not play nicely with the work to avoid pinning for virtual threads.

I'd like to hear from others more knowledgeable than myself in this area, unfortunately @coleenp is on vacation and won't be back till next week.

dholmes-ora · 2025-08-25T20:23:52Z

src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp

+  // Method bytecodes can be rewritten during linking.
+  // Whilst the linking process rewriting bytescodes,
+  // is_rewritten() returns false. So we won't restore the original bytecodes.
+  // We hold a lock to guarantee we are not getting bytecodes
+  // at the same time the linking process are rewriting them.


Suggested change

// Method bytecodes can be rewritten during linking.

// Whilst the linking process rewriting bytescodes,

// is_rewritten() returns false. So we won't restore the original bytecodes.

// We hold a lock to guarantee we are not getting bytecodes

// at the same time the linking process are rewriting them.

// We acquire the init_lock monitor to serialize with class linking so we are not getting

// bytecodes at the same time the linking process is rewriting them.

dholmes-ora

I've heard from Coleen and she seems okay with this approach; and Patricio doesn't think it will have an impact on the virtual thread work - so that eases my concerns. I have some minor requested changes whilst we wait for a serviceability review.

Thanks

dholmes-ora · 2025-08-25T20:25:42Z

src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp

+  Handle h_init_lock(Thread::current(), mh->method_holder()->init_lock());
+  ObjectLocker ol(h_init_lock, JavaThread::current());


Suggested change

Handle h_init_lock(Thread::current(), mh->method_holder()->init_lock());

ObjectLocker ol(h_init_lock, JavaThread::current());

JavaThread* current = JavaThread::current();

Handle h_init_lock(current, mh->method_holder()->init_lock());

ObjectLocker ol(h_init_lock, current);

dean-long · 2025-08-25T21:11:34Z

@dean-long retransformClasses is specified to to be able to throw LinkageErrors.

I see that in java.lang.instrument, but the JVMTI spec says "In response to this call, no events other than the ClassFileLoadHook event will be sent." Normally during linking we send the ClassPrepare event.

dholmes-ora · 2025-08-26T04:50:37Z

@dean-long retransformClasses is specified to to be able to throw LinkageErrors.

I see that in java.lang.instrument, but the JVMTI spec says "In response to this call, no events other than the ClassFileLoadHook event will be sent." Normally during linking we send the ClassPrepare event.

To have a Class object it must have already undergone the "preparation" part of linking (it is done at the end of loading when we create the class mirror).

eastig · 2025-08-26T13:35:24Z

@dholmes-ora Thank you for review and the suggestions.

dean-long · 2025-08-26T20:53:57Z

(I realize this is a tangent, but maybe there is a separate bug here...)

To have a Class object it must have already undergone the "preparation" part of linking (it is done at the end of loading when we create the class mirror).

If that's what "preparation" means, and Class.forName(name, false, loader) does not link the class, then posting the event here in InstanceKlass::link_class_impl() instead of earlier seems misplaced:

jdk/src/hotspot/share/oops/instanceKlass.cpp

Line 1064 in c755345

JvmtiExport::post_class_prepare(THREAD, this);

Class.forName(name, false, loader) would result in a prepared class but without the event being sent.

alexmenkov

The fix looks good from serviceability perspective

dholmes-ora · 2025-08-27T06:37:44Z

If that's what "preparation" means, and Class.forName(name, false, loader) does not link the class, then posting the event here in InstanceKlass::link_class_impl() instead of earlier seems misplaced

@dean-long it seems the JVM TI "prepare" event is somewhat misnamed. It states "At this point, class fields, methods, and implemented interfaces are available, and no code from the class has been executed." but that neither describes preparation, nor the rest of linking. You can only access fields and methods after a class has been initialized! But lets take this elsewhere if needed.

eastig · 2025-08-27T11:17:50Z

MacOS test failures are known:

eastig · 2025-08-27T11:20:45Z

Hi @dholmes-ora,
Are there any other action items/changes required?

jaikiran · 2025-08-27T11:28:26Z

MacOS test failures are known

The virtual thread test that is failing in this PR's GitHub actions job is java/lang/Thread/virtual/stress/GetStackTraceALotWhenBlocking#id0 It has this:

javatestOS=Mac OS X 14.7.6 (aarch64)
javatestVersion=6.0-ea+b24-2025-08-18-${BUILT_FROM_COMMIT}
jtregVersion=jtreg 7.5.2 dev 0
...
#section:main
----------messages:(10/424)----------
command: main GetStackTraceALotWhenBlocking 100000
reason: User specified action: run main/othervm/timeout=300 GetStackTraceALotWhenBlocking 100000 
started: Tue Aug 26 14:24:40 UTC 2025
Mode: othervm [/othervm specified]
Additional options from @modules: --add-modules jdk.management
Process id: 21219
Timeout information:
--- Timeout information end.
finished: Tue Aug 26 14:47:22 UTC 2025
elapsed time (seconds): 1361.83
----------configuration:(3/42)----------
Boot Layer
  add modules: jdk.management

----------System.out:(1190/56017)----------
2025-08-26T14:24:41.601528Z => 48 of 100000
...
2025-08-26T14:44:37.436452Z => 99703 of 100000
2025-08-26T14:44:38.451885Z => 99752 of 100000
2025-08-26T14:44:39.474953Z => 99787 of 100000
Timeout signalled after 1200 seconds
2025-08-26T14:44:40.477403Z => 99833 of 100000
2025-08-26T14:44:41.478976Z => 99886 of 100000
2025-08-26T14:44:42.487173Z => 99931 of 100000
2025-08-26T14:44:43.495950Z => 99975 of 100000
2025-08-26T14:44:44.055060Z => 100000 of 100000
2025-08-26T14:44:44.055248Z VirtualThread[#27]/runnable@ForkJoinPool-1-worker-3 => 3904532796 loops
2025-08-26T14:44:44.055381Z VirtualThread[#23]/runnable@ForkJoinPool-1-worker-1 => 3293339644 loops
----------System.err:(1/15)----------
STATUS:Passed.
...
test result: Error. Program `/Users/runner/work/jdk/jdk/bundles/jdk/jdk-26.jdk/Contents/Home/bin/java' timed out (timeout set to 1200000ms, elapsed time including timeout handling was 1361819ms).

It looks like it's taking long to complete and that's causing the test timeout. I recollect that this test failure was addressed some time back, so this appears to be a new occurrence. In any case, this failure doesn't look related to the changes in this PR because I see some other PRs having failed with this same issue. I'll check and file an issue later today/tomorrow (unless anyone else gets to it first).

coleenp · 2025-08-29T12:56:02Z

Does it fail with the patch? Sorry for the delay. @dholmes-ora and I have been discussing how all this works offline, but with time-zone differences, we won't have any agreement until next week. I wrote a more limited version of the patch I sent and am testing it now.

coleenp · 2025-08-29T14:07:51Z

Okay I ran the test case in the issue and I see why it wouldn't be reliable. I verified it with the new more minimalistic patch here: #26971

eastig · 2025-08-29T15:31:05Z

Okay I ran the test case in the issue and I see why it wouldn't be reliable. I verified it with the new more minimalistic patch here: #26971

Thank you for the minimalistic patch.

I now have a version of the jtreg test which fails more reliably.
The test always passes the the previous version of #26971.

BTW I have updated the title of JDK-8277444.
The issue is the data race between copy_bytecodes and class linking. So we must guarantee no data race when copy_bytecodes is used.

eastig · 2025-08-29T15:32:14Z

I'll add the test to the PR soon.

eastig · 2025-08-29T22:56:16Z

@coleenp,

I have pulled your changes.
I added a guarantee check to copy_bytecodes. IMO it's better to be overcautious to prevent incorrect uses of copy_bytes. Because of it I had to add link_class to GetBytecodes.

I don't use the macros because they rely on THREAD. It is a variable in your patch but it is usually used as a macro.

jaikiran · 2025-09-02T09:34:02Z

The virtual thread test that is failing in this PR's GitHub actions job is java/lang/Thread/virtual/stress/GetStackTraceALotWhenBlocking#id0
It looks like it's taking long to complete and that's causing the test timeout. I recollect that this test failure was addressed some time back, so this appears to be a new occurrence. In any case, this failure doesn't look related to the changes in this PR because I see some other PRs having failed with this same issue. I'll check and file an issue later today/tomorrow (unless anyone else gets to it first).

I've filed https://bugs.openjdk.org/browse/JDK-8366669 to track this failure.

coleenp

I had a couple of minor comments but otherwise looks good. Is the test now reliable? Thank you for adding a test.

coleenp · 2025-09-02T12:04:01Z

src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp

 #include "prims/jvmtiClassFileReconstituter.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/signature.hpp"
+#include "runtime/synchronizer.hpp"


You don't need this include anymore.

coleenp · 2025-09-02T12:06:57Z

src/hotspot/share/prims/jvmtiEnv.cpp

+  if (current_thread->has_pending_exception()) {
+    current_thread->clear_pending_exception();
+    return JVMTI_ERROR_INVALID_CLASS;
+  }


Can you use the pattern:

JavaThread* THREAD = current_thread; ... link_class(THREAD); if (HAS_PENDING_EXCEPTION) etc.

Switched to macros.

coleenp

This looks good now. Thank you for your patience while we found the right solution.

dholmes-ora

Generally looks good. There are a few minor nits/typos.

I'm not sure about the test, in particular the attempt to calculate MIN_LINK_TIME_MS. It is very hard to see/know that you will actually induce the desired race. But as the test doesn't actually have any explicit failure conditions, it at least won't generate false reports.

Thanks

dholmes-ora · 2025-09-04T00:44:29Z

src/hotspot/share/prims/jvmtiEnv.cpp

    InstanceKlass* ik = InstanceKlass::cast(klass);
    if (ik->get_cached_class_file_bytes() == nullptr) {
+      // Link the class to avoid races with the rewriter. This will call the verifier also
+      // on the class. Linking is done already below in VM_RedefineClasses below, but we need


Suggested change

// on the class. Linking is done already below in VM_RedefineClasses below, but we need

// on the class. Linking is also done in VM_RedefineClasses below, but we need

There are two "below"s

dholmes-ora · 2025-09-04T00:46:53Z