RFR: 6983726: remove Proxy from MethodHandleProxies.asInterfaceInstance SAM conversion

[Chen Liang]

On Wed, 5 Apr 2023 17:59:24 GMT, Johannes Kuhn <jkuhn at openjdk.org> wrote:

>>> I think it should be possible to spin the class bytes once, stick the result in a ClassValue cache, but then use the bytes to define multiple classes with different class data (MethodHandles).
>> 
>> Great point, I was dumbfounded there.
>> 
>>> Let suppose you have an interface like:
>>> 
>>> ```
>>> interface StringConsumer extends Consumer<String> {}
>>> ```
>>> 
>>> The implementation needs to override both accept(Object) and accept(String).
>> 
>> It already does, and there are multiple test cases covering that: [mine](https://github.com/openjdk/jdk/pull/13197/files#diff-9753e5dae7b27070a1fe0d6c12eb65db605ee479d0e93d214ce710a8bbfc4922R176) and another in [MethodHandlesGeneralTest](https://github.com/openjdk/jdk/blob/927e674c12aa7965c63059b8f650d8f60156cefc/test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java#L1785)
>> 
>>> Also, it would be nice if you could include the benchmarks you used in the patch as well.
>> 
>> They are from the JDK itself: https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesSuppl.java https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstance.java
>> 
>> Due to make issues (it passed `/` for module exports as `` on my windows somehow), I ran the test by copying them to a separate Gradle project with Gradle JMH plugin.
>> 
>>> I've worked on something similar before: https://bugs.openjdk.org/browse/JDK-8257605 The idea then was to add a new API in MethodHandles but since then I've been thinking that enhancing the performance of MethodHandleProxies might be better. The only issue I saw with that is that the current API doesn't accept a MethodHandles.Lookup object which would be needed to define an interface instance if the interface is in a non-exported package. Your solution just creates the lookup directly. Typically we have to be careful with injecting classes into other packages/modules since it can lead to access escalation, but maybe in this case it is fine. I don't think there's a way that this could be used to escalate access, though it seems strange that anyone could now define a class in a non-exported package/some other module.
>> 
>> About access:
>> 1. The method handle itself is already access-checked upon creation. 
>> 2. The interface access may be problematic: A non-exported interface Class object can be obtained via Reflection inspection on exported types, such as java packages and jdk.internal packages. 
>>    - In that case, it might not be of best interest to create an interface, but I don't think the current asInterfaceInstance API rejects such creations either.
>> 3. The class definition under the interface and its classloader IMO is safe, as the class will not refer to any type the interface itself does not refer to; the annotation is not involved in class loading either.
>> 
>>> We've also recently discussed LambdaMetafactory https://github.com/openjdk/jdk/pull/12493 which will produce classes strongly tied to the class loader. If we can bring MethodHandleProxies up to the same performance level as the classes produced by LambdaMetaFactory, it could serve as an alternative and people could move away from using the LambdaMetafactory runtime API (which is not really meant to be used directly). WRT that, I think the fact that the implementation proposed by this patch defines classes that are not strongly tied to the defining loader is a good thing.
>> 
>> Sounds good; it's not hard to make a benchmark that compares asInterfaceInstance and metafactory side-by-side either. The non-strong feature was intended indeed when one class is defined for each class + methodhandle combination. I agree `asInterfaceInstance` would be extremely useful to users, like a plugin loader converting json-based static method references into SAM implementations, which definitely should not become defined as a part of the plugin loader; or an event bus firing events.
>
>> The interface access may be problematic: A non-exported interface Class object can be obtained via Reflection inspection on exported types, such as java packages and jdk.internal packages.
>> 
>>     * In that case, it might not be of best interest to create an interface, but I don't think the current asInterfaceInstance API rejects such creations either.
> 
> See the Javadoc: https://github.com/openjdk/jdk/pull/13197/files#diff-6de80127c851b1b0ba6b2ab0a739ffae803187028a721d4a28cd47fb17b1bbcdL64-L65
> 
> As this API was added in Java 7, `public` access was easy. W.R.T. modules, no changes have been made to this API.  
> The (previously) underlying `java.lang.reflect.Proxy` does not even require that.
> 
> @liach  Can you please test calling `MethodHandleProxies.wrapperInstanceTarget(MethodHandleProxies.asInterfaceInstance(Runnable.class, MethodHandles.zero(void.class)))` **with an installed `SecurityManager`**?
> Also with an interface in an untrusted protection domain, for example:
> 
> 
> public interface Test {
>     void run();
>     public static void main(String[] args) {
>         System.out.println(MethodHandleProxies.wrapperInstanceTarget(MethodHandleProxies.asInterfaceInstance(Test.class, MethodHandles.zero(void.class))));
>     }
> }
> 
> also with a `SecurityManager` (`-Djava.security.manager` as VM argument).

> > @DasBrain Thanks for the recommendation to test with SecurityManager, added a test and found two places that needs to do privileged indeed.
> 
> What operations require the security permission check? I suspect some doPrivileged may be missing in the ClassFile API implementation.

It appears the default class hierarchy resolver cannot be used to generates stack maps with only the FilePermission required by JTReg granted.

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

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1499433516