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

Mon Sep 19 19:19:19 UTC 2016

Hi Dan,

Thanks for the review!

For now, I'll leave out the blanks following the commas.  If people feel 
that the blanks are important then I will enter an RFE to add blanks 
after all the commas in the JNI calls.

Thanks, Harold

On 9/19/2016 2:05 PM, Daniel D. Daugherty wrote:
> Just FYI. The prevailing style in that file is for JNI calls to not have
> a space after that comma. I don't like it either, but that appears to be
> how the original code was written...
>
> Dan
>
>
> On 9/19/16 11:46 AM, harold seigel wrote:
>> Will do.
>>
>> Thanks! Harold
>>
>>
>> On 9/19/2016 1:26 PM, Dmitry Samersoff wrote:
>>> 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
>>>
>>
>