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

[coleen.phillimore at oracle.com]

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

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
>