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.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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>