On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP InvokeStatic()
method was depending on the JNI function that it called to enforce the
requirement that the specified method must be a member of the
specified class or one of its super classes. But, JNI does not enforce
this requirement. This fix adds code to JDWP to do its own check that
the specified method is a member of the specified class or one of its
super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This will
be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed in
the bug, the JCK Lang, VM, and API tests, the hotspot JTReg tests, the
java/lang, java/util and other JTReg tests, the co-located and
non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
