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

Reply via email to