RFR: 8159746: (proxy) Support for default methods

[Mandy Chung]

On Fri, 16 Oct 2020 14:46:59 GMT, Peter Levart <plevart at openjdk.org> wrote:

>> Hi Peter,
>> 
>> The current `newProxyInstance` API does not perform access check against the proxy interfaces.    I want to avoid
>> adding new caller-sensitive method where possible.   I did consider any alternative to make it non-static method which
>> only allows a proxy's invocation handler to call `invokeDefaultMethod` if `proxy.getInvocationHandler() == this`.  If
>> access check done at invocation time, `invokeDefaultMethod` still needs to be caller-sensitive which must be a final
>> method (can't be on a default method)  In addition, an invocation handler to call `invokeDefaultMethod` can't be
>> implemented in a lambda because it can't self-reference itself.  So the invocation handler that wants to invoke default
>> methods must be a concrete or anonymous class which is a limitation.    Alan and I also discussed other alternative
>> earlier.   I agree to prototype a slightly different alternative that will perform access check at proxy instance
>> creation time: 1. define an abstract `DelegatingInvocationHandler` class that defines a protected final `invokeDefault`
>> method (shortened method name). 2. `Proxy::newProxyClass` will perform access check if the given `InvocationHandler` is
>> an instance of `DelegatingInvocationHandler` 3. `Proxy(InvocationHandler h)` constructor should throw an exception if
>> the given `h` is an instance of `DelegatingInvocationHandler`.  `Proxy::getProxyClass` is deprecated and it's
>> recommended to call `newProxyInstance` instead.  It's strongly discouraged to invoke the Proxy constructor and so I
>> think not supporting `DelegatingInvocationHandler` may be reasonable. 4. `Proxy::getInvocationHandler` needs to check
>> if the caller class has access to the proxy interfaces if it's a `DelegatingInvocationHandler`.  So one can't get a
>> proxy instance whose invocation handler can invoke default methods if the caller has no access to the proxy interfaces;
>> similarly one can't get a `DelegatingInvocationHandler` from a proxy instance if the caller has no access.  This
>> alternative avoids `invokeDefault` being caller-sensitive but the limitation of `DelegatingInvocationHandler`
>> implementation in a concrete/anonymous class.  Thought/feedback?
>
> Hi Mandy,
> 
> I read your concerns and have an alternative API proposition here. Instread of an abstract class with protected final
> `invokeDefaultMethod` method, we can keep the public static `invokeDefaultMethod` and add a `Lookup` parameter to it.
> The Lookup instance to be passed to the method should be able to `findSpecial` the default method - it has to be the
> full privilege lookup with proxy class as lookup class. Such instance is provided in a call to the alternative
> invocation handler: `InvocationHandlerWithLookup` which is a super-interface of plain `InvocationHandler` that just
> forwards the invocation with the additional Lookup parameter to the abstract method without parameter (in order to keep
> backwards-compatibility). The generated proxy class invokes this new method in new interface and passes its own lookup
> to it. There's also an overloaded Proxy::newProxyInstance method that takes this new InvocationHandlerWithLookup
> parameter instead of plain InvocationHandler. This allows specifying lambdas for both overloaded methods and the right
> one is selected according to the number of lambda parameters. invoking the constructor of the proxy class taking plain
> InvocationHandler is still allowed but only if the handler class does not use the Lookup parameter. The overloaded
> Proxy.newProxyInstance throws IllegalAccessException in case the passed-in handler uses the provided lookup but some
> interfaces of the proxy are not accessible.  Performance with this implementation is restored and is not worse when
> dealing with inaccessible interfaces:  Mandy's original:  Benchmark             Mode  Cnt    Score    Error  Units
> ProxyBench.implClass  avgt    5    3.709 ±  0.026  ns/op ProxyBench.implProxy  avgt    5  926.215 ± 11.835  ns/op
> Peter's performance patch:  Benchmark             Mode  Cnt   Score   Error  Units ProxyBench.implClass  avgt    5
> 3.777 ± 0.005  ns/op ProxyBench.implProxy  avgt    5  27.579 ± 0.250  ns/op
> 
> Moved to InvocationHandler + added access check
> 
> Benchmark               Mode  Cnt    Score   Error  Units
> ProxyBench.implClass    avgt    5    3.740 ± 0.004  ns/op
> ProxyBench.implProxy    avgt    5   34.226 ± 0.125  ns/op
> ProxyBench.ppImplClass  avgt    5    3.780 ± 0.004  ns/op
> ProxyBench.ppImplProxy  avgt    5  147.318 ± 1.422  ns/op
> 
> 
> Alternative API with access check in newProxyInstance
> 
> Benchmark               Mode  Cnt   Score   Error  Units
> ProxyBench.implClass    avgt    5   3.782 ± 0.013  ns/op
> ProxyBench.implProxy    avgt    5  32.493 ± 0.192  ns/op
> ProxyBench.ppImplClass  avgt    5   3.749 ± 0.002  ns/op
> ProxyBench.ppImplProxy  avgt    5  30.565 ± 0.190  ns/op
> 
> The code is in this pull request against your last commit in your branch: https://github.com/mlchung/jdk/pull/2

Hi Peter,

This seems an attracting idea to keep the possibility of using lambdas.  However, the full privileged lookup of the
proxy class will be leaked to `InvocationHandlerWithLookup` implementation which imposes security concerns.   This
full-privileged lookup has access to other proxy classes in that module including private members that we don't want to
hand out to user code.

Other than this security concern, we will also need to consider the inconsistency having `java.lang.reflect` API to
reference `Lookup`.

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

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