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

[Daniel D. Daugherty]

On 9/12/13 5:54 AM, David Holmes wrote:
> Hi Sergey,
>
> On 12/09/2013 8:34 PM, Sergey Gabdurakhmanov wrote:
>> Hello,
>>
>> Here is webrev of changes I'm about to integrate:
>>
>> webrev: http://cr.openjdk.java.net/~sgabdura/7154963/webrev.00/
>> 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.
>
> I don't think this fix is safe. If the thread has terminated and been 
> deallocated then the code:
>
> _java_thread->threadObj() != NULL && !_java_thread->is_exiting()
>
> may be using a pointer that no longer points to a valid thread object. 
> (This is unfortunately true in a number of places in the VM.)

The original fix:

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

came from me back in April of this year. The above is a coding pattern
that we use in a number of places in JVM/TI.


> Also threadObj() is never set to NULL after the thread is initialized 
> so that condition seems unnecessary if the thread was already 
> determined to be alive when the initial checks were made.

If I remember correctly, the threadObj() part of the check covers the
case when we're querying a JavaThread early in its life. I agree that
this part of the check is not necessary at this point in the code
path, but I would prefer to be paranoid.




> I would think it sufficient to only check 
> Threads::includes(_java_thread) - as if that is true then the thread 
> is alive and while it may be terminating it can't complete that action 
> as the Threads_lock is needed for Threads::remove(this), and we hold 
> the Threads_lock as this is a VM op and we are at a safepoint.

The is_exiting() check is necessary because the JVM/TI THREAD_END event
is posted just before that flag/state is set. JVM/TI operations should
not happen after a thread has marked itself as "exiting". Also, a
JavaThread is permitted to start freeing up various resources after it
has marked itself as "exiting" so we really don't want to do query ops
after that point.


> That all said, after reading the bug report (again) I'm unclear what 
> role this thread is actually playing in getCurrentContendedMonitor. If 
> it is not the current thread, what is it? The owner of the monitor 
> which we consider contended?

That particular JavaThread is the target of the operation. In other words,
we are asking that JavaThread what its current contended monitor is. The
calling JavaThread is passed down through the various internal function
calls, but the calling JavaThread is not passed as part of the original
JVM/TI GetCurrentContendedMonitor() call.

Dan


>
> David
> -----
>
>> BR,
>> Sergey
>