8361103: java_lang_Thread::async_get_stack_trace does not properly protect JavaThread

[alexmenkov]

The fix updates java_lang_Thread::async_get_stack_trace() (used by java.lang.Thread.getStackTrace() to get stack trace for platform and mounted virtual threads) to correctly use ThreadListHandle for thread protection.

Testing: tier1..5

---

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-8361103: java_lang_Thread::async_get_stack_trace does not properly protect JavaThread (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26119

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back amenkov! 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]

@alexmenkov This change is no longer ready for integration - check the PR body for details.

[openjdk]

@alexmenkov The following label will be automatically applied to this pull request:

• hotspot

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

[mlbridge]

Webrevs

• 02: Full - Incremental (04b2a383)
• 01: Full - Incremental (f4932e4c)
• 00: Full (fa20e584)

[dholmes-ora]

Generally looks good. A couple of minor nits/queries.

Thanks

[dholmes-ora]

Won't java_thread already have been set correctly by cv_internal_thread_to_JavaThread?

[alexmenkov]

No. This is for virtual thread. We need to get JavaThread from the carrier

[dholmes-ora]

Got it.

[dholmes-ora]

Actually I think we need a further check here. If we get the carrier thread directly, we have not checked that it is actually protected by the TLH - that is normally done by cv_internal_thread_to_JavaThread but that doesn't know about virtual threads and carriers! I need to check if we have to fix cv_internal_thread_to_JavaThread for the virtual thread case.

[alexmenkov]

Well, we don't have TLH protection for carrier (and looks like JVMTI also doesn't care about carrier protection).
I think it may be useful to add virtual thread support to ThreadsListHandle (and update comments in threadSMR.hpp).
What about:

  bool cv_internal_thread_to_JavaThread(jobject jthread, JavaThread ** jt_pp, oop * thread_oop_p);

+  bool cv_oop_to_JavaThread(oop thread_oop, JavaThread** jt_pp);
+  bool cv_thread_or_carrier_to_JavaThread(oop thread_oop, JavaThread** jt_pp, bool* is_virtual_p = nullptr);

Also we need a way to check if specific vthread is mounted to JavaThread (to be checked in handshake), I think it should go to javaThread.hpp/.cpp:

  inline bool is_vthread_mounted() const;
+  inline bool is_vthread_mounted(oop vthread) const;

What do you think?
Would be nice to hear @dcubed-ojdk opinion (AFAIK he implemented threadSMR stuff)

[dholmes-ora]

Looks good. Thanks

[dholmes-ora]

Got it.

[dholmes-ora]

You can leave the // terminated platform thread comment

[dholmes-ora]

I'm also unclear if this code in the handshake is sufficient for dealing with the case where we have a mounted virtual thread initially, but it is unmounted before the handshake gets to execute:

    if (java_lang_VirtualThread::is_instance(_java_thread())) {
        // if (thread->vthread() != _java_thread()) // We might be inside a System.executeOnCarrierThread
        const ContinuationEntry* ce = thread->vthread_continuation();
        if (ce == nullptr || ce->cont_oop(thread) != java_lang_VirtualThread::continuation(_java_thread())) {
          return; // not mounted
        }
      }

The commented line seems an inaccurate characterization as we could be executing a completely different virtual thread on this carrier now - which is not related to executeOnCarrierThread.

[dholmes-ora]

Revoking my approval as there may be further issues.

[alexmenkov]

I'm also unclear if this code in the handshake is sufficient for dealing with the case where we have a mounted virtual thread initially, but it is unmounted before the handshake gets to execute:

    if (java_lang_VirtualThread::is_instance(_java_thread())) {
        // if (thread->vthread() != _java_thread()) // We might be inside a System.executeOnCarrierThread
        const ContinuationEntry* ce = thread->vthread_continuation();
        if (ce == nullptr || ce->cont_oop(thread) != java_lang_VirtualThread::continuation(_java_thread())) {
          return; // not mounted
        }
      }

The commented line seems an inaccurate characterization as we could be executing a completely different virtual thread on this carrier now - which is not related to executeOnCarrierThread.

I suppose the comment is obsolete. I don't see executeOnCarrierThread in the current codebase.
The condition checks if there were some changes in vthread/carrier:
ce == nullptr - the carries has no mounted vthread now (i.e. the vthread has been unmounted);
ce->cont_oop(thread) != java_lang_VirtualThread::continuation(_java_thread()) - the carrier has other vthread mounted;
Even if there were unmount/mount the same vthread on the carrier, it's fine (we need vthread stack trace and we are in handshake with its carrier)

[AlanBateman]

I suppose the comment is obsolete. I don't see executeOnCarrierThread in the current codebase. The condition checks if there were some changes in vthread/carrier: ce == nullptr - the carries has no mounted vthread now (i.e. the vthread has been unmounted); ce->cont_oop(thread) != java_lang_VirtualThread::continuation(_java_thread()) - the carrier has other vthread mounted; Even if there were unmount/mount the same vthread on the carrier, it's fine (we need vthread stack trace and we are in handshake with its carrier)

I suspect it dates back to when there were temporary transitions and where the thread identity had to be temporarily changed to the carrier thread. We've been able to remove that complexity, which eliminates complexity from JVMTI, and maybe here too.