RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri May 29 07:42:57 UTC 2020 

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:
> Hi Serguei,
>
> On 28/05/2020 3:12 pm, serguei.spitsyn at oracle.com wrote:
>> Hi David,
>>
>> I've updated the CSR and webrev in place.
>>
>> The changes are:
>>   - addressed David's suggestion to rephrase StopThread description 
>> change
>>   - replaced JVMTI_ERROR_INVALID_OBJECT with 
>> JVMTI_ERROR_ILLEGAL_ARGUMENT
>>   - updated the implementation in jvmtiEnv.cpp to return 
>> JVMTI_ERROR_ILLEGAL_ARGUMENT
>>   - updated one of the nsk.jvmti StopThread tests to check error case 
>> with the JVMTI_ERROR_ILLEGAL_ARGUMENT
>>
>>
>> I'm reposting the links for convenience.
>>
>> Enhancement:
>>    https://bugs.openjdk.java.net/browse/JDK-8234882
>>
>> CSR draft:
>>    https://bugs.openjdk.java.net/browse/JDK-8245853
>
> Spec updates are good - thanks.

Thank you for the CSR review.

>> Webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 
>>
>
> src/hotspot/share/prims/jvmtiEnv.cpp
>
> The ThreadDeath check is fine but I'm a bit confused about the 
> additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I 
> can't see how resolve_external_guard can return NULL when not passed 
> in NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT 
> rather than JVMTI_ERROR_NULL_POINTER. And I note 
> JVMTI_ERROR_NULL_POINTER is not even a listed error for StopThread! 
> This part of the change seems unrelated to this issue.

I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes for a 
jobject parameter.

Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error

You can find a link to this section at the start of the Error section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

My understanding (not sure, it is right) is that NULL has to be reported 
with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has to be 
reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this error code 
returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.

> test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java 
>
>
> The copyright year should be change to "2018, 2020,".
Thank you for the catch.
I planned to update the copyright comments.

> I'm a little surprised the test doesn't actually check that a valid 
> call doesn't produce an error. But that's an existing quirk of the 
> test and not something you need to address here (if indeed it needs 
> addressing - perhaps there is another test for that).

There are plenty of other nsk.jvmti tests which check valid calls.

Thanks,
Serguei
>
> Thanks,
> David
> -----
>
>>
>> Updated JVM TI StopThread spec:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 
>>
>>
>>
>> The old webrev and spec are here:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.0/
>>
>>
>> Thanks,
>> Serguei
>>
>> On 5/27/20 18:03, serguei.spitsyn at oracle.com wrote:
>>> Hi David,
>>>
>>>
>>> On 5/27/20 02:00, David Holmes wrote:
>>>> On 27/05/2020 6:36 pm, serguei.spitsyn at oracle.com wrote:
>>>>> Hi David,
>>>>>
>>>>>
>>>>> On 5/27/20 00:47, David Holmes wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> On 27/05/2020 1:01 pm, serguei.spitsyn at oracle.com wrote:
>>>>>>> Please, review a fix for:
>>>>>>>    https://bugs.openjdk.java.net/browse/JDK-8234882
>>>>>>>
>>>>>>> CSR draft (one CSR reviewer is needed before finalizing it):
>>>>>>>    https://bugs.openjdk.java.net/browse/JDK-8245853
>>>>>>
>>>>>> I have some thoughts on the wording which I will add to the CSR.
>>>>>
>>>>> Thank you a lot for looking at this!
>>>>>
>>>>>> Also on reflection I think JVMTI_ERROR_ILLEGAL_ARGUMENT would the 
>>>>>> best error to use, and it has an equivalent in JDWP and at the 
>>>>>> Java level for JDI.
>>>>>
>>>>> This is an interesting variant, thanks!
>>>>> We need to balance on several criteria:
>>>>>   1) Compatibility: keep returning error as close as possible to 
>>>>> the current spec
>>>>
>>>> If you are adding a new error condition I don't understand what you 
>>>> mean by "close to the current spec" ??
>>>
>>> If the JVMTI_ERROR_INVALID_OBJECT is returned than the JDWP agent 
>>> does not need any new error handling.
>>> The same can be true in the JDI if the JDWP returns the same error 
>>> as it returned before.
>>> In this case we do not add new error code but extend the existing to 
>>> cover new error condition.
>>>
>>> But, in fact (especially, after rethinking), I do not like the 
>>> JVMTI_ERROR_INVALID_OBJECT
>>> error code as it normally means something different.
>>> So, let's avoid using it and skip this criteria.
>>> Then we need new error code to cover new error condition.
>>>
>>>>>   2) Best error naming match between JVM TI and JDI/JDWP
>>>>>   3) Best practice in errors naming
>>>>
>>>> If the argument is not a ThreadDeath instance then it is an illegal 
>>>> argument - perfect fit semantically all the specs involved have an 
>>>> "illegal argument" error form.
>>>
>>> I agree with this.
>>> It is why I like this suggestion. :)
>>> The JDWP equivalent is: ILLEGAL_ARGUMENT.
>>> The JDI equivalent is:  IllegalArgumentException
>>>
>>> I'll prepare and send the update.
>>>
>>> Thanks!
>>> Serguei
>>>
>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> I think the #1 is most important but will look at it once more.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 
>>>>>>>
>>>>>>>
>>>>>>> Updated JVM TI StopThread spec:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 
>>>>>>>
>>>>>>>
>>>>>>> Summary:
>>>>>>>
>>>>>>>    The JVM TI StopThread method mirrored the functionality of the
>>>>>>>    java.lang.Thread::stop(Throwable t) method, in that it allows 
>>>>>>> any exception
>>>>>>>    type to be installed as an asynchronous exception in the 
>>>>>>> target thread.
>>>>>>>    However, the java.lang.Thread::stop(Throwable t) method was 
>>>>>>> inherently unsafe
>>>>>>>    and in Java 8 (under JDK-7059085) it was "retired" so that it 
>>>>>>> always threw
>>>>>>>    UnsupportedOperationException.
>>>>>>>    The updated JVM TI StopThread spec disallows an arbitrary 
>>>>>>> Throwable from being passed,
>>>>>>>    and instead restricts the argument to being an instance of 
>>>>>>> ThreadDeath, thus
>>>>>>>    mirroring the (deprecated but still functional) 
>>>>>>> java.lang.Thread::stop() method.
>>>>>>>    The error JVMTI_ERROR_INVALID_OBJECT is returned if the 
>>>>>>> exception argument
>>>>>>>    is not an instance of ThreadDeath.
>>>>>>>
>>>>>>>    Also, I will file similar RFE and CSR on the JDI and JDWP spec.
>>>>>>>
>>>>>>>
>>>>>>> Testing:
>>>>>>>    Built docs and checked the doc has been generated as expected.
>>>>>>>    Will run the nsk.jvmti tests locally.
>>>>>>>    Will submit hs-tiers1-3 to make sure there are no regressions 
>>>>>>> in the JVM TI and JDI tests.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>
>>>
>>

More information about the serviceability-dev mailing list