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

Yasumasa Suenaga suenaga at oss.nttdata.com
Sat Jun 27 01:54:31 UTC 2020


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


More information about the serviceability-dev mailing list