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

Mon Sep 19 18:05:10 UTC 2016

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