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

Fri Sep 16 14:37:51 UTC 2016

Going down the JNI rabbit hole is my fault. When I mumbled on 2016-09-02 
about
how this bug might be fixed, I was too focused on the fact that the JNI 
layer
didn't do what we want and I talked about how to fix it using JNI functions.

Harold, my apologies for wasting your time.

Dan

On 9/16/16 8: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.spitsyn at oracle.com wrote:
>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>