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

GetOwnedMonitorInfo() returns incorrect owned monitor

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: P4
    • Resolution: Fixed
    • Affects Version/s: 10
    • Fix Version/s: 10
    • Component/s: hotspot
    • Labels:
    • Subcomponent:
    • Resolved In Build:
      b21

      Backports

        Description

        GetOwnedMonitorInfo JVMTI function returns a monitor which is not yet owned when it is called at MonitorContendedEnter JVMTI event.

        I attached reproducer in this tichet. Please read README.md.

        GetOwnedMonitorInfo() should not return a pending monitor.

          Activity

          Hide
          dcubed Daniel Daugherty added a comment -
          Moving from hotspot/svc -> hotspot/jvmti since GetOwnedMonitorInfo
          is a JVM/TI API.
          Show
          dcubed Daniel Daugherty added a comment - Moving from hotspot/svc -> hotspot/jvmti since GetOwnedMonitorInfo is a JVM/TI API.
          Hide
          dcubed Daniel Daugherty added a comment -
          So the JVM/TI GetOwnedMonitorInfo() wrapper calls
          JvmtiEnv::GetOwnedMonitorInfo() which determines
          whether to use JvmtiEnvBase::get_owned_monitors()
          directly or via a VM operation.

          JvmtiEnvBase::get_owned_monitors() calls
          JvmtiEnvBase::get_locked_objects_in_frame() which
          has a check for a pending monitor:

              if (pending_obj == obj) {
                // the thread is pending on this monitor so it isn't really owned
                continue;
              }

          However, that check is dependent on the earlier code:

            oop pending_obj = NULL;
            {
              // save object of current enter() call (if any) for later comparison
              ObjectMonitor *mon = java_thread->current_pending_monitor();
              if (mon != NULL) {
                pending_obj = (oop)mon->object();
              }
            }


          Since the JavaThread::_current_pending_monitor field hasn't
          been set at the time that the MONITOR_CONTENDED_ENTER
          event is posted, the pending monitor logic in
          JvmtiEnvBase::get_locked_objects_in_frame() is fooled.

          The problem is that the _current_pending_monitor field setting
          is carefully placed:

            { // Change java thread status to indicate blocked on monitor enter.
              JavaThreadBlockedOnMonitorEnterState jtbmes(jt, this);

              DTRACE_MONITOR_PROBE(contended__enter, this, object(), jt);
              if (JvmtiExport::should_post_monitor_contended_enter()) {
                JvmtiExport::post_monitor_contended_enter(jt, this);

                // The current thread does not yet own the monitor and does not
                // yet appear on any queues that would get it made the successor.
                // This means that the JVMTI_EVENT_MONITOR_CONTENDED_ENTER event
                // handler cannot accidentally consume an unpark() meant for the
                // ParkEvent associated with this ObjectMonitor.
              }

              OSThreadContendState osts(Self->osthread());
              ThreadBlockInVM tbivm(jt);

              Self->set_current_pending_monitor(this);

              // TODO-FIXME: change the following for(;;) loop to straight-line code.
              for (;;) {
                jt->set_suspend_equivalent();
                // cleared by handle_special_suspend_equivalent_condition()
                // or java_suspend_self()

                EnterI(THREAD);


          The field is set after OSThreadContendState and ThreadBlockInVM
          helper creation which changes a bunch of visible state. We'll have to
          carefully examine the places that query the _current_pending_monitor
          field to make sure that code won't be confused by this value being
          set in a different thread state.
          Show
          dcubed Daniel Daugherty added a comment - So the JVM/TI GetOwnedMonitorInfo() wrapper calls JvmtiEnv::GetOwnedMonitorInfo() which determines whether to use JvmtiEnvBase::get_owned_monitors() directly or via a VM operation. JvmtiEnvBase::get_owned_monitors() calls JvmtiEnvBase::get_locked_objects_in_frame() which has a check for a pending monitor:     if (pending_obj == obj) {       // the thread is pending on this monitor so it isn't really owned       continue;     } However, that check is dependent on the earlier code:   oop pending_obj = NULL;   {     // save object of current enter() call (if any) for later comparison     ObjectMonitor *mon = java_thread->current_pending_monitor();     if (mon != NULL) {       pending_obj = (oop)mon->object();     }   } Since the JavaThread::_current_pending_monitor field hasn't been set at the time that the MONITOR_CONTENDED_ENTER event is posted, the pending monitor logic in JvmtiEnvBase::get_locked_objects_in_frame() is fooled. The problem is that the _current_pending_monitor field setting is carefully placed:   { // Change java thread status to indicate blocked on monitor enter.     JavaThreadBlockedOnMonitorEnterState jtbmes(jt, this);     DTRACE_MONITOR_PROBE(contended__enter, this, object(), jt);     if (JvmtiExport::should_post_monitor_contended_enter()) {       JvmtiExport::post_monitor_contended_enter(jt, this);       // The current thread does not yet own the monitor and does not       // yet appear on any queues that would get it made the successor.       // This means that the JVMTI_EVENT_MONITOR_CONTENDED_ENTER event       // handler cannot accidentally consume an unpark() meant for the       // ParkEvent associated with this ObjectMonitor.     }     OSThreadContendState osts(Self->osthread());     ThreadBlockInVM tbivm(jt);     Self->set_current_pending_monitor(this);     // TODO-FIXME: change the following for(;;) loop to straight-line code.     for (;;) {       jt->set_suspend_equivalent();       // cleared by handle_special_suspend_equivalent_condition()       // or java_suspend_self()       EnterI(THREAD); The field is set after OSThreadContendState and ThreadBlockInVM helper creation which changes a bunch of visible state. We'll have to carefully examine the places that query the _current_pending_monitor field to make sure that code won't be confused by this value being set in a different thread state.
          Hide
          dholmes David Holmes added a comment -
          Given JvmtiEnvBase::get_owned_monitors has:

           assert((SafepointSynchronize::is_at_safepoint() ||
                    is_thread_fully_suspended(java_thread, false, &debug_bits)),
                   "at safepoint or target thread is suspended");

          it seems to me that (as I often comment about API calls made from callbacks) there is no realization that this code might be called via a callback during the monitor acquisition process, and hence that the "pending monitor" state might not be stable.

          The suggested fix addresses the problem and I don't see any other uses of the pending monitor that might be impacted negatively.
          Show
          dholmes David Holmes added a comment - Given JvmtiEnvBase::get_owned_monitors has:  assert((SafepointSynchronize::is_at_safepoint() ||           is_thread_fully_suspended(java_thread, false, &debug_bits)),          "at safepoint or target thread is suspended"); it seems to me that (as I often comment about API calls made from callbacks) there is no realization that this code might be called via a callback during the monitor acquisition process, and hence that the "pending monitor" state might not be stable. The suggested fix addresses the problem and I don't see any other uses of the pending monitor that might be impacted negatively.
          Hide
          dcubed Daniel Daugherty added a comment -
          Copied from errant backport bug JDK-8185926:

           hgupdate HG Updates added a comment - 35 minutes ago
          URL: http://hg.openjdk.java.net/jdk10/hs/hotspot/rev/f2ec523d900b
          User: sspitsyn
          Date: 2017-08-07 20:53:58 +0000
          Show
          dcubed Daniel Daugherty added a comment - Copied from errant backport bug JDK-8185926 :  hgupdate HG Updates added a comment - 35 minutes ago URL: http://hg.openjdk.java.net/jdk10/hs/hotspot/rev/f2ec523d900b User: sspitsyn Date: 2017-08-07 20:53:58 +0000
          Hide
          sspitsyn Serguei Spitsyn added a comment -
          Dan, thank you for fixing the errant backport issue!
          Show
          sspitsyn Serguei Spitsyn added a comment - Dan, thank you for fixing the errant backport issue!
          Hide
          sspitsyn Serguei Spitsyn added a comment -
          [ @David ]
          Yes, I know it was my fault as I did not notice this bug had the 'Fix Version' 11.
          I agree, we have to pay more attention to the 'Fix Version' when pushing fixes for 10.
          Show
          sspitsyn Serguei Spitsyn added a comment - [ @David ] Yes, I know it was my fault as I did not notice this bug had the 'Fix Version' 11. I agree, we have to pay more attention to the 'Fix Version' when pushing fixes for 10.
          Hide
          hgupdate HG Updates added a comment -
          URL: http://hg.openjdk.java.net/jdk10/jdk10/hotspot/rev/f2ec523d900b
          User: jwilhelm
          Date: 2017-08-18 18:01:35 +0000
          Show
          hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk10/jdk10/hotspot/rev/f2ec523d900b User: jwilhelm Date: 2017-08-18 18:01:35 +0000

            People

            • Assignee:
              ysuenaga Yasumasa Suenaga
              Reporter:
              ysuenaga Yasumasa Suenaga
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: