On Thu, 15 Oct 2020 09:04:09 GMT, Peter Levart <plev...@openjdk.org> wrote:
>> `invokeDefaultMethod` is moved to a static method in `InvocationHandler`.
>> I also add `@throws IllegalAccessException` if the specified default method
>> is
>> inaccessible to the caller (access check was missing in the previous
>> version).
>> The declaring interface of the default method must be exported to the caller
>> to
>> use. If it's a non-public interface, the caller class must be in the same
>> runtime
>> package as the interface. The example in javadoc includes the case when
>> the proxy interface is modified.
>
> Hi Mandy,
>
> I'm beginning to think that we somewhat wondered away from the original
> intent which was to mimic the bytecode
> behaviour as much as possible. Bytecode behaviour for super calls
> (invokespecial) is very special and what you do in
> the last patch is guarding invocation to a super default method via
> @CallerSensitive method which checks the access of
> the caller as though this was a normal call. Similar access check when a
> default method might reside in a
> package-private class or when this class might not be exported is already
> performed when the code creates a Proxy
> class/instance (Proxy::getProxyClass/newProxyInstance). This code also
> designates an InvocationHandler for the proxy
> instance. From that point on, the designated invocation handler should have
> full access to invoking super default
> methods on the proxy instance regardless of where it resides because it has
> been chosen as the delegate of the Proxy
> class with full powers. Since we are exposing the invokeDefaultMethod
> functionality in a public static method, it is
> hard (or not quite possible) to check that the code doing the invocation is
> in fact the designated invocation handler
> code. One approximation might be the following check:
> `Proxy.getInvocationHandler(proxy).getClass() == callerClass`
> ...but that would not work for code residing in the invocation handler
> superclass even though such code might be
> eligible to call the super default method. OTOH such check is over permissive
> as it allows calling the super method
> from static context too. So I was thinking about an alternative approach. To
> expose the method as a protected final
> instance method in an abstract class implementing InvocationHandler: public
> interface InvocationHandler {
> ...
> abstract class Support implements InvocationHandler {
> protected final Object invokeDefaultMethod(Object proxy, Method method,
> Object... args) throws
> InvocationTargetException {
> if (this != Proxy.getInvocationHandler(proxy)) throw ...
> ...
> }
> }
> }
>
> Users would have to extend `InvocationHandler.Support` to access this
> feature. This might be OK for most users, but
> those wanting to extend some other class will not be able to do that.
> So WDYT? In case you still prefer your approach of re-checking the
> permissions in invokeDefaultMethod, then I might
> have some ideas of how to speed-up theses checks. As currently stands,
> invoking public exported interface default
> methods does exhibit a little more penalty, but for non-public or
> non-exported interfaces, the penalty is bigger:
> 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
>
> The updated benchmark code is here:
> https://gist.github.com/plevart/f6e0a5224c3ffbf14b28b47755599226
> (the ppImpl... benchmarks are against package-private interface)
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?
-------------
PR: https://git.openjdk.java.net/jdk/pull/313
