RFR: 8159746: (proxy) Support for default methods

[Peter Levart]

On Tue, 27 Oct 2020 19:08:10 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Hi Peter, 
>> 
>> Just a quick reply.  I like your idea for a `newProxyInstance` to take an invocation handler factory.   I went through the security implication and also looked into ways to avoid the need of `protected static getSuperHandler(Lookup)` method.  I haven't checked the performance and also updating the javadoc.   My prototype is in the proxy-default-method-4 branch.   I will come back to this tomorrow and respond to the other comments you had.
>> 
>> This approach seems promising.
>
> 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 cons
 tructs 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...

-------------

PR: https://git.openjdk.java.net/jdk/pull/313