Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[harold seigel]

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