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

[harold seigel]

Hi David,

Thanks again.  I included Sergeui's suggestions before pushing the fix.

I also created an enhancement bug to improve the test: 
https://bugs.openjdk.java.net/browse/JDK-8166453


Harold


On 9/21/2016 2:37 AM, David Holmes wrote:

Hi Harold,

On 21/09/2016 12:11 AM, harold seigel wrote:

Hi David,

Thanks for the review.  Please review this updated webrev containing the
comment modifications that you suggested:

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


No further comments - the suggestions from Sergeui seem reasonable.


Please also see comments in-line.


I'll answer this here to save scrolling :)

It hadn't occurred to me that reflective lookup of the method would 
fail before we even get to the invocation part of the test. To me this 
further demonstrates the confusing way this test has been written 
because we don't actually test the invocation mechanism in those 
cases! This means some things that should be tested are not. For example:


 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");

fails because method lookup in InterfaceB fails, but what we should 
also be testing is the actual InterfaceType.invokeMethod on InterfaceB 
using the Method for staticMethodA obtained from InterfaceA. So this 
suite of tests in incomplete.


It is also unclear to me if some things that should be checked at the 
JDWP level are in fact caught at the JDI level - a lot of argument 
checking/validation occurs in the JDI Java code. You might have 
written your fix in the Java code, but that would not have fixed JDWP. 
So it makes me wonder if other things are checked at the JDI level 
instead of the JDWP level!


Aside: The test also seems to be missing half the invocation tests 
because for each call that ends up doing a virtual/non-virtual invoke, 
there should a corresponding non-virtual/virtual invoke.


Thanks,
David



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
:

   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 

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

[David Holmes]

Hi Harold,

On 21/09/2016 12:11 AM, harold seigel wrote:

Hi David,

Thanks for the review.  Please review this updated webrev containing the
comment modifications that you suggested:

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


No further comments - the suggestions from Sergeui seem reasonable.


Please also see comments in-line.


I'll answer this here to save scrolling :)

It hadn't occurred to me that reflective lookup of the method would fail 
before we even get to the invocation part of the test. To me this 
further demonstrates the confusing way this test has been written 
because we don't actually test the invocation mechanism in those cases! 
This means some things that should be tested are not. For example:


 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");

fails because method lookup in InterfaceB fails, but what we should also 
be testing is the actual InterfaceType.invokeMethod on InterfaceB using 
the Method for staticMethodA obtained from InterfaceA. So this suite of 
tests in incomplete.


It is also unclear to me if some things that should be checked at the 
JDWP level are in fact caught at the JDI level - a lot of argument 
checking/validation occurs in the JDI Java code. You might have written 
your fix in the Java code, but that would not have fixed JDWP. So it 
makes me wonder if other things are checked at the JDI level instead of 
the JDWP level!


Aside: The test also seems to be missing half the invocation tests 
because for each call that ends up doing a virtual/non-virtual invoke, 
there should a corresponding non-virtual/virtual invoke.


Thanks,
David



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
:

   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 

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

[harold seigel]

Hi Dan,

Thanks for looking at it again.

See comment in-line.

Thanks, Harold


On 9/20/2016 11:25 AM, Daniel D. Daugherty wrote:

On 9/20/16 8:11 AM, harold seigel wrote:

Hi David,

Thanks for the review.  Please review this updated webrev containing 
the comment modifications that you suggested:


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


src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
No comments.

test/com/sun/jdi/InterfaceMethodsTest.java
L200: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)

L208: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)

So for the test cases on L202 and L210 that changed from
testInvokePos() -> testInvokeNeg(), we had test cases in
place that verified the exact opposite of what the spec says?
And the same testInvokePos() -> testInvokeNeg() changes on
L255 and L263?
Note that the spec changed in JDK-9 (see JDK-8040167 
) from:


   "Invokes a static method. The method must be member of the class
   type or one of its superclasses,  superinterfaces, or implemented
   interfaces."

To:

   "Invokes a static method. The method must be member of the class
   type or one of its superclasses."



L253: // the instance
nit typo: 'the' -> 'The' and L254 needs a period at the end.

Re: use of "re-defined"
I doubt this means class redefinition but it had me going for a 
second. :-)


Thumbs up! Feel free to ignore the nits above and I don't need
to see another webrev if you fix those.

Dan




Please also see comments in-line.


On 9/19/2016 10:50 PM, David Holmes wrote:

Hi Harold,

I'm having a lot of issues with the code and testing here. Please 
bear with me.


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

Hi,

Please review this updated webrev for fixing JDK-8160987
:

   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 

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

[harold seigel]

Hi Serguei,

Thanks for the suggestions.

Harold


On 9/20/2016 1:01 PM, serguei.spit...@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
:

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

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

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

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
:

   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 

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

[Daniel D. Daugherty]

On 9/20/16 8:11 AM, harold seigel wrote:

Hi David,

Thanks for the review.  Please review this updated webrev containing 
the comment modifications that you suggested:


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


src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
No comments.

test/com/sun/jdi/InterfaceMethodsTest.java
L200: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)

L208: // invoking static
nit typo: 'invoking' -> 'Invoking' (since you made it a sentence)

So for the test cases on L202 and L210 that changed from
testInvokePos() -> testInvokeNeg(), we had test cases in
place that verified the exact opposite of what the spec says?
And the same testInvokePos() -> testInvokeNeg() changes on
L255 and L263?

L253: // the instance
nit typo: 'the' -> 'The' and L254 needs a period at the end.

Re: use of "re-defined"
I doubt this means class redefinition but it had me going for a 
second. :-)


Thumbs up! Feel free to ignore the nits above and I don't need
to see another webrev if you fix those.

Dan




Please also see comments in-line.


On 9/19/2016 10:50 PM, David Holmes wrote:

Hi Harold,

I'm having a lot of issues with the code and testing here. Please 
bear with me.


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

Hi,

Please review this updated webrev for fixing JDK-8160987
:

   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 

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

[harold seigel]

Hi Serguei,

Thanks for checking on this.

Harold


On 9/19/2016 10:13 PM, serguei.spit...@oracle.com wrote:

On 9/19/16 11:03, Daniel D. Daugherty wrote:

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?



Dan,

Thank you for checking this.
We should be OK here as the local reference must be cleaned up upon 
return to java.


Thanks,
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 = 

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

[harold seigel]

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
:

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

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

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

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

This is just normal way of thinking. :)

Thanks,
Serguei


On 9/16/16 07:37, Daniel D. Daugherty wrote:
Going down the JNI rabbit hole is my fault. When I mumbled on 
2016-09-02 about
how this bug might be fixed, I was too focused on the fact that the 
JNI layer
didn't do what we want and I talked about how to fix it using JNI 
functions.


Harold, my apologies for wasting your time.

Dan


On 9/16/16 8: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

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

[harold seigel]

Hi Dan,

Thanks, but your comments were very helpful.

Harold


On 9/16/2016 10:37 AM, Daniel D. Daugherty wrote:
Going down the JNI rabbit hole is my fault. When I mumbled on 
2016-09-02 about
how this bug might be fixed, I was too focused on the fact that the 
JNI layer
didn't do what we want and I talked about how to fix it using JNI 
functions.


Harold, my apologies for wasting your time.

Dan


On 9/16/16 8: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 

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

[Daniel D. Daugherty]

Going down the JNI rabbit hole is my fault. When I mumbled on 2016-09-02 
about
how this bug might be fixed, I was too focused on the fact that the JNI 
layer

didn't do what we want and I talked about how to fix it using JNI functions.

Harold, my apologies for wasting your time.

Dan


On 9/16/16 8: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 

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

[harold seigel]

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

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

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

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

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

[David Holmes]

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

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

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

Hi Harold,

I did not got deep into the fix yet but wonder why the JVMTI function is 
not used.


Thanks,
Serguei


On 9/15/16 13:05, harold seigel wrote:

Hi Dan,

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.


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

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

[Daniel D. Daugherty]

On 9/15/16 2:05 PM, harold seigel wrote:

Hi Dan,

One could argue that a spec compliant JNI implementation wouldn't need 
this change in the first place...


That's a completely different discussion. And we don't want to go down
that rat hole again... :-)


Regardless, I'm withdrawing this change because I found that it fails 
a com/sun/jdi JTreg test involving static methods in interfaces.


Ouch! OK a test failure for this obscure corner is unexpected... :-(

Dan




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

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

[harold seigel]

Hi Dan,

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.


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

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

[Daniel D. Daugherty]

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

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

[harold seigel]

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


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

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

[Daniel D. Daugherty]

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