RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
harold seigel
harold.seigel at oracle.com
Tue Sep 20 14:12:14 UTC 2016
Hi Serguei,
Thanks for checking on this.
Harold
On 9/19/2016 10:13 PM, serguei.spitsyn at oracle.com wrote:
> On 9/19/16 11:03, Daniel D. Daugherty wrote:
>> On 9/19/16 6:51 AM, 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/
>>
>> src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
>> L356: error =
>> JVMTI_FUNC_PTR(gdata->jvmti,GetMethodDeclaringClass)
>> L357: (gdata->jvmti, method, &containing_class);
>> When containing_class is set to a jclass, we have a JNI local
>> reference that needs to be managed. So on the code path that
>> calls invoker_requestInvoke(), we create one more JNI local
>> than we used to.
>>
>> I poked around the JDWP code and I think we're OK because we
>> create the JNI local ref for the time that the new code needs
>> it. When the invoke code path returns from native back into
>> Java, then the JNI local refs are automatically cleaned up.
>>
>> Would be nice if someone else sanity checked my assertion
>> that we're OK here... Serguei?
>
>
> Dan,
>
> Thank you for checking this.
> We should be OK here as the local reference must be cleaned up upon
> return to java.
>
> Thanks,
> Serguei
>
>
>>
>> test/com/sun/jdi/InterfaceMethodsTest.java
>> No comments.
>>
>> Thumbs up!
>>
>> Dan
>>
>>
>>
>>>
>>> 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
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list