JDK-8160984 : JNI CallStaticMethod don't validate class
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 9
  • Priority: P3
  • Status: Closed
  • Resolution: Not an Issue
  • Submitted: 2016-07-07
  • Updated: 2016-10-05
  • Resolved: 2016-07-20
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Resolved
Related Reports
Relates :  
Relates :  
Description
Specification of CallStatic<Type>Method family says:
"The method ID must be derived from clazz, not from one of its superclasses."

But the functions allow to pass not only the superclass declaring the type, but any arbitrary type. 

Example:
============================
public class CallStaticTest {
    private static native void nativeMethod();

    public static void m() {
        System.out.println("Static method m is called!");
    }

    public static void main(String[] args) throws NoSuchMethodException {
        System.loadLibrary("CallStaticTest");
        nativeMethod();
    }
}
============================
#include "jni.h"

JNIEXPORT void JNICALL Java_CallStaticTest_nativeMethod
  (JNIEnv * env, jclass clazz, jobject method)
{
    jmethodID methodID = (*env)->GetStaticMethodID(env, clazz, "m", "()V");
    jclass stringClass = (*env)->FindClass(env, "java/lang/String");
    (*env)->CallStaticVoidMethod(env, stringClass, methodID);
}
============================
$ javac CallStaticTest.java -d classes
$ gcc -m64 -shared -o CallStaticTest.dll -I"$jdk\include" -I'$jdk\include\win32' CallStaticTest.c
$ java -cp classes CallStaticTest
Static method m is called!
============================
Comments
Related to this ... For the Call<Type>Method* we have: "When these functions are used to call private methods and constructors, the method ID must be derived from the real class of obj, not from one of its superclasses." But we do not check that. If we did it would be impossible to call private interface methods via this API. Again the spec does not say what happens if this constraint is violated.
05-10-2016

I'm not sure the spec is fixable without breaking everything. The methodID is concrete for CallStatic and CallNonvirtual, but abstract for CallVirtual (lookup has to be performed every time). I'm inclined to just leave this "house of cards" untouched.
22-07-2016

Coleen, - On "Throws": Thanks! Let me summarize then: JNI functions may give constraints, and consequences of violation of these constraints are undefined, *unless* there is a concrete exception specified, in which case the exception is mandatory. - On j.l.r.Method: you can obtain a methodID from a Method via FromReflectedMethod(JNIEnv *env, jobject method). David, After rereading "Accessing Fields and Methods" (https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#accessing_fields_and_methods) I think that jmethodID necessarily represents a concrete method. Especially the statement "The native code can then use the method ID repeatedly without the cost of method lookup" implies that.
21-07-2016

Let me clarify my concern with a concrete example. Given class A { public static void m() { } } class B extends A { public static void m() { } // hides A.m } If we obtain the methodID from "class A" and invoke CallStaticVoidMethod passing in "Class B", what should happen based on the spec, and what actually happens based on hotspot's implementation? I think Hotspot will invoke A.m, yet to me the spec suggests execution of B.m (if the methodID is just an abstract representation for m() and "Class B" contains the concrete implementation.). The spec should not allow two different implementations to behave differently here.
21-07-2016

I think in specifying for CallStatic<type>Method for methodID: "This family of operations invokes a static method on a Java object, according to the specified method ID. The methodID argument must be obtained by calling GetStaticMethodID()." It is one way to have GetStaticMethodID to enforce the rule about clazz: "The method ID must be derived from clazz, not from one of its superclasses." But GetStaticMethodID doesn't only look for the method in the declaring class, and it's not specified that way either. How do you get a MethodID from a java.lang.reflect.Method? Lastly, It seems to me that the "Throws" part of the JNI specification is defined behavior, so yes conformance tests should check these conditions.
20-07-2016

Disclaimer: I'm not asking to add the incompatible check by default and not insisting on adding it for -Xcheck:jni. Now I'm only talking about spec clarity. About Get{Static}MethodID(): it looks like the spec is just too strict there. One may get a method ID from a j.l.r.Method or in any other way. Would be better to say "The methodID must be an ID of a static/instance method of the class" or something like that. Let me ask one more question. Can we treat any of the exceptions in the "Throws" sections as mandatory (i.e. can we enforce them in conformance tests)? E.g. are ArrayIndexOutOfBoundsException and ArrayStoreException guaranteed for SetObjectArrayElement, or the behavior is undefined in these cases as well?
20-07-2016

I agree the spec covers a wide range of programming errors - not withstanding that in some cases it does flag specific errors, or pre-supposes what kinds of exceptions an implementation might throw. Coleen: The issue with the methodID is, I think, whether (at the JNI spec level) it uniquely identifies a particular method implementation (ie points to a Method in a particular class) or is an abstract representation of the method ie just a signature/descriptor. Hotspot implements it as a Method*, but in the callVirtual case it actually has to ignore that and lookup the correct method* based on the Class of the passed in instance. If you consider it an abstract representation then you should be able to get the MethodID from any class in a hierarchy, and use it to invoke the method in a specific class, using CallNonVirtual, or CallStatic. I think if we pull on this too hard it will all unwravel.
20-07-2016

Note there is a parallel between the callStatic and callNonvirtual forms - neither of which use the Class parameter in hotspot. I think the API is perfectly correct. A methodID is an abstract representation of a method - based on signature and return type, as obtained by looking up in the context of a given class. A possible implementation strategy for methodIDs is to encode a direct function pointer, based on the class used for the lookup. The virtual calls don't use this hard-wired function pointer because they need to call the most-derived version of the method (and there is no guarantee the methodID was obtained from the most-derived class that implemented the method). For the callStatic and callNonvirtual functions, we can use this direct method pointer and avoid any kind of secondary lookup. So in our implementation the Class parameter to the call is unused - though we could/should be doing a validity check. I'm actually suspicious of a bug in hotspot in here, particularly in the callNonVirtual case. The callNonvirtual method allows you to call a super-class implementation of a method directly. Seems to me there are two ways to do this: 1. Obtain the methodID from the superclass in question 2. Obtain the methodID from the actual class of the object upon which the call is to be made If we use #2 and pass the superclass class object then we should invoke the super method - but I don't think hotspot will - hotspot only supports #1 AFAICS. But I see nothing in the spec that precludes #2 from being valid. Also see JDK-4831190: "The clazz parameter is ignored in hotspot. The class information used by hotspot is actually derived from the method id. Setting the clazz parameter to null (or any other value) will not interfere with the java program in hotspot. Other JVMs may vary. "
13-07-2016

The constraint needs to be removed because the clazz parameter is not used at all by the JNI CallStatic<Type>Method()s. We should not enforce constraints on a parameter which has no meaning or impact on its methods' functionality. The mistake was having a clazz parameter in the first place. We should not compound that mistake by enforcing constraints on it. Perhaps there may be a rare implementation that uses the clazz parameter but the possibility of such an implementation is far less than the possibility of breaking countless users whose applications currently pass unknown values for clazz to the CallStatic<Type>Method() functions. The right fix is to remove the constraints on the clazz parameters from the docs.
12-07-2016

I think the issue with the JNI spec is exactly what Dan has pointed to above - there is a constraint, but no consequences of its violation. There should be something - if not an exact exception then at least "methodID must be... but if it is not the behavior is undefined". That way both implementations that do and don't validate and/or use the parameters can coexist and be conformant. Based on the David's words about that another implementation may actually use clazz, I think the constraint itself cannot be removed - it would suddenly make such implementation illegal.
12-07-2016

JDK-8160987 should be reopened and the two API's handled separately. For JNI I see no issue here and no scope for any test that checks argument verification.
12-07-2016

On re-reading the JNI spec for CallStatic<Type>Method, I just noticed that the spec does not state what should happen if a bad clazz parameter is passed. The wording from the JNI spec on errors is: > THROWS: > > Exceptions raised during the execution of the Java method. This is unlike the spec for GetStaticMethodID which says: > THROWS: > > NoSuchMethodError: if the specified static method cannot be found. Exactly what is the new JCK test for JNI CallStatic<Type>Method going to expect for the error condition?
11-07-2016

Specification of ClassType.InvokeMethod says: "The method must be member of the class type or one of its superclasses". However, the command allows to invoke method passing an arbitrary class. Example (pseudocode): ============================ Debuggee classes: public class A { } public class B { public static void testedMethod() { } } ============================ Debugger pseudocode: threadId = suspendThread() classIdA = getClassId("A") classIdB = getClassId("B") methodId = getMethodId(classIdB, "testedMethod") ClassType.InvokeMethod(classIdA, threadId, methodId) ============================ Such debugger code invokes the method with error code NONE, but INVALID_METHODID is expected.
11-07-2016

That part of the spec seems to think that static methods are invoked as-if instance methods of the Class object - which is not the case in hotspot but is a potential implementation strategy. Because the JNI spec expects the programmer to do their own argument validation -Xcheck:jni turns on additional checking when JNI functions are used. I don't think there is a 'spec' for this, it is whatever additional checking we happen to have chosen to perform in hotspot. Arguably we could augment -Xcheck:jni to verify the correct class is passed - but we may find that programmers routinely pass NULL as they know the class is not actually needed. In general, given the way the spec is, tests for conformance should only be "positive tests".
11-07-2016

I think the question is whether there is any benefit for a user to be able to validate whether the method is declared in the class they think it is. If there is not, then maybe not only the JNI check, but also the check in JDWP's ClassType.InvokeMethod should be dropped (JDWP's one is not implemented as well, see JDK-8160987).
08-07-2016

If not for backward compatibility we would remove the jclass parameter because it is unused. I think it should be ignored by -Xcheck:jni, also. The jclass is only needed in the call to GetStaticMethodID() and it was a mistake to include it in the CallStaticMethod() API's.
08-07-2016

Are there any means for the jclass parameter other than the (unimplemented) method validation? Would removing the requirement leave it just unused? Also, what is the status of -Xcheck:jni? Is Hotspot's JNI supposed to be conformant with it? If so, it seems that it would be better to implement the check for the -Xcheck:jni mode only. Benefits 2 and 3 from the message above are still there in that case.
08-07-2016

Perhaps we should remove the requirement from the spec. That would have the following benefits: 1. No coding changes are needed to fix this 2. We won't break compatibility 3. No new JCK tests are needed
08-07-2016

I see a couple of warnings when -Xcheck:jni is used, but they seem to be unrelated: $ java -Xcheck:jni -cp classes CallStaticTest WARNING: JNI local refs: zu, exceeds capacity: zu at java.lang.System.initProperties(java.base/Native Method) at java.lang.System.initPhase1(java.base/System.java:1861) WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethod Static method m is called!
07-07-2016

JNI is famous/infamous for not validating input parameters by default as stated in the JNI spec. Is this "spec violation" detected when the '-Xcheck:jni' option is used?
07-07-2016