RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
harold seigel
harold.seigel at oracle.com
Tue Sep 20 17:25:41 UTC 2016
Hi Serguei,
Thanks for the suggestions.
Harold
On 9/20/2016 1:01 PM, serguei.spitsyn at oracle.com wrote:
> Hi Harold,
>
>
> Looks good.
>
>
> http://cr.openjdk.java.net/~hseigel/bug_8160987.3/test/com/sun/jdi/InterfaceMethodsTest.java.frames.html
>
>
> 197 // invoke interface static method A
> 198 testInvokePos(ifaceClass, null, "staticMethodA", "()I", vm().mirrorOf(RESULT_A));
> 199
> 200 // invoking static method A on the instance fails because
> staticMethodA is not
> 201 // inherited by TargetClass.
> 202 testInvokeNeg(ifaceClass, ref, "staticMethodA", "()I",
> vm().mirrorOf(RESULT_A),
> 203 "Invalid MethodID");
> 204
> 205 // invoke interface static method B
> 206 testInvokePos(ifaceClass, null, "staticMethodB", "()I", vm().mirrorOf(RESULT_A));
> 207
> 208 // invoking static method B on the instance fails because
> staticMethodB is not
> 209 // inherited by TargetClass.
> 210 testInvokeNeg(ifaceClass, ref, "staticMethodB", "()I",
> vm().mirrorOf(RESULT_A),
> 211 "Invalid MethodID");
> 212
>
> I'd suggest the following changes to make it more precise and
> consistent with other fragments:
> "static method A" => "staticMethodA"
> "static method B" => "staticMethodB"
>
> Thanks,
> Serguei
>
>
>
>
> On 9/20/16 07:11, 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/
>>
>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160920/a30ebdb3/attachment-0001.html>
More information about the serviceability-dev
mailing list