RFR: 8248379: Handshake closures for JVMTI monitor functions lack of some validations

[serguei.spitsyn at oracle.com]

Hi Yasumasa,

Yes, you can list me as a reviewer.

Thanks,
Serguei


On 6/26/20 18:54, Yasumasa Suenaga wrote:
> Hi Serguei, David,
>
>>> If you are executing the handshake operation then you are in a 
>>> handshake with the target thread which means it must exist in some 
>>> ThreadsList.
>
> Yes, it is checked at Handshake::execute_direct().
>
> ```
> 349   ThreadsListHandle tlh;
> 350   if (tlh.includes(target)) {
> 351     target->set_handshake_operation(&op);
> 352   } else {
> 353     log_handshake_info(start_time_ns, op.name(), 0, 0, "(thread 
> dead)");
> 354     return false;
> 355   }
> ```
>
> Serguei, can I list you as a Reviewer? If so, I will push this change 
> when I got second reviewer.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/06/27 8:51, serguei.spitsyn at oracle.com wrote:
>> Hi David,
>>
>> Thank you for clarification.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 6/26/20 15:50, David Holmes wrote:
>>> Hi Serguei,
>>>
>>> On 27/06/2020 2:40 am, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>> I see, some VM_op's also have this check:
>>>>
>>>> 1546   ThreadsListHandle tlh;
>>>> 1547   if (jt != NULL && tlh.includes(jt)
>>>>
>>>>
>>>> I wonder if it make sense to add as well.
>>>
>>> If you are executing the handshake operation then you are in a 
>>> handshake with the target thread which means it must exist in some 
>>> ThreadsList.
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>> Otherwise, it looks good to me.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 6/26/20 00:03, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this change.
>>>>>
>>>>>   JBS: https://bugs.openjdk.java.net/browse/JDK-8248379
>>>>>   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8248379/webrev.00/
>>>>>
>>>>> JDK-8242425 introduces to migrate to thread local handshake from 
>>>>> VM operation for GetOwnedMonitorInfo, 
>>>>> GetOwnedMonitorStackDepthInfo, and GetCurrentContendedMonitor 
>>>>> JVMTI functions. However it lacks of validations for thread state 
>>>>> and thread oop of the target.
>>>>>
>>>>> This change has been tested on submit repo and 
>>>>> serviceability/jvmti, serviceability/jdwp vmTestbase/nsk/jvmti, 
>>>>> vmTestbase/nsk/jdi vmTestbase/nsk/jdwp.
>>>>> On submit repo, tools/javac/7118412/ShadowingTest.java and 
>>>>> java/foreign/TestMismatch.java were failed 
>>>>> (mach5-one-ysuenaga-JDK-8248379-20200626-0503-12110818). However 
>>>>> they do not seems to be related to this change.
>>>>> (Both tests have been passed on my Linux AMD64)
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>
>>