RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
David Holmes
david.holmes at oracle.com
Wed Sep 21 06:37:36 UTC 2016
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 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