Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-6880972

examine JVM/TI uses of Threads::number_of_threads() == 0 & SafepointSynchronize::is_at_safepoint()



    • Subcomponent:
    • CPU:
    • OS:


      This issue came up during the code review of the fix for:

          6419370 4/4 new jmethodID code has tiny holes in synchronization

      since the comments were about existing code, I filed this new bug.

      JVM/TI's uses of Threads::number_of_threads() need to be closely
      examined and verified to be valid. Consider this use:


        2933 // rmonitor - pre-checked for validity
        2934 jvmtiError
        2935 JvmtiEnv::RawMonitorEnter(JvmtiRawMonitor * rmonitor) {
        2936 if (Threads::number_of_threads() == 0) {
        2937 // No JavaThreads exist so ObjectMonitor enter cannot be
        2938 // used, add this raw monitor to the pending list.
        2939 // The pending monitors will be actually entered when
        2940 // the VM is setup.
        2941 // See transition_pending_raw_monitors in create_vm()
        2942 // in thread.cpp.
        2943 JvmtiPendingMonitors::enter(rmonitor);
        2944 } else {
        2945 int r;
        2946 Thread* thread = Thread::current();

      RawMonitors are built on top of ObjectMonitors and thus cannot be
      used before we have JavaThreads so the above use is generally
      correct. The only part I wonder about is proper coordination
      between agent thread(s) adding to JvmtiPendingMonitors and the
      create Java VM thread cleaning up JvmtiPendingMonitors after
      the main JavaThread has been started. More investigation is
      needed there.

      Now consider this usage:


         293 // prefix_count - pre-checked to be greater than or equal to 0
         294 // prefixes - pre-checked for NULL
         295 jvmtiError
         296 JvmtiEnv::SetNativeMethodPrefixes(jint prefix_count, char** prefixes) {
         297 // Have to grab JVMTI thread state lock to be sure that some thread
         298 // isn't accessing the prefixes at the same time we are setting them.
         299 // No locks during VM bring-up.
         300 if (Threads::number_of_threads() == 0) {
         301 return set_native_method_prefixes(prefix_count, prefixes);
         302 } else {
         303 MutexLocker mu(JvmtiThreadState_lock);
         304 return set_native_method_prefixes(prefix_count, prefixes);
         305 }
         306 } /* end SetNativeMethodPrefixes */

      The comment on line 299 explains why we don't grab the
      JvmtiThreadState_lock, but I'm not sure that it is right.
      Consider src/share/vm/runtime/mutexLocker.hpp:

      extern Mutex* JvmtiThreadState_lock; // a lock on modification of JVMTI thread data

      A Mutex object is different than a JvmtiRawMonitor/ObjectMonitor
      object so I think we need to figure out if there is a real reason
      for not grabbing various Mutex objects in JVM/TI.

      And finally consider this usage:


         438 char**
         439 JvmtiExport::get_all_native_method_prefixes(int* count_ptr) {
         440 // Have to grab JVMTI thread state lock to be sure environment doesn't 441 // go away while we iterate them. No locks during VM bring-up.
         442 if (Threads::number_of_threads() == 0 || SafepointSynchronize::is_at_safepoint()) {
         443 return JvmtiEnvBase::get_all_native_method_prefixes(count_ptr);
         444 } else {
         445 MutexLocker mu(JvmtiThreadState_lock);
         446 return JvmtiEnvBase::get_all_native_method_prefixes(count_ptr);
         447 }
         448 }

      The "Threads::number_of_threads() == 0" check has the same concerns
      as mentioned before. The SafepointSynchronize::is_at_safepoint()
      check allows the VMThread to call the function without trying to
      acquire the Mutex. However, because that check does not also verify
      that the caller is the VMThread there is concern that another thread
      could be permitted unlocked access. If there are other threads that
      could be using this unlocked code path during a safepoint, then there
      is also concern that the system could leave the safepoint so we'd
      have unlocked access racing with locked access. More investigation
      is needed here also.


          Issue Links



              Unassigned Unassigned
              dcubed Daniel Daugherty
              0 Vote for this issue
              4 Start watching this issue