RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Sep 20 02:13:35 UTC 2016
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