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
