RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context

[Daniel D.Daugherty]

On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

> A part of this issue was contributed with the following changeset:
> 
> commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2
> Author: Daniel D. Daugherty <[dcubed at openjdk.org](mailto:dcubed at openjdk.org)>
> Date: Mon Nov 8 14:45:04 2021 +0000
> 
>     8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
>     Reviewed-by: coleenp, sspitsyn, dholmes, rehn
> 
> The following change in `src/hotspot/share/runtime/thread.cpp` added new assert:
> 
> bool JavaThread::java_suspend() {
> - ThreadsListHandle tlh;
> - if (!tlh.includes(this)) {
> - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on ThreadsList, no suspension", p2i(this));
> - return false;
> - }
> + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true),
>  + "missing ThreadsListHandle in calling context.");
>   return this->handshake_state()->suspend();
> }
> 
> This new assert misses a check for target thread as being current `JavaThread`.
> 
> Also, the JVMTI SuspendThread is protected with TLH:
> 
> JvmtiEnv::SuspendThread(jthread thread) {
>   JavaThread* current = JavaThread::current();
>   ThreadsListHandle tlh(current);              <= TLS defined here!!!
> 
>    oop thread_oop = NULL;
>    {
>      JvmtiVTMSTransitionDisabler disabler(true); 
> 
> 
> However, it is possible that a new carrier thread (and an associated `JavaThread`) can be created after the `TLH` was set and the target virtual thread can be mounted on new carrier thread. Then target virtual thread will be associated with newly created `JavaThread` which is unprotected by the TLH.
> The right way to be protected from this situation it is to prevent mount state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as in the change below:
> 
> 
> @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, jthread** threads_ptr) {
>  jvmtiError
>  JvmtiEnv::SuspendThread(jthread thread) {
>    JavaThread* current = JavaThread::current();
> -  ThreadsListHandle tlh(current);
> 
>    jvmtiError err;
>    JavaThread* java_thread = NULL;
>    oop thread_oop = NULL;
>    {
>      JvmtiVTMSTransitionDisabler disabler(true);
> +    ThreadsListHandle tlh(current);
> 
>      err = get_threadOop_and_JavaThread(tlh.list(), thread, &java_thread, &thread_oop);
>      if (err != JVMTI_ERROR_NONE) {
> 
> 
> 
> This problem exist in all JVMTI Suspend functions:
>  `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`.

Before Loom, there was a TLH in the outer JVM/TI SuspendThread() entry point
so the current thread was already protected. Here's the relevant code from
build/macosx-x86_64-normal-server-release/hotspot/variant-server/gensrc/jvmtifiles/jvmtiEnter.cpp
in JDK18:


static jvmtiError JNICALL
jvmti_SuspendThread(jvmtiEnv* env,
            jthread thread) {

#if !INCLUDE_JVMTI 
  return JVMTI_ERROR_NOT_AVAILABLE; 
#else 
  if(!JvmtiEnv::is_vm_live()) {
    return JVMTI_ERROR_WRONG_PHASE;
  }
  Thread* this_thread = Thread::current_or_null(); 
  if (this_thread == NULL || !this_thread->is_Java_thread()) {
    return JVMTI_ERROR_UNATTACHED_THREAD;
  }
  JavaThread* current_thread = JavaThread::cast(this_thread);
  MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, current_thread));
  ThreadInVMfromNative __tiv(current_thread);
  VM_ENTRY_BASE(jvmtiError, jvmti_SuspendThread , current_thread)
  debug_only(VMNativeEntryWrapper __vew;)
  PreserveExceptionMark __em(this_thread);
  JvmtiEnv* jvmti_env = JvmtiEnv::JvmtiEnv_from_jvmti_env(env);
  if (!jvmti_env->is_valid()) {
    return JVMTI_ERROR_INVALID_ENVIRONMENT;
  }

  if (jvmti_env->get_capabilities()->can_suspend == 0) {
    return JVMTI_ERROR_MUST_POSSESS_CAPABILITY;
  }
  jvmtiError err;
  JavaThread* java_thread = NULL;
  ThreadsListHandle tlh(this_thread);
  if (thread == NULL) {
    java_thread = current_thread;
  } else {
    err = JvmtiExport::cv_external_thread_to_JavaThread(tlh.list(), thread, &java_thread, NULL);
    if (err != JVMTI_ERROR_NONE) {
      return err;
    }
  }
  err = jvmti_env->SuspendThread(java_thread);
  return err;
#endif // INCLUDE_JVMTI
}


so it was definitely my intent that the current thread be protected
by the TLH that was in the entry code. Yes, that protection is not
needed, but that's the way I implemented it when
is_JavaThread_protected_by_TLH() was added to the system.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8878