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

[harold seigel]

Hi Dan,

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.

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