RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sun May 31 07:50:06 UTC 2020
Hi David,
Also jumping to end.
On 5/30/20 06:50, David Holmes wrote:
> 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;
> + }
>
I see your point, thanks!
I'll check these cases and file a bug if necessary.
> Though not sure why you didn't use a second NULL_CHECK
I've already replaced it with:
NULL_CHECK(e, JVMTI_ERROR_INVALID_OBJECT);
You, probably, need to refresh the webrev page.
Thanks,
Serguei
> 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