On Wed, 5 Apr 2023 17:59:24 GMT, Johannes Kuhn <[email protected]> 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.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1499420043
