Re: RFR: 8150758: [TESTBUG] need jvmti tests for module aware agents

[serguei.spit...@oracle.com]

Hi Dmitry,


Thanks a lot for this additional test coverage and discovering new bug:
  https://bugs.openjdk.java.net/browse/JDK-8165681

The tests look pretty good to me.

A couple of minor comments.

Dots are missed in the .c files comments.


http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/test/serviceability/jvmti


 265 printf("Expecting to find '%s' class in ClassLoad events duringVM start 
phase.\n", EXPECTED_SIGNATURE);
 266 if (class_in_class_load_events_vm_start == JNI_FALSE) {
 267 throw_exc(env, "Unable to find expected class in ClassLoad events 
duringstart phase!\n");
 268 return FAILED;
 269 }
 270
 271 if (class_prepare_events_vm_start_count == 0) {
 272 throw_exc(env, "Didn't get ClassPrepare events in start 
phase!\n");
 273 return FAILED;
 274 }
 275
 276 printf("Expecting to find '%s' class in ClassPrepare events duringVM 
start phase.\n", EXPECTED_SIGNATURE);
 277 if (class_in_class_prepare_events_vm_start == JNI_FALSE) {
 278 throw_exc(env, "Unable to find expected class in ClassPrepare 
events duringstart phase!\n");
 279 return FAILED;
 280 }


   Could you, please, replace "start phase" with "early start phase"? 
   It will be more clear that the start phase mode is "early".


 288 if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
 289 throw_exc(env, "Class is found in ClassLoad events duringVM Start 
phase!\n");
 290 return FAILED;
 291 }
 292
 293 printf("Expecting that '%s' class is absent in ClassPrepare 
events.\n", EXPECTED_SIGNATURE);
 294 if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
 295 throw_exc(env, "Class is found in ClassPrepare events duringVM 
Start phase!\n");
 296 return FAILED;
 297 }

   Could you, please, replace "VM Start phase" with "normal VM start 
phase"?It will be more clear that the start phase mode is "normal". 
Thanks, Serguei On 9/19/16 05:47, Dmitry Dmitriev wrote:
Hello, Please review new tests for module aware agents. There are 3 
tests: 1) MAAClassFileLoadHook.java - verifies ClassFileLoadHook event 
with different combinations of can_generate_early_vmstart and 
can_generate_early_class_hook_events JVMTI capabilities. Expects to 
find(or not) class from java.base module in the right VM phase. 2) 
MAAClassLoadPrepare.java - verifies ClassLoad and ClassPrepare events 
with and without can_generate_early_vmstart JVMTI capability. Expects 
to find(or not) class from java.base module in the VM start phase. 3) 
MAAThreadStart.java - verifies ThreadStart event with 
can_generate_early_vmstart JVMTI capability. Expect to find events in 
the VM start phase. JBS: 
https://bugs.openjdk.java.net/browse/JDK-8150758 webrev.00: 
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/ 
 Testing: 
RBT on all platforms Thanks, Dmitry 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[David Holmes]

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
:

   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!


---

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


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. ??


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 ???


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.


Similarly:

 255 // the instance invokes the overriden form of 
"staticMethodB" from InterfaceB


overridden -> re-defined

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?


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.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 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[David Holmes]

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
:

   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 =

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[David Holmes]

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.


???

David


On 19/09/2016 10:51 PM, harold seigel wrote:

Hi,

Please review this updated webrev for fixing 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' 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[harold seigel]

Hi Dan,

Thanks for the review!

For now, I'll leave out the blanks following the commas.  If people feel 
that the blanks are important then I will enter an RFE to add blanks 
after all the commas in the JNI calls.


Thanks, Harold


On 9/19/2016 2:05 PM, Daniel D. Daugherty wrote:

Just FYI. The prevailing style in that file is for JNI calls to not have
a space after that comma. I don't like it either, but that appears to be
how the original code was written...

Dan


On 9/19/16 11:46 AM, harold seigel wrote:

Will do.

Thanks! Harold


On 9/19/2016 1:26 PM, Dmitry Samersoff wrote:

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, _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
:

 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 = 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[Daniel D. Daugherty]

Just FYI. The prevailing style in that file is for JNI calls to not have
a space after that comma. I don't like it either, but that appears to be
how the original code was written...

Dan


On 9/19/16 11:46 AM, harold seigel wrote:

Will do.

Thanks! Harold


On 9/19/2016 1:26 PM, Dmitry Samersoff wrote:

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, _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
:

 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 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[Daniel D. Daugherty]

On 9/19/16 6:51 AM, harold seigel wrote:

Hi,

Please review this updated webrev for fixing JDK-8160987 
:


   http://cr.openjdk.java.net/~hseigel/bug_8160987.2/


src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
L356: error = JVMTI_FUNC_PTR(gdata->jvmti,GetMethodDeclaringClass)
L357: (gdata->jvmti, method, _class);
When containing_class is set to a jclass, we have a JNI local
reference that needs to be managed. So on the code path that
calls invoker_requestInvoke(), we create one more JNI local
than we used to.

I poked around the JDWP code and I think we're OK because we
create the JNI local ref for the time that the new code needs
it. When the invoke code path returns from native back into
Java, then the JNI local refs are automatically cleaned up.

Would be nice if someone else sanity checked my assertion
that we're OK here... Serguei?

test/com/sun/jdi/InterfaceMethodsTest.java
No comments.

Thumbs up!

Dan





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 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[harold seigel]

Will do.

Thanks! Harold


On 9/19/2016 1:26 PM, Dmitry Samersoff wrote:

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, _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
:

 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

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[Dmitry Samersoff]

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, _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
>>> :
>>>
>>> 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 = 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[harold seigel]

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, _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 
:


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' 

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[serguei.spit...@oracle.com]

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, _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 
:


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 

Re: RFR: JDK-8166202: Tracefile gensrc cannot handle closed src dir in different location

[Tim Bell]

Erik:


The xml/xsl source files used to generate sources for trace in hotspot
have hard coded relative paths in them that make assumptions on where
the oracle closed files are located. The source files should not make
these kind of assumptions since that makes it difficult to change the
repository layout structure.

While this is mostly an oracle internal change, there are some light
changes in the open makefile too. The end result should be unchanged.

Bug: https://bugs.openjdk.java.net/browse/JDK-8166202

Webrev: http://cr.openjdk.java.net/~erikj/8166202/webrev.01/


Looks good to me.

/Tim

Re: ThreadMXBean.getThreadAllocatedBytes() allocates memory

[Bengt Rutisson]

Hi Claes and David,

Thanks for the quick responses!

Yes, a pre-allocated array is probably not the way to go. Letting the user
pass a result array is much better or just specialising the "long
getThreadAllocatedBytes(long id)" to avoid  allocations instead of re-using
the "long[] getThreadAllocatedBytes(long[] ids)" version.

As a user of the API I am not surprised that the "long[]
getThreadAllocatedBytes(long[] ids)" version needs to allocate the result
array. But since I know that "long getThreadAllocatedBytes(long id)" pretty
much just needs to read a field in the thread it surprises me that it needs
to allocate.

I'll file an RFE for 10.

Thanks again for the help with this!

Cheers,
Bengt


2016-09-19 1:45 GMT+02:00 David Holmes :

> Hi Bengt,
>
> On 19/09/2016 7:14 AM, Bengt Rutisson wrote:
>
>>
>> Hi Serviceability,
>>
>> Not sure, but I hope this is the correct list to post this on.
>>
>
> Sure is.
>
>
> I wanted to use the ThreadMXBean.getThreadAllocatedBytes() method to get
>> some information about how much memory some Java code allocated.
>>
>> When I dug into the results they didn't properly add up until I realized
>> that the call to getThreadAllocatedBytes() actually allocates memory.
>> This was a surprise to me.
>>
>> I'm attaching a small example to illustrate what I mean.
>>
>> Running the example renders this output:
>>
>> $ javac AllocMeasure.java
>> $ java AllocMeasure
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>> Bytes allocated: 48
>>
>> What I would have expected was that it would say "Bytes allocated: 0"
>> since I would like to add my own code between line 9 and 10 in the
>> example and get the value for how much memory it allocates. As it is now
>> I have to deduct the bytes that the getThreadAllocatedBytes() allocates
>> to get the correct result.
>>
>> The problem is that getThreadAllocatedBytes() is implemented this way:
>>
>> public long getThreadAllocatedBytes(long id) {
>> long[] ids = new long[1];
>> ids[0] = id;
>> final long[] sizes = getThreadAllocatedBytes(ids);
>> return sizes[0];
>> }
>>
>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/32d957185656/
>> src/java.management/share/classes/sun/management/ThreadImpl.java#l345
>>
>> I was surprised to see the "new long[1]". I realize that it is nice to
>> reuse getThreadAllocatedBytes(long []) method, but maybe a pre-allocated
>> array can be used instead of allocating a new one for each call?
>>
>
> Did you also notice this code:
>
>   protected long[] getThreadAllocatedBytes(long[] ids) {
> boolean verified = verifyThreadAllocatedMemory(ids);
>
> long[] sizes = new long[ids.length];
>
> so we're allocating  another array for the return value(s).
>
> Bit difficult to use a pre-allocated array - when would you allocate it?
> when would you release it? Would you have one per-thread or a freelist of
> them? It all gets a bit too hard to manage.
>
> A better API would allow the user to pass in the result array as well - to
> allow for array allocation and reuse outside of the region of code that is
> being measured.
>
> I know the specification for this method is kind of fuzzy, but is this
>> to be considered a bug or does it work as intended?
>>
>
> I'd call it a quality of implementation issue. It would be better if the
> allocation could be avoided, but that requires two entry points to the VM
> code. Certainly the query for a single thread should avoid the need for any
> array allocation. But I think this pattern is common throughout the
> management code.
>
> You should a file a RFE for 10.
>
> Cheers,
> David
>
>
> Thanks,
>> Bengt
>>
>

Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class

[harold seigel]

Hi,

Please review this updated webrev for fixing 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()?

RFR: 8150758: [TESTBUG] need jvmti tests for module aware agents

[Dmitry Dmitriev]

Hello,

Please review new tests for module aware agents. There are 3 tests:
1) MAAClassFileLoadHook.java - verifies ClassFileLoadHook event with 
different combinations of can_generate_early_vmstart and 
can_generate_early_class_hook_events JVMTI capabilities. Expects to 
find(or not) class from java.base module in the right VM phase.
2) MAAClassLoadPrepare.java - verifies ClassLoad and ClassPrepare events 
with and without can_generate_early_vmstart JVMTI capability. Expects to 
find(or not) class from java.base module in the VM start phase.
3) MAAThreadStart.java - verifies ThreadStart event with 
can_generate_early_vmstart JVMTI capability. Expect to find events in 
the VM start phase.


JBS: https://bugs.openjdk.java.net/browse/JDK-8150758
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/ 


Testing: RBT on all platforms

Thanks,
Dmitry

Re: RFR: JDK-8166202: Tracefile gensrc cannot handle closed src dir in different location

[Erik Gahlin]

Looks good.

Erik

The xml/xsl source files used to generate sources for trace in hotspot 
have hard coded relative paths in them that make assumptions on where 
the oracle closed files are located. The source files should not make 
these kind of assumptions since that makes it difficult to change the 
repository layout structure.


While this is mostly an oracle internal change, there are some light 
changes in the open makefile too. The end result should be unchanged.


Bug: https://bugs.openjdk.java.net/browse/JDK-8166202

Webrev: http://cr.openjdk.java.net/~erikj/8166202/webrev.01/

/Erik

Re: RFR: JDK-8166202: Tracefile gensrc cannot handle closed src dir in different location

[David Holmes]

Hi Erik,

On 17/09/2016 12:51 AM, Erik Joelsson wrote:

The xml/xsl source files used to generate sources for trace in hotspot
have hard coded relative paths in them that make assumptions on where
the oracle closed files are located. The source files should not make
these kind of assumptions since that makes it difficult to change the
repository layout structure.

While this is mostly an oracle internal change, there are some light
changes in the open makefile too. The end result should be unchanged.

Bug: https://bugs.openjdk.java.net/browse/JDK-8166202

Webrev: http://cr.openjdk.java.net/~erikj/8166202/webrev.01/


This seems okay.

Thanks,
David


/Erik