Review request 8153895 (proxy) redundant read edges to superinterfaces of proxy interfaces
Peter spots the redundant read edges are added to dynamic module when creating a proxy class. Proxy class does not access super interfaces of proxy interfaces. I have verified this simple patch with all core tests and JCK api tests. diff --git a/src/java.base/share/classes/java/lang/reflect/Proxy.java b/src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java @@ -804,11 +804,6 @@ continue; } ensureAccess(target, c); - - // add all superinterfaces - for (Class<?> intf : c.getInterfaces()) { - deque.add(intf); - } } // set up proxy class access to types referenced in the method signature
Looks ok Mandy. -Chris. On 12 Apr 2016, at 00:45, Mandy Chung <mandy.chung@oracle.com> wrote:
Peter spots the redundant read edges are added to dynamic module when creating a proxy class. Proxy class does not access super interfaces of proxy interfaces. I have verified this simple patch with all core tests and JCK api tests.
diff --git a/src/java.base/share/classes/java/lang/reflect/Proxy.java b/src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java @@ -804,11 +804,6 @@ continue; } ensureAccess(target, c); - - // add all superinterfaces - for (Class<?> intf : c.getInterfaces()) { - deque.add(intf); - } }
// set up proxy class access to types referenced in the method signature
Hi Mandy, This is OK, but the whole loop could be simplified. No need for Dequeue any more: // set up proxy class access to proxy interfaces Set<Class<?>> visited = new HashSet<>(interfaces); for (Class<?> c : visited) { ensureAccess(target, c); } Regards, Peter On 04/12/2016 01:45 AM, Mandy Chung wrote:
Peter spots the redundant read edges are added to dynamic module when creating a proxy class. Proxy class does not access super interfaces of proxy interfaces. I have verified this simple patch with all core tests and JCK api tests.
diff --git a/src/java.base/share/classes/java/lang/reflect/Proxy.java b/src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java @@ -804,11 +804,6 @@ continue; } ensureAccess(target, c); - - // add all superinterfaces - for (Class<?> intf : c.getInterfaces()) { - deque.add(intf); - } }
// set up proxy class access to types referenced in the method signature
Hi Mandy, There's another redundant validation and read edge addition I think. When deriving referenced types from methods of interfaces, static methods could be skipped as they are not implemented or overridden by Proxy class: http://cr.openjdk.java.net/~plevart/jdk9-dev/Proxy.noStaticSignatures/webrev... And there's also a question whether referenced types derived from method signatures should be checked for visibility from the specified class loader (in method validateProxyInterfaces). On one hand it is nice to ensure that later usage of the proxy class will not encounter problems of visibility of types in method signatures, but on the other, Java has been known to practice late binding and a normal class implementing an interface is loaded without problems although its method signatures reference types that are not visible from the class' class loader. Only when such method is called does the resolving happen and exception is thrown. So the question is whether such visibility checks are actually beneficial. For example, one could successfully use a proxy class by only invoking methods that reference visible types if this check was not performed. The problem is of course with the setup procedure of the dynamic module where you have to add exports from modules/packages of those referenced types and read edges to modules of those references types upfront. But this problem is resolvable. If a type is not visible from the proxy class' class loader, then we don't need an export from its module/package and we don't need a read edge to its module, do we? What do you think? Regards, Peter On 04/12/2016 02:34 PM, Peter Levart wrote:
Hi Mandy,
This is OK, but the whole loop could be simplified. No need for Dequeue any more:
// set up proxy class access to proxy interfaces Set<Class<?>> visited = new HashSet<>(interfaces); for (Class<?> c : visited) { ensureAccess(target, c); }
Regards, Peter
On 04/12/2016 01:45 AM, Mandy Chung wrote:
Peter spots the redundant read edges are added to dynamic module when creating a proxy class. Proxy class does not access super interfaces of proxy interfaces. I have verified this simple patch with all core tests and JCK api tests.
diff --git a/src/java.base/share/classes/java/lang/reflect/Proxy.java b/src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java @@ -804,11 +804,6 @@ continue; } ensureAccess(target, c); - - // add all superinterfaces - for (Class<?> intf : c.getInterfaces()) { - deque.add(intf); - } } // set up proxy class access to types referenced in the method signature
Mandy, On 04/12/2016 03:16 PM, Peter Levart wrote:
The problem is of course with the setup procedure of the dynamic module where you have to add exports from modules/packages of those referenced types and read edges to modules of those references types upfront. But this problem is resolvable. If a type is not visible from the proxy class' class loader, then we don't need an export from its module/package and we don't need a read edge to its module, do we?
What do you think?
If you think my claims make sense, then the following change could establish such policy: http://cr.openjdk.java.net/~plevart/jdk9-dev/Proxy.noStaticSignatures/webrev... Regards, Peter
On Apr 12, 2016, at 6:16 AM, Peter Levart <peter.levart@gmail.com> wrote:
Hi Mandy,
There's another redundant validation and read edge addition I think. When deriving referenced types from methods of interfaces, static methods could be skipped as they are not implemented or overridden by Proxy class:
I think static methods could be skipped.
http://cr.openjdk.java.net/~plevart/jdk9-dev/Proxy.noStaticSignatures/webrev...
And there's also a question whether referenced types derived from method signatures should be checked for visibility from the specified class loader (in method validateProxyInterfaces). On one hand it is nice to ensure that later usage of the proxy class will not encounter problems of visibility of types in method signatures, but on the other, Java has been known to practice late binding and a normal class implementing an interface is loaded without problems although its method signatures reference types that are not visible from the class' class loader. Only when such method is called does the resolving happen and exception is thrown. So the question is whether such visibility checks are actually beneficial. For example, one could successfully use a proxy class by only invoking methods that reference visible types if this check was not performed.
I noticed this missing visibility check when updating proxies to work with modules. I don’t know the history why it only checks of proxy interfaces when the API was defined. In typical cases, when a proxy interface is visible to the specified class loader, the referenced types are likely visible. On the other hand, I’m cautious of the compatibility risk if the visibility check applies to referenced types as well. I don’t think this check has vast benefits while past proxy changes broke existing libraries that show up the incompatibility risk is somewhat hard to assess.
The problem is of course with the setup procedure of the dynamic module where you have to add exports from modules/packages of those referenced types and read edges to modules of those references types upfront. But this problem is resolvable. If a type is not visible from the proxy class' class loader, then we don't need an export from its module/package and we don't need a read edge to its module, do we?
True. If the type is not visible, NCFE will be thrown and this read edge and qualified exports would be redundant. Are you worrying the unused qualified exports causing security concerns? The proxy classes in a dynamic module are encapsulate and I’m not too concerned of the redundant ones. I’ll see what I would do for the static methods and send an updated webrev. Mandy
What do you think?
Regards, Peter
On 04/12/2016 02:34 PM, Peter Levart wrote:
Hi Mandy,
This is OK, but the whole loop could be simplified. No need for Dequeue any more:
// set up proxy class access to proxy interfaces Set<Class<?>> visited = new HashSet<>(interfaces); for (Class<?> c : visited) { ensureAccess(target, c); }
Regards, Peter
On 04/12/2016 01:45 AM, Mandy Chung wrote:
Peter spots the redundant read edges are added to dynamic module when creating a proxy class. Proxy class does not access super interfaces of proxy interfaces. I have verified this simple patch with all core tests and JCK api tests.
diff --git a/src/java.base/share/classes/java/lang/reflect/Proxy.java b/src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java @@ -804,11 +804,6 @@ continue; } ensureAccess(target, c); - - // add all superinterfaces - for (Class<?> intf : c.getInterfaces()) { - deque.add(intf); - } } // set up proxy class access to types referenced in the method signature
Hi Mandy, On 04/13/2016 01:55 AM, Mandy Chung wrote:
On Apr 12, 2016, at 6:16 AM, Peter Levart <peter.levart@gmail.com> wrote:
...
And there's also a question whether referenced types derived from method signatures should be checked for visibility from the specified class loader (in method validateProxyInterfaces). On one hand it is nice to ensure that later usage of the proxy class will not encounter problems of visibility of types in method signatures, but on the other, Java has been known to practice late binding and a normal class implementing an interface is loaded without problems although its method signatures reference types that are not visible from the class' class loader. Only when such method is called does the resolving happen and exception is thrown. So the question is whether such visibility checks are actually beneficial. For example, one could successfully use a proxy class by only invoking methods that reference visible types if this check was not performed.
I noticed this missing visibility check when updating proxies to work with modules. I don’t know the history why it only checks of proxy interfaces when the API was defined. In typical cases, when a proxy interface is visible to the specified class loader, the referenced types are likely visible. On the other hand, I’m cautious of the compatibility risk if the visibility check applies to referenced types as well. I don’t think this check has vast benefits while past proxy changes broke existing libraries that show up the incompatibility risk is somewhat hard to assess.
The problem is of course with the setup procedure of the dynamic module where you have to add exports from modules/packages of those referenced types and read edges to modules of those references types upfront. But this problem is resolvable. If a type is not visible from the proxy class' class loader, then we don't need an export from its module/package and we don't need a read edge to its module, do we? True. If the type is not visible, NCFE will be thrown and this read edge and qualified exports would be redundant. Are you worrying the unused qualified exports causing security concerns? The proxy classes in a dynamic module are encapsulate and I’m not too concerned of the redundant ones.
No, not about security. Mainly about binary compatibility. For example: - library A v1 defines an interface I with some methods - library B creates a dynamic proxy implementing I. It depends on library A and libraries defining types from method signatures of the interface - program P uses B and depends on the transitive closure now comes new version of library A v2 which adds a default method to interface I with signature that requires additional dependency which is tagged as "optional". Program P does not need to call this new method on the proxy created by B. Should we force P to bundle the new dependency although it is not used? Regards, Peter
On Apr 12, 2016, at 11:22 PM, Peter Levart <peter.levart@gmail.com> wrote:
No, not about security. Mainly about binary compatibility. For example:
- library A v1 defines an interface I with some methods - library B creates a dynamic proxy implementing I. It depends on library A and libraries defining types from method signatures of the interface - program P uses B and depends on the transitive closure
now comes new version of library A v2 which adds a default method to interface I with signature that requires additional dependency which is tagged as "optional". Program P does not need to call this new method on the proxy created by B. Should we force P to bundle the new dependency although it is not used?
This is the compatibility concern I mentioned if we change the spec to do the visibility checks on types referenced by the method signatures. My take on this is that the benefit does not seem to justify the potential incompatibility. We could file an issue if you think we should look at further. I’d like to push webrev.01. Mandy
On 04/13/2016 06:02 PM, Mandy Chung wrote:
On Apr 12, 2016, at 11:22 PM, Peter Levart <peter.levart@gmail.com> wrote:
No, not about security. Mainly about binary compatibility. For example:
- library A v1 defines an interface I with some methods - library B creates a dynamic proxy implementing I. It depends on library A and libraries defining types from method signatures of the interface - program P uses B and depends on the transitive closure
now comes new version of library A v2 which adds a default method to interface I with signature that requires additional dependency which is tagged as "optional". Program P does not need to call this new method on the proxy created by B. Should we force P to bundle the new dependency although it is not used? This is the compatibility concern I mentioned if we change the spec to do the visibility checks on types referenced by the method signatures. My take on this is that the benefit does not seem to justify the potential incompatibility. We could file an issue if you think we should look at further.
I’d like to push webrev.01.
Mandy
Yes, of course. This is a different issue. I think it would be better to fail lazily when some type from a method signature is not visible. Regards, Peter
Updated webrev to skip the static methods: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153895/webrev.01/ Mandy
+1 -Sundar On 4/13/2016 9:24 AM, Mandy Chung wrote:
Updated webrev to skip the static methods: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153895/webrev.01/
Mandy
On 04/13/2016 05:54 AM, Mandy Chung wrote:
Updated webrev to skip the static methods: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153895/webrev.01/
Mandy
Looks good. Regards, Peter
participants (4)
-
Chris Hegarty
-
Mandy Chung
-
Peter Levart
-
Sundararajan Athijegannathan