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

[serguei.spitsyn at oracle.com]

On 9/15/16 19:13, David Holmes wrote:
> On 16/09/2016 8:52 AM, serguei.spitsyn at 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>