Thanks Serguei. I'll make that change before checking in the fix.
Harold
On 9/19/2016 12:55 PM, serguei.spit...@oracle.com wrote:
Hi Harold,
351 static jvmtiError check_methodClass(JNIEnv *env, jclass clazz,
jmethodID method)
352 {
353 jclass containing_class;
354 jvmtiError error;
355
356 error = JVMTI_FUNC_PTR(gdata->jvmti,GetMethodDeclaringClass)
357 (gdata->jvmti, method, &containing_class);
It is better to initialize containing_class with NULL. Otherwise, it
looks good. Thanks for taking care about this issue! Serguei On
9/19/16 05:51, harold seigel wrote:
Hi,
Please review this updated webrev for fixing JDK-8160987
<https://bugs.openjdk.java.net/browse/JDK-8160987>:
http://cr.openjdk.java.net/~hseigel/bug_8160987.2/
It provides a more efficient implementation and fixes a test
problem. This fix was tested as described below and with the JTReg
JDK com/sun/jdi tests.
Thanks, Harold
On 9/16/2016 10:32 AM, harold seigel wrote:
Hi Serguei, Thanks for the suggestion! That provides a much cleaner
implementation. Harold On 9/15/2016 11:28 PM,
serguei.spit...@oracle.com wrote:
On 9/15/16 19:13, David Holmes wrote:
On 16/09/2016 8:52 AM, serguei.spit...@oracle.com wrote:
Hi Harold, I did not got deep into the fix yet but wonder why the
JVMTI function is
My copy-paste failed. I wanted to list the JVMTI function name:
GetMethodDeclaringClass. Thanks, Serguei
not used.
I was wondering a similar thing. It seems very heavyweight to use
Java level reflection from inside native code to validate the
native "handles" passed to that native code. I would have expected
a way to go from a MethodId to the declaring class of the method,
and a simple way to test if there is an ancestor relation between
the two classes.
On 9/15/16 13:05, harold seigel wrote:
One could argue that a spec compliant JNI implementation
wouldn't need this change in the first place... Regardless, I'm
withdrawing this change because I found that it fails a
com/sun/jdi JTreg test involving static methods in interfaces.
I find it both intriguing and worrying that ClassType.InvokeMethod
refers to superinterfaces when prior to 8 (and this spec was not
updated in this area) static interface methods did not exist! The
main changes were in the definition of InterfaceType.InvokeMethod.
I wonder whether invocation of static interface methods via
ClassType.InvokeMethod is actually tested directly? I realize the
specs are a bit of a minefield when it comes to what is required
by the different specs and what is implemented in hotspot.
Unfortunately it is a minefield I also have to wade through for
private interface methods. In many cases it is not clear what
should happen and all we have to guide us is what hotspot does (eg
"virtual" invocations on non-virtual methods). David -----
Thanks, Harold On 9/15/2016 3:37 PM, Daniel D. Daugherty wrote:
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime) Hi Dan, Thanks for looking at this. I
could pass NULL instead of clazz to ToReflectMethod() to
ensure that the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the JNI
ToReflectedMethod() function. That would be relying on the fact
that HotSpot doesn't use the clazz parameter to convert
{clazz,jmethodID} => method_object. Sorry... again... Dan
Harold On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
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
