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

[David Holmes]

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!

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