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
