RFR (S) 8229900: RedefineDeleteJmethod.java fails with -Xcheck:jni

[David Holmes]

Hi Coleen,

On 3/10/2019 8:11 am, coleen.phillimore at oracle.com wrote:
> 
> I added more checks (as discussed below) and reran tier1 with -Xcheck:jni.
> 
> http://cr.openjdk.java.net/~coleenp/2019/8229900.02/webrev/index.html

That aspect seems fine. Thanks for adding the extra checks.

David
-----

> thanks,
> Coleen
> 
> On 10/2/19 10:03 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 10/2/19 12:53 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> Sorry rather long-winded reply ...
>>>
>>> On 1/10/2019 11:52 pm, coleen.phillimore at oracle.com wrote:
>>>> Summary: Remove RedefineClasses adjustment and test, but improve 
>>>> checking for method/class matching.
>>>>
>>>> Tested with tier1 with -Xcheck:jni locally, and tier1 on Oracle 
>>>> platforms.
>>>>
>>>> open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2019/8229900.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8229900
>>>
>>> I was troubled by this change because it made me think more deeply 
>>> about what should reasonably happen with respect to jmethodIDs and 
>>> class redefinition.
>>>
>>> I had a (simplistic?) mental model that a jmethodID is a "pointer" to 
>>> a specific method in a specific class (the defining class of the 
>>> method). As such a jmethodID should only be used when operating on 
>>> that class (or subclass thereof) or an instance of that class (or 
>>> subclass thereof). With that mental model it makes sense for 
>>> Xcheck:jni to validate the defining class is the target class, or a 
>>> superclass thereof. But it doesn't then make sense for class 
>>> redefinition to update jmethodIDs as the redefined class is not the 
>>> original class! If a class is redefined then it should, in my mental 
>>> model, invalidate all existing jmethodIDs that pertained to the 
>>> original class.
>>>
>>> However, that's not very user friendly in the face of redefinition of 
>>> a superclass as code that only works with the subclass may reasonably 
>>> expect jmethodIDs to remain valid even if they refer to an inherited 
>>> method that has been redefined. So we update them to refer to the 
>>> redefined method implementation.
>>>
>>> So in that regards the update to jniCheck::validate_call_class seems 
>>> correct. Though I wonder if we also need to check that obj->class is 
>>> a subtype of clazz? As far as I can see we never actually validate 
>>> that! We use the jmethodID to find the method and we then find the 
>>> vtable index wrt. the method->holder class, and then we use that 
>>> vtable index to lookup a method in the receiver object's class - 
>>> which could lead to a random method being selected in a different class!
>>
>> I just added this check and am testing it now.
>> Coleen
>>>
>>> Continuing on, if we do expect jmethodIDs to get updated upon class 
>>> redefinition then it makes sense to me to keep the logic that handles 
>>> deleted methods, by redirecting them to a method that throws NSME. 
>>> The fact that method is in Unsafe is unfortunate but it is what we do 
>>> elsewhere in the VM. I'm assuming the problem here is that the 
>>> augmented jniCheck::validate_call_class will fail in such cases? That 
>>> is a problem, but I think I'd rather see it special-cased than change 
>>> the existing behaviour:
>>>
>>>   if (obj != NULL) {
>>>     jniCheck::validate_object(thr, obj);
>>>   }
>>> + if (m == Universe::throw_no_such_method_error())
>>> +   return;  // skip class checks in this case
>>>
>>> then the test could also remain.
>>>
>>> Also note that while we generally require JNI programmers to ensure 
>>> everything is called correctly, jmethodIDs are also used by JVM TI 
>>> and we tend to want JVM TI to have well defined semantics. I'm 
>>> unclear now what happens if we invoke a deleted method through JVM TI ?
>>>
>>> Thanks,
>>> David
>>> ----
>>>
>>>> Thanks,
>>>> Coleen
>>
>