On Tue, 27 Oct 2020 21:45:53 GMT, Peter Levart <plev...@openjdk.org> wrote:
>> Hi Peter, >> >> To capture what I considered for security: we need to ensure that the one >> who can invoke the default methods of the proxy interfaces of a proxy via >> the super-default-method invocation handler should have access. So >> - `newProxyInstance` will do access check to ensure that the caller class >> has access to all proxy interfaces at proxy creation time >> - `getInvocationHandler` will do access check to ensure that the caller >> class has access to all proxy interfaces at proxy creation time. >> - the owner of the proxy instance and invocation handler should protect the >> super-default-method handler passed to the invocation handler factory and >> make sure it's not leaked to malicious code. >> >> Since this is a new factory method, I'm thinking to move the interfaces >> argument to the last so that it can be a vararg, like this: >> >> public static Object newProxyInstance( >> Function<? super InvocationHandler, ? extends InvocationHandler> >> handlerFactory, >> ClassLoader loader, >> Class<?>... interfaces) throws IllegalAccessException >> >> But the usage may be a bit strange. >> Proxy.newProxyInstance(superHandler -> (p, m, a) -> superHandler.invoke(p, >> m, a), >> loader, I.class, J.class); >> >> Since the current `getInvocationHandler` API does not throw >> `IllegalAccessException`, we have two options >> a) have `getInvocationHandler` to throw an unchecked exception wrapping >> `IAE` such as `IllegalCallerException` or `IllegalArgumentException`. Or a >> new `UncheckedIllegalAccessException`. >> b) add a new method `Proxy.invocationHandler(Object proxy) throws >> IllegalAccessException`. `getInvocationHandler` will still need to throw >> an unchecked exception if the given proxy supports invocation of default >> methods. >> >> Responding to Peter's comments: >>> The changes to ProxyGenerator are just to accomodate for new static final >>> field holding the super-default-method InvocationHandler. Alternatively a >>> ClassValue could be used for that without changes to ProxyGenerator albeit >>> with somewhat bigger footprint per proxy class. >> >> The static `newSuperHandler` method is just a bridge for the proxy class to >> create its super-default-method handler. It's not really needed to expose >> as a public API. Instead I pass the super-default-method to the private >> Proxy constructor and store it in a private final field. Yes, a small >> footprint increase. It's a tradeoff. >> >>> So wouldn't it be better for the super-invoke API to throw exact exceptions >>> that the invoked methods are throwing? >> >> With this new API, `SuperInvocationHandler` should follow the spec of >> `InvocationHandler::invoke`: >> Throwable - the exception to throw from the method invocation on the proxy >> instance. The exception's type must be assignable either to any of the >> exception types declared in the throws clause of the interface method or to >> the unchecked exception types java.lang.RuntimeException or java.lang.Error. >> If a checked exception is thrown by this method that is not assignable to >> any of the exception types declared in the throws clause of the interface >> method, then an UndeclaredThrowableException containing the exception that >> was thrown by this method will be thrown by the method invocation on the >> proxy instance. >> >> In other words, I agree it does not need to wrap it with >> `InvocationTargetException`. And `IllegalArgumentException` may be thrown >> by the invoked method and also thrown due to arguments mismatched with >> method signature. > > Hi Mandy, > > Comments inline... > >> Hi Peter, >> >> To capture what I considered for security: we need to ensure that the one >> who can invoke the default methods of the proxy interfaces of a proxy via >> the super-default-method invocation handler should have access. So >> >> * `newProxyInstance` will do access check to ensure that the caller >> class has access to all proxy interfaces at proxy creation time >> >> * `getInvocationHandler` will do access check to ensure that the caller >> class has access to all proxy interfaces at proxy creation time. >> >> * the owner of the proxy instance and invocation handler should protect >> the super-default-method handler passed to the invocation handler factory >> and make sure it's not leaked to malicious code. > > I agree (see about exceptions below). > >> >> >> Since this is a new factory method, I'm thinking to move the interfaces >> argument to the last so that it can be a vararg, like this: >> >> ``` >> public static Object newProxyInstance( >> Function<? super InvocationHandler, ? extends InvocationHandler> >> handlerFactory, >> ClassLoader loader, >> Class<?>... interfaces) throws IllegalAccessException >> ``` >> >> But the usage may be a bit strange. >> >> ``` >> Proxy.newProxyInstance(superHandler -> (p, m, a) -> superHandler.invoke(p, >> m, a), >> loader, I.class, J.class); >> ``` > > Well, yes. Vararg parameter can only be the last parameter, but lambda > parameter looks best as the last parameter too. In particular if it is a > statement lambda (with curly braces in lambda body and long body). So I'm > divided on that part. What about a List<Class<?>> insteadn of Class<?>[] ? > Hm... > > Proxy.newProxyInstance(loader, of(I.class, J.class), superHandler -> (p, m, > a) -> { > ... > }); > > Vs. > > Proxy.newProxyInstance(loader, new Class<?>[] {I.class, J.class}, > superHandler -> (p, m, a) -> { > ... > }); > > Not too bad. Shortening `List.of(I.class, J.class)` to `of(I.class, J.class)` > with static import we get almost the same character count without reordering > lambda parameter... > > >> >> Since the current `getInvocationHandler` API does not throw >> `IllegalAccessException`, we have two options >> a) have `getInvocationHandler` to throw an unchecked exception wrapping >> `IAE` such as `IllegalCallerException` or `IllegalArgumentException`. Or a >> new `UncheckedIllegalAccessException`. >> b) add a new method `Proxy.invocationHandler(Object proxy) throws >> IllegalAccessException`. `getInvocationHandler` will still need to throw an >> unchecked exception if the given proxy supports invocation of default >> methods. >> > > I'm divided on this one too. But, if we must create new exception type, then > instead of `UncheckedIllegalAccessException`, we could create a type that > covers all `ReflectiveOperationException`(s) like `UncheckedIOException` > covers all `IOException`(s). We could create an > `UncheckedReflectiveOperationException` which would be universally useful for > wrapping reflective exceptions in lambdas etc. > > Question remains how do we distinguish proxies with old-fassioned > InvocationHandlers from proxies with InvocationHandlers having access to > super-default-handler. Both kinds of handlers look the same from Proxy's > perspective... Perhaps we need a boolean flag in Proxy instance for that, > coupled with an overloaded constructor that takes it... > >> Responding to Peter's comments: >> >> > The changes to ProxyGenerator are just to accomodate for new static final >> > field holding the super-default-method InvocationHandler. Alternatively a >> > ClassValue could be used for that without changes to ProxyGenerator albeit >> > with somewhat bigger footprint per proxy class. >> >> The static `newSuperHandler` method is just a bridge for the proxy class to >> create its super-default-method handler. It's not really needed to expose as >> a public API. Instead I pass the super-default-method to the private Proxy >> constructor and store it in a private final field. Yes, a small footprint >> increase. It's a tradeoff. >> > > Problem with this approach is that it is not really useful in the proxy > instance. We don't need it in the proxy instance. We need it before we call > the invocation handler factory function with it and that is before we create > the proxy instance with the resulting invocation handler. We need to cache it > at the proxy class level. No problem, we can use ClassValue for that. What I > achieved with protected static Proxy.newSuperHandler method was an easy way > to pass the Lookup object created in the static initializer of the proxy > class down to the constructor of the SuperHandler. But if we have an > internal way of constructing a full-privileged Lookup object with proxy class > as lookup class, then we don't need to construct it in the proxy class static > initializer... One way to do that was your implementation of a generated > static method in the proxy class that verifies the caller by checking the > passed in Lookup object is full-privileged Lookup with Proxy.class as lookup > class and then co nstructs and returns its own Lookup instance (LIEP - Lookup Instance Exchange Protocol). > >> > So wouldn't it be better for the super-invoke API to throw exact >> > exceptions that the invoked methods are throwing? >> >> With this new API, `SuperInvocationHandler` should follow the spec of >> `InvocationHandler::invoke`: >> >> ``` >> Throwable - the exception to throw from the method invocation on the proxy >> instance. The exception's type must be assignable either to any of the >> exception types declared in the throws clause of the interface method or to >> the unchecked exception types java.lang.RuntimeException or java.lang.Error. >> If a checked exception is thrown by this method that is not assignable to >> any of the exception types declared in the throws clause of the interface >> method, then an UndeclaredThrowableException containing the exception that >> was thrown by this method will be thrown by the method invocation on the >> proxy instance. >> ``` >> >> In other words, I agree it does not need to wrap it with >> `InvocationTargetException`. And `IllegalArgumentException` may be thrown by >> the invoked method and also thrown due to arguments mismatched with method >> signature. > > Right. It feels better that way. I can take some time tomorrow or in then > next days to prepare a variant with these changes if we reach an agreement on > the exceptions thrown from getInvocationHandler(proxy) method... Hi Peter, > Question remains how do we distinguish proxies with old-fassioned > InvocationHandlers from proxies with InvocationHandlers having access to > super-default-handler. Both kinds of handlers look the same from Proxy's > perspective... Perhaps we need a boolean flag in Proxy instance for that, > coupled with an overloaded constructor that takes it... I have the Proxy constructor taking both the invocation handler and `superHandler` in my [mlchung:proxy-default-method-4](https://github.com/mlchung/jdk/tree/proxy-default-method-4) branch. > Problem with this approach is that it is not really useful in the proxy > instance. We don't need it in the proxy instance. See above. About the exception thrown: we need to distinguish CCE and NPE thrown by the default method vs thrown by arguments incompatible with the method signature. In my proxy-default-method-4 branch, I define an internal exception type for that. There is an overhead doing the wrapping but it's an exception case which performance should not be critical. ------------- PR: https://git.openjdk.java.net/jdk/pull/313