[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
From: harold seigel <harold.seigel () oracle ! com>
Date: 2016-09-21 12:34:20
Message-ID: 06ce0ae9-fe9e-7c8a-d524-455acab4d6ad () oracle ! com
[Download RAW message or body]
Hi David,
Thanks again. I included Sergeui's suggestions before pushing the fix.
I also created an enhancement bug to improve the test:
https://bugs.openjdk.java.net/browse/JDK-8166453
Harold
On 9/21/2016 2:37 AM, David Holmes wrote:
> Hi Harold,
>
> On 21/09/2016 12:11 AM, harold seigel wrote:
>> Hi David,
>>
>> Thanks for the review. Please review this updated webrev containing the
>> comment modifications that you suggested:
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8160987.3/
>
> No further comments - the suggestions from Sergeui seem reasonable.
>
>> Please also see comments in-line.
>
> I'll answer this here to save scrolling :)
>
> It hadn't occurred to me that reflective lookup of the method would
> fail before we even get to the invocation part of the test. To me this
> further demonstrates the confusing way this test has been written
> because we don't actually test the invocation mechanism in those
> cases! This means some things that should be tested are not. For example:
>
> 244 // "staticMethodA" must not be inherited by InterfaceB
> 245 testInvokeNeg(ifaceClass, null, "staticMethodA", "()I",
> vm().mirrorOf(RESULT_A),
> 246 "Static interface methods are not inheritable");
>
> fails because method lookup in InterfaceB fails, but what we should
> also be testing is the actual InterfaceType.invokeMethod on InterfaceB
> using the Method for staticMethodA obtained from InterfaceA. So this
> suite of tests in incomplete.
>
> It is also unclear to me if some things that should be checked at the
> JDWP level are in fact caught at the JDI level - a lot of argument
> checking/validation occurs in the JDI Java code. You might have
> written your fix in the Java code, but that would not have fixed JDWP.
> So it makes me wonder if other things are checked at the JDI level
> instead of the JDWP level!
>
> Aside: The test also seems to be missing half the invocation tests
> because for each call that ends up doing a virtual/non-virtual invoke,
> there should a corresponding non-virtual/virtual invoke.
>
> Thanks,
> David
>
>>
>> On 9/19/2016 10:50 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> I'm having a lot of issues with the code and testing here. Please bear
>>> with me.
>>>
>>> On 19/09/2016 10:51 PM, 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
>>>
>>> + // If not the same class then check that containing_class is a
>>> super type of
>>> + // clazz and not an interface (hence it's a super class).
>>>
>>> Simpler to just say:
>>>
>>> + // If not the same class then check that containing_class is a
>>> superclass of
>>> + // clazz (not a superinterface).
>>>
>>> Took me a while to notice that interfaces don't inherit static methods
>>> from superinterfaces either!
>> Done.
>>>
>>> ---
>>>
>>> test/com/sun/jdi/InterfaceMethodsTest.java
>>>
>>> The new comments are very verbose compared to other negative tests:
>>>
>>> 200 // try to invoke static method A on the instance. This
>>> should fail because ref
>>> 201 // is of type InterfacemethodsTest$TargetClass which is
>>> not a subtype of the
>>> 202 // class containing staticMethodA.
>>>
>>> Could simply be:
>>>
>>> 200 // "staticMethodA" is not inherited by TargetClass
>> Done.
>>>
>>>
>>> That aside the more I look at this test the more things I see that
>>> seem to be wrong or at the very least confused, in the existing code.
>>> First it seems that the test chooses to ignore the "class" object when
>>> given a non-null ref object - so it talks about invoking a static
>>> method on an instance, which is misleading at best as what it will
>>> actually do is take a path that tries to invoke the static method
>>> using the instances' class instead of the specified class (which may
>>> be the interface class). This makes the test descriptions and expected
>>> behaviours somewhat unintuitive in my opinion.
>>>
>>> 244 // "staticMethodA" must not be inherited by InterfaceB
>>> 245 testInvokeNeg(ifaceClass, null, "staticMethodA", "()I",
>>> vm().mirrorOf(RESULT_A),
>>> 246 "Static interface methods are not inheritable");
>>>
>>> I am wondering how this test passes without your fix? It suggests
>>> there must already exist some code that compares the declaring class
>>> with the passed in class. ??
>> This test passes without my fix because the ifaceClass passed to
>> testInvokeNeg() is InterfaceB, which does not contain a method called
>> staticMethodA. So, this code in the test's invoke() method throws an
>> exception because getMethod() returns null:
>> Method method = getMethod(targetClass, methodName, methodSig);
>> if (method == null) {
>> throw new Exception("Can't find method: " + methodName + "
>> for class = " + targetClass);
>> }
>>
>> The code containing my fix does not get reached in this test case. In
>> contrast, the test cases that originally failed with my fix passed a
>> correct ifaceClass but an incorrect ObjectReference.
>>>
>>>
>>> 248 // however it is possible to call "staticMethodA" on the
>>> actual instance
>>> 249 testInvokeNeg(ifaceClass, ref, "staticMethodA", "()I",
>>> vm().mirrorOf(RESULT_A),
>>> 250 "Static interface methods are not inheritable");
>>>
>>> The comment here suggests a successful execution when in fact it
>>> expects it to fail. It should say "nor is it possible ...". But again,
>>> how does this pass (by failing) without your fix ???
>> The comment here is obviously wrong. The test passes (by failing)
>> without my fix for the same reason as above. I fixed the comment.
>>>
>>> 252 // "staticMethodB" is overridden in InterfaceB
>>>
>>> "overridden" is the wrong word here. Static interface methods are not
>>> inherited so they can not be overridden. "re-defined" would be an
>>> accurate description.
>> Done.
>>>
>>> Similarly:
>>>
>>> 255 // the instance invokes the overriden form of
>>> "staticMethodB" from InterfaceB
>>>
>>> overridden -> re-defined
>> Will fix.
>>>
>>> 298 /* Static method calls */
>>>
>>> This starts all the static method calls on the instance class - all of
>>> which are expected to fail, and do so _without_ your fix present! How?
>> These pass (by failing) for the same reasons as the above tests. Method
>> invoke()'s call to getMethod() returns null. So, invoke() throws an
>> exception.
>>
>> Thanks, Harold
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>>> 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@oracle.com wrote:
>>>>>> On 9/15/16 19:13, David Holmes wrote:
>>>>>>> On 16/09/2016 8:52 AM, serguei.spitsyn@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
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic