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

David Holmes david.holmes at oracle.com
Fri May 29 07:59:48 UTC 2020 

On 29/05/2020 5:42 pm, serguei.spitsyn at oracle.com wrote:
> 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 

Ah I see. Some functions specify JVMTI_ERROR_NULL_POINTER explicitly.

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

Got it - thanks.

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

I'm a bit unclear here. If you pass a jweak then resolving it may yield 
a NULL if the referent has been GC'd. But IIUC a plain jobject is just a 
JNI global reference to an Object - if the jobject is not NULL then 
resolving it to an oop can also not yield a NULL.

But regardless this seems unrelated to current issue and just 
complicates matters.

Cheers,
David

> 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