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

[Daniel D. Daugherty]

On 9/15/16 2:05 PM, harold seigel wrote:
> Hi Dan,
>
> One could argue that a spec compliant JNI implementation wouldn't need 
> this change in the first place...

That's a completely different discussion. And we don't want to go down
that rat hole again... :-)


> Regardless, I'm withdrawing this change because I found that it fails 
> a com/sun/jdi JTreg test involving static methods in interfaces.

Ouch! OK a test failure for this obscure corner is unexpected... :-(

Dan


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