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

[Dmitry Samersoff]

Harold,

Please, add space after comma at 356 and 362

$0.2
-Dmitry


On 2016-09-19 20:07, harold seigel wrote:
> 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 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.