Harold, Please, add space after comma at 356 and 362
$0.2 -Dmitry On 2016-09-19 20:07, harold seigel wrote: > Thanks Serguei. I'll make that change before checking in the fix. > > Harold > > > On 9/19/2016 12:55 PM, serguei.spit...@oracle.com wrote: >> Hi Harold, >> >> 351 static jvmtiError check_methodClass(JNIEnv *env, jclass clazz, >> jmethodID method) >> 352 { >> 353 jclass containing_class; >> 354 jvmtiError error; >> 355 >> 356 error = JVMTI_FUNC_PTR(gdata->jvmti,GetMethodDeclaringClass) >> 357 (gdata->jvmti, method, &containing_class); >> It is better to initialize containing_class with NULL. Otherwise, it >> looks good. Thanks for taking care about this issue! Serguei On >> 9/19/16 05:51, 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.spit...@oracle.com wrote: >>>>> On 9/15/16 19:13, David Holmes wrote: >>>>>> On 16/09/2016 8:52 AM, serguei.spit...@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 -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.