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

[David Holmes]

On 13/09/2013 2:52 AM, Daniel D. Daugherty wrote:
> 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.

That looks better :)

>
>> 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.

Ok.

>> 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.

Ah I see. Thanks for clarifying. I was only thinking about the pointer 
validity, not the semantic validity.

>> 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.

Doh! Of course.

Thanks,
David
-----


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
>>
>