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

[Sergey Gabdurakhmanov]

Hello,

New version:
http://cr.openjdk.java.net/~sgabdura/7154963/webrev.01/

BR,
Sergey

On 13.09.2013 13:16, David Holmes wrote:
> 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
>>>
>>