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

Tue Sep 20 01:42:10 UTC 2016

I'm missing something here. Why are you considering it an error to find 
the method in a super-interface?

InvokeMethod Command (3)
Invokes a static method. The method must be member of the class type or 
one of its superclasses, superinterfaces, or implemented interfaces. 
Access control is not enforced; for example, private methods can be invoked.

???

David

On 19/09/2016 10:51 PM, 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.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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>
>