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

Mon Sep 19 17:07:51 UTC 2016

Thanks Serguei.  I'll make that change before checking in the fix.

Harold

On 9/19/2016 12:55 PM, serguei.spitsyn at oracle.com wrote:
> Hi Harold,
>
> 351 static jvmtiError check_methodClass(JNIEnv *env, jclass clazz, 
> jmethodID method)
> 352 {
> 353 jclass containing_class;
> 354 jvmtiError error;
> 355
> 356 error = JVMTI_FUNC_PTR(gdata->jvmti,GetMethodDeclaringClass)
> 357 (gdata->jvmti, method, &containing_class);
>   It is better to initialize containing_class with NULL. Otherwise, it 
> looks good. Thanks for taking care about this issue! Serguei On 
> 9/19/16 05:51, 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 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160919/febfe114/attachment.html>