On Fri, 16 Oct 2020 14:46:59 GMT, Peter Levart <plev...@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

Reply via email to