On 10/2/19 12:53 AM, David Holmes wrote:
Hi Coleen,
Sorry rather long-winded reply ...
On 1/10/2019 11:52 pm, [email protected] 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.
Your simplistic model is correct. The jmethodID is a pointer to a
specific method in a class. In the call, we check that it applies to a
class or subclass.
The current behavior is that if the method is "obsolete", meaning that
the bytecodes are new, the methodID is not replaced:
// obsolete methods need a unique idnum so they become new entries in
// the jmethodID cache in InstanceKlass
assert(old_method->method_idnum() == new_method->method_idnum(),
"must match");
u2 num = InstanceKlass::cast(_the_class)->next_method_idnum();
if (num != ConstMethod::UNSET_IDNUM) {
old_method->set_method_idnum(num);
}
If the redefined method is the same *bytecodes* as the old method
"emcp", the methodID *is* replaced. This is good.
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.
Not sure I follow this. We only update them for emcp methods, but I
don't see why calling through a subclass is a distinction.
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!
We could add that check in a new RFE.
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 ?
JVMTI semantics should follow JNI semantics. If JVMTI uses jmethodIDs
and invoke a deleted method, it should get undefined behavior. Not
sure what you mean here either.
When looking at this change and trying to decide whether to keep this
replacement with the special cases in jniCheck.cpp above or remove it,
these things led me to decide to remove it.
1. This is JNI and we don't generally protect people from undefined
behavior. In fact, if you're using JNI, I think a crash is a better
debugging tool than a Java NSME exception, which will propagate out of
the JNI context. That would be my preference if I were a developer.
2. Adding/deleting methods in redefinition has been removed, except with
a command line switch to enable old behavior, so adding something
helpful for something deprecated seemed silly.
3. This "helpful" NSME for jmethodID was a recent addition for a
different problem. Nobody relies on this.
4. It looks like the "helpful" NSME replacement unintentionally affected
more than deleted methods, so old code might get different results (see
1 above).
5. We have so many special cases in the jvm for so many different
things, we don't need another.
Thanks,
Coleen
Thanks,
David
----
Thanks,
Coleen
