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

[coleen.phillimore at oracle.com]

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