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

[Daniel D. Daugherty]

On 9/17/13 2:48 AM, Sergey Gabdurakhmanov wrote:
> Hello,
>
> New version:
> http://cr.openjdk.java.net/~sgabdura/7154963/webrev.01/

Thumbs up!

src/share/vm/prims/jvmtiEnvBase.hpp
     No comments.

Dan


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