RR(XS): 7154963 crash in JvmtiEnvBase::get_current_contended_monitor()

[Daniel D. Daugherty]

On 9/12/13 4:34 AM, Sergey Gabdurakhmanov wrote:
> Hello,
>
> Here is webrev of changes I'm about to integrate:
>
> webrev: http://cr.openjdk.java.net/~sgabdura/7154963/webrev.00/

src/share/vm/prims/jvmtiEnvBase.hpp
     You have rewritten the logic from earlier versions of the fix
     without any explanation as to why. In fact, this version is
     different than the one you had me review a day or two ago.

     You have inverted the if-statement itself and I'm OK with that,
     but you have also changed the order of the checks from previous
     versions of the fix. Here's the previous if-statement:

     if (!Threads::includes(_java_thread) || _java_thread->is_exiting() ||
         _java_thread->threadObj() == NULL) {

     which gives us the checks in this order:

         !Threads::includes(_java_thread)
         _java_thread->is_exiting()
         _java_thread->threadObj()

     This latest version does the checks in this order:

         _java_thread->threadObj() != NULL
         !_java_thread->is_exiting()
         Threads::includes(_java_thread)

     The reason for doing the Threads::includes() check first is to
     verify that the JavaThread pointer is still valid. By using the
     _java_thread variable first, you run the risk of a crash.


Dan


> bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7154963
>
> The fix check that the _java_thread parameter is valid when it is 
> possible that the JavaThread has exited after the initial checks were 
> made in generated/jvmtifiles/jvmtiEnter.cpp: 
> jvmti_GetCurrentContendedMonitor().
>
> The fix checked by customer on JDK6. Crash is not reproducible with 
> this fix.
>
> BR,
> Sergey