RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
David Holmes
david.holmes at oracle.com
Tue Sep 20 01:57:02 UTC 2016
Never mind ...
On 20/09/2016 11:42 AM, David Holmes wrote:
> I'm missing something here. Why are you considering it an error to find
> the method in a super-interface?
>
> InvokeMethod Command (3)
> Invokes a static method. The method must be member of the class type or
> one of its superclasses, superinterfaces, or implemented interfaces.
> Access control is not enforced; for example, private methods can be
> invoked.
>
> ???
This spec has been modified in 9 to match:
http://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.4.8
specifically:
"A class does not inherit static methods from its superinterfaces."
The most surprising thing with the spec as it stood was that it
mentioned static methods and superinterfaces in the same sentence when
static interface methods did not even exist at the time!
I find it quite unintuitive that a superinterface does not contribute to
the set of methods accessible from an instance the same way as a
superclass does. I'm assuming that was again done to allow existing
interfaces to have static methods added without affecting any
implementing classes. It makes for a smoother transition path, but you
end up with a language model with some very rough edges. :(
David
> David
>
>
> 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/
>>
>> 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