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

David Holmes david.holmes at oracle.com
Sat May 30 13:50:03 UTC 2020 

Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spitsyn at oracle.com wrote:
> Hi David and reviewers,
> 
> The updated webrev version is:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/
> 
> This update adds testing that StopThread can return 
> JVMTI_ERROR_INVALID_OBJECT error code.
> 
> The JVM TI StopThread spec is:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread
> 
> 
> There is a couple of comments below.
> 
> 
> On 5/29/20 06:18, David Holmes wrote:
>> On 29/05/2020 6:24 pm, serguei.spitsyn at oracle.com wrote:
>>> On 5/29/20 00:56, serguei.spitsyn at oracle.com wrote:
>>>> On 5/29/20 00:42, 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 
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Found the explanation.
>>>> The JDI file:
>>>> src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java
>>>>
>>>> has a fragment that translate the INVALID_OBJECT error to the 
>>>> ObjectCollectedException:
>>>>     RuntimeException toJDIException() {
>>>>         switch (errorCode) {
>>>>             case JDWP.Error.INVALID_OBJECT:
>>>>                 return new ObjectCollectedException();
>>>>
>>>> So, the INVALID_OBJECT is for a jobject handle that is referencing a 
>>>> collected object.
>>>> It means that previous implementation incorrectly returned 
>>>> JVMTI_ERROR_NULL_POINTER error code.
>>>
>>> I should create and delete local or global ref to construct a test 
>>> case for this.
>>>
>>> Interesting that the JDWPException::toJDIException() does not convert 
>>> the ILLEGAL_ARGUMENT error code to an IllegalArgumentException.
>>> I've just added this conversion.
>>
>> Given the definition of JDWP INVALID_OBJECT then obviously JDI 
>> converts it to ObjectCollectedException.
>>
>> So reading further in JNI spec:
>>
>> "Weak global references are a special kind of global reference. Unlike 
>> normal global references, a weak global reference allows the 
>> underlying Java object to be garbage collected. Weak global references 
>> may be used in any situation where global or local references are used."
>>
>> So it seems that any function that takes a jobject cxould in fact 
>> accept a jweak, in which case JVMTI_ERROR_INVALID_OBJECT is a 
>> possibility in all cases. So IIUC JNIHandles::resolve_external_guard 
>> can return NULL if a weak reference has been collected. So the new 
>> code you propose seems correct.
> 
> You are right about weak global references.
> I was able to construct a test case for JVMTI_ERROR_INVALID_OBJECT.
> The JNI NewGlobalRef and DeleteGlobalRef are used for it.
> You can find it in the updated webrev version.
> 
>> However, this still is unrelated to the current issue and I do not see 
>> other JVM TI doing checks for this case. So this seems to be a much 
>> broader issue.
> There are many such checks in JVM TI.
> For instance, there are checks like the following in jvmtiEnv.cpp:
> NULL_CHECK(o, JVMTI_ERROR_INVALID_OBJECT)

Yes but they are incorrect IMO e.g.

JvmtiEnv::GetObjectSize(jobject object, jlong* size_ptr) {
   oop mirror = JNIHandles::resolve_external_guard(object);
   NULL_CHECK(mirror, JVMTI_ERROR_INVALID_OBJECT);

The NULL_CHECK will fail if either object is NULL or object is a jweak 
that has been cleared. In the first case it should report 
JVMTI_ERROR_NULL_POINTER.

The correct pattern is what you proposed with this fix:

+   NULL_CHECK(exception, JVMTI_ERROR_NULL_POINTER);
     oop e = JNIHandles::resolve_external_guard(exception);
+   // the exception must be a valid jobject
+   if (e == NULL) {
+     return JVMTI_ERROR_INVALID_OBJECT;
+   }

Though not sure why you didn't use a second NULL_CHECK

David
-----

> Thanks,
> Serguei
> 
>>
>> David
>> -----
>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>>> 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