Review request for 7198429: need checked categorization of caller-sensitive methods in the JDK

[Mandy Chung]

Peter,

Thanks.  I misread your example and missed the fact that SM2 extends 
SM1.  After seeing John's reply, I reread the example and realized the 
correctness issue you try to point out.

I'll revise it and send out a new webrev.

Mandy

On 4/3/2013 12:39 AM, Peter Levart wrote:
> On 04/03/2013 12:25 AM, Mandy Chung wrote:
>> On 4/2/13 3:00 PM, Peter Levart wrote:
>>> Hi Mandy,
>>>
>>> There could be:
>>>
>>> public class SM1 extends SecurityManager {
>>> @Override
>>> public void checkMemberAccess(Class<?> clazz, int which) {...
>>>
>>> and:
>>>
>>> public class SM2 extends SM1 { ... // no checkMemberAccess override
>>>
>>> now if if you take SM2.class.getDeclaredMethod("checkMemberAccess", 
>>> ...) it will throw NoSuchMethodException, but the method is 
>>> overriden in
>>> SM1. So I think it's better to use what John suggested (although not 
>>> using getDeclaredMethod, but getMethod instead):
>>>
>>> smgr.getClass().getMethod("checkMemberAccess", 
>>> ...).getDeclaringClass() == SecurityManager.class
>>
>> Are you concerned the overhead of an exception thrown that we should 
>> avoid?
>
> Hi Mandy,
>
> No, I was not thinking of performance. Just the correctness. 
> Class.getDeclaredMethod(name, ...) only considers methods "declared" 
> by the class (not inherited from a superclass - directy or 
> indirectly). But you could have, like in example above, a hierarchy: 
> SM2 extends SM1 extends SecurityManager where SM1 overrides the method 
> in SecurityManager and SM2 doesn't. Now if you try 
> SM2.class.getDeclaredMethod(), you will get NoSuchMethodException, but 
> that does not mean the method is not overriden - it is in class SM1.
>
> You could use getDeclaredMethod on the class and all superclasses 
> until reaching SecurityManager to find out if the method is overriden, 
> but it's fortunate that the method is public and so 
> Class.getMethod(name, ...) can be used which does exactly that and 
> already considers inheritance and overriding and returns the most 
> specific method in the hierarchy. Explicit search using 
> getDeclaredMethod() could skip searching the method in SecurityManager 
> class though, but:
>
> - negative answer via exception is slow
> - getMethod() only searches among public methods (which are separately 
> cached in special array) and can therefore be faster if there is 
> relatively large share of non-public methods declared in the class 
> being searched.
>
> Here's a quick micro-benchmark for the common case (only one level of 
> subclassing the SecurityManager) and both variants (the method is 
> overriden in SecurityManager subclass or not):
>
> ##############################################################
> # Java: 1.8.0-internal-peter_2013_01_16_13_44-b00
> # VM: OpenJDK 64-Bit Server VM 25.0-b14 (mixed mode)
> # OS: Linux 3.7.9-104.fc17.x86_64 (amd64)
> # CPUs: 8 (virtual)
> #
> #-------------------------------------------------------------
> # GetDeclaredMethodOverridenSearch: run duration: 3,000 ms
> #
> # Warm up:
> # 1 threads, Tavg = 327.59 ns/op (σ = 0.00 ns/op) [ 327.59]
> # 1 threads, Tavg = 326.06 ns/op (σ = 0.00 ns/op) [ 326.06]
> # Measure:
> 1 threads, Tavg = 325.18 ns/op (σ = 0.00 ns/op) [ 325.18]
> #
> #-------------------------------------------------------------
> # GetPublicMethodOverridenSearch: run duration: 3,000 ms
> #
> # Warm up:
> # 1 threads, Tavg = 325.69 ns/op (σ = 0.00 ns/op) [ 325.69]
> # 1 threads, Tavg = 329.67 ns/op (σ = 0.00 ns/op) [ 329.67]
> # Measure:
> 1 threads, Tavg = 325.88 ns/op (σ = 0.00 ns/op) [ 325.88]
> #
> #-------------------------------------------------------------
> # GetDeclaredMethodNotOverridenSearch: run duration: 3,000 ms
> #
> # Warm up:
> # 1 threads, Tavg = 1,405.84 ns/op (σ = 0.00 ns/op) [ 1,405.84]
> # 1 threads, Tavg = 1,371.14 ns/op (σ = 0.00 ns/op) [ 1,371.14]
> # Measure:
> 1 threads, Tavg = 1,358.02 ns/op (σ = 0.00 ns/op) [ 1,358.02]
> #
> #-------------------------------------------------------------
> # GetPublicMethodNotOverridenSearch: run duration: 3,000 ms
> #
> # Warm up:
> # 1 threads, Tavg = 557.50 ns/op (σ = 0.00 ns/op) [ 557.50]
> # 1 threads, Tavg = 553.05 ns/op (σ = 0.00 ns/op) [ 553.05]
> # Measure:
> 1 threads, Tavg = 554.59 ns/op (σ = 0.00 ns/op) [ 554.59]
> #
> #-------------------------------------------------------------
> # END.
> ##############################################################
>
>
> Here's the source of the test:
>
>
> public class MethodSearchTest extends TestRunner {
>
> static final Class[] paramTypes = {Class.class, int.class};
>
> static class SMOverriden extends SecurityManager {
> @Override
> public void checkMemberAccess(Class<?> clazz, int which) {
> }
> }
>
> static class SMNotOverriden extends SecurityManager {
> }
>
> public static class GetDeclaredMethodOverridenSearch extends Test {
> @Override
> protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, 
> DevNull devNull3, DevNull devNull4, DevNull devNull5) {
> while (loop.nextIteration()) {
> try {
> SMOverriden.class.getDeclaredMethod("checkMemberAccess", paramTypes);
> devNull1.yield(true);
> }
> catch (NoSuchMethodException e) {
> devNull1.yield(false);
> }
> }
> }
> }
>
> public static class GetPublicMethodOverridenSearch extends Test {
> @Override
> protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, 
> DevNull devNull3, DevNull devNull4, DevNull devNull5) {
> while (loop.nextIteration()) {
> try {
> devNull1.yield(SMOverriden.class.getMethod("checkMemberAccess", 
> paramTypes).getDeclaringClass() != SecurityManager.class);
> }
> catch (NoSuchMethodException e) {
> throw new Error(e);
> }
> }
> }
> }
>
> public static class GetDeclaredMethodNotOverridenSearch extends Test {
> @Override
> protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, 
> DevNull devNull3, DevNull devNull4, DevNull devNull5) {
> while (loop.nextIteration()) {
> try {
> SMNotOverriden.class.getDeclaredMethod("checkMemberAccess", paramTypes);
> devNull1.yield(true);
> }
> catch (NoSuchMethodException e) {
> devNull1.yield(false);
> }
> }
> }
> }
>
> public static class GetPublicMethodNotOverridenSearch extends Test {
> @Override
> protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, 
> DevNull devNull3, DevNull devNull4, DevNull devNull5) {
> while (loop.nextIteration()) {
> try {
> devNull1.yield(SMNotOverriden.class.getMethod("checkMemberAccess", 
> paramTypes).getDeclaringClass() != SecurityManager.class);
> }
> catch (NoSuchMethodException e) {
> throw new Error(e);
> }
> }
> }
> }
>
> public static void main(String[] args) throws Exception {
> startTests();
> doTest(GetDeclaredMethodOverridenSearch.class, 3000L, 1, 1, 1);
> doTest(GetPublicMethodOverridenSearch.class, 3000L, 1, 1, 1);
> doTest(GetDeclaredMethodNotOverridenSearch.class, 3000L, 1, 1, 1);
> doTest(GetPublicMethodNotOverridenSearch.class, 3000L, 1, 1, 1);
> endTests();
> }
> }
>
>
> Regards, Peter
>
>>
>> Mandy
>