RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Sep 20 15:25:40 UTC 2016
On 9/20/16 8: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/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
No comments.
test/com/sun/jdi/InterfaceMethodsTest.java
L200: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)
L208: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)
So for the test cases on L202 and L210 that changed from
testInvokePos() -> testInvokeNeg(), we had test cases in
place that verified the exact opposite of what the spec says?
And the same testInvokePos() -> testInvokeNeg() changes on
L255 and L263?
L253: // the instance
nit typo: 'the' -> 'The' and L254 needs a period at the end.
Re: use of "re-defined"
I doubt this means class redefinition but it had me going for a
second. :-)
Thumbs up! Feel free to ignore the nits above and I don't need
to see another webrev if you fix those.
Dan
>
> Please also see comments in-line.
>
>
> 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