8280377: MethodHandleProxies does not correctly invoke default methods with varags #7185

mlchung · 2022-01-21T22:49:38Z

The MethodHandle of a default method should be made as a fixed arity method handle because it is invoked via Proxy's invocation handle with a non-vararg array of arguments. On the other hand, the InvocationHandle::invokeDefault method was added in Java 16 to invoke a default method of a proxy instance. This patch simply converts the implementation to call InvocationHandle::invokeDefault instead.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8280377: MethodHandleProxies does not correctly invoke default methods with varags

Reviewers

Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 30f1a05

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7185/head:pull/7185
$ git checkout pull/7185

Update a local copy of the PR:
$ git checkout pull/7185
$ git pull https://git.openjdk.java.net/jdk pull/7185/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7185

View PR using the GUI difftool:
$ git pr show -t 7185

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7185.diff

…s with varags

bridgekeeper · 2022-01-21T22:50:42Z

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-01-21T22:52:35Z

@mlchung The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-01-21T22:56:42Z

Webrevs

liach · 2022-01-21T23:16:50Z

Will older versions up to Java 8 get an alternative fix (changing the accessed handle to non-vararg) too? This API is currently the most backward-compatible way to create a proper (as in calling default methods as desired) interface proxy in Java 8 or 11.

mlchung · 2022-01-21T23:40:20Z

Will older versions up to Java 8 get an alternative fix (changing the accessed handle to non-vararg) too?

This is up to the update release maintainers to decide. The backport fix is straight-forward. I include that in the JBS report.

openjdk · 2022-01-22T12:39:37Z

@mlchung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8280377: MethodHandleProxies does not correctly invoke default methods with varags

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 46 new commits pushed to the master branch:

295c047: 8279242: Reflection newInstance() error message when constructor has no access modifiers could use improvement
841eae6: 8269542: JDWP: EnableCollection support is no longer spec compliant after JDK-8255987
76fe03f: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
cebaad1: 8280041: Retry loop issues in java.io.ClassCache
cbe8395: 8280168: Add Objects.toIdentityString
f4575e4: 8279946: (ch) java.nio.channels.FileChannel tryLock and write methods are missing @throws NonWritableChannelException
674a97b: 8280396: G1: Full gc mark stack draining should prefer to make work available to other threads
fe77250: 8280414: Memory leak in DefaultProxySelector
496baad: 8280030: [REDO] Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
4503d04: 8280375: G1: Tighten mem region limit in G1RebuildRemSetHeapRegionClosure
... and 36 more: https://git.openjdk.java.net/jdk/compare/c1e4f3dd1b42474c9abc22c7b981a98f9c36e0d5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

DasBrain · 2022-01-22T21:48:38Z

When testing this patch from a named module and not-exported package, I get the following exception:

Exception in thread "main" java.lang.reflect.UndeclaredThrowableException
	at jdk.proxy1/com.sun.proxy.jdk.proxy1.$Proxy0.toString(Unknown Source)
	at test.openjdk/test.openjdk.ProxyTest.main(ProxyTest.java:22)
Caused by: java.lang.IllegalAccessException: class java.lang.invoke.MethodHandleProxies$1 (in module java.base) cannot access interface test.openjdk.ProxyTest$Test (in module test.openjdk) because module test.openjdk does not export test.openjdk to module java.base
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:394)
	at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:674)
	at java.base/java.lang.reflect.InvocationHandler.invokeDefault(InvocationHandler.java:278)
	at java.base/java.lang.invoke.MethodHandleProxies$1.invoke(MethodHandleProxies.java:202)
	... 2 more

My test code.
Running without the patch results in the wrong output for default varargs methods, so this is a regression.

mlchung · 2022-01-24T23:02:58Z

@DasBrain thanks for catching this. MethodHandleProxies::asInterfaceInstance should perform no additional access checks other than checking the interface is public and not sealed, as specified in the spec.

The patch is updated.

DasBrain · 2022-01-24T23:02:52Z

src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java

@@ -202,7 +202,8 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
                    if (isObjectMethod(method))
                        return callObjectMethod(proxy, method, args);
                    if (isDefaultMethod(method)) {
-                        return callDefaultMethod(defaultMethodMap, proxy, intfc, method, args);
+                        // no additional access check is performed
+                        return JLRA.invokeDefault(proxy, method, null, args);


Isn't the argument order ..., args, caller?

Such edit was an accident. Fixed.

DasBrain · 2022-01-24T23:05:12Z

src/java.base/share/classes/java/lang/reflect/ReflectAccess.java

+
+    public Object invokeDefault(Object proxy, Method method, Object[] args, Class<?> caller)
+            throws Throwable {
+        return Proxy.invokeDefault(proxy, method, args, caller);
+    }


What about other, non-jdk code that wishes to implement a similar thing - make a proxy for an arbitrary interface and handle default methods by invoking them?

To invoke the default method, the caller will need access to the declaring interface of the default method (via bytecode invocation or reflection). The bug I had was because java.base (the module of the invocation handler) does not have access to the interface even though the caller has.

To invoke the default method, the caller will need access to the declaring interface of the default method (via bytecode invocation or reflection). The bug I had was because java.base (the module of the invocation handler) does not have access to the interface even though the caller has.

So invokeDefault is moved to Proxy with an optional access check, and you've made it callable from MHProxies by way of JLRA. I think that is okay.

AlanBateman · 2022-01-25T14:36:26Z

src/java.base/share/classes/jdk/internal/access/JavaLangReflectAccess.java

@@ -102,4 +102,7 @@ public void setConstructorAccessor(Constructor<?> c,
    /** Returns a new instance created by the given constructor with access check */
    public <T> T newInstance(Constructor<T> ctor, Object[] args, Class<?> caller)
        throws IllegalAccessException, InstantiationException, InvocationTargetException;
+
+    public Object invokeDefault(Object proxy, Method method, Object[] args, Class<?> caller)
+        throws Throwable;


Minor nit: add a comment to the method so that it's consistent with the other JLRA methods.

What about this:

/** Invokes the given default method if the method's declaring interface is * accessible to the given caller. Otherwise, IllegalAccessException will * be thrown. If the caller is null, no access check is performed. */ public Object invokeDefault(Object proxy, Method method, Object[] args, Class<?> caller) throws Throwable;

What about this:

Looks okay, main thing is to have it be consistent with the existing methods.

DasBrain · 2022-01-25T22:01:22Z

My question was for when a library wants to implement something similar to MethodHandleProxies.asInterfaceInstace, and for example supports Interfaces with more than a single abstract method.
Currently, such a library would also have to call ReflectAccess.invokeDefault, as the interface may not be accessible by the InvocationHandler (as it could be the case with MethodHandleProxies.asInterfaceInstace

But this might be a discussion for an other time.
Patch looks good.

liach · 2022-01-26T00:06:25Z

My question was for when a library wants to implement something similar to MethodHandleProxies.asInterfaceInstace, and for example supports Interfaces with more than a single abstract method. Currently, such a library would also have to call ReflectAccess.invokeDefault, as the interface may not be accessible by the InvocationHandler (as it could be the case with MethodHandleProxies.asInterfaceInstace

But this might be a discussion for an other time. Patch looks good.

They will probably accept a MethodHandles.Lookup object to define a class for such a single-interface instance plus calling the default methods beyond the restrictions.

mlchung · 2022-01-26T01:09:10Z

They will probably accept a MethodHandles.Lookup object to define a class for such a single-interface instance plus calling the default methods beyond the restrictions.

Yes, that's one option. Such library should take a Lookup parameter to find the method handle for the default method as in the previous MethodHandleProxies implementation. Alternatively, the module of the target interface will need to be open to the library's module for deep reflection and the library can use MethodHandles::privateLookupIn to lookup the default method then.

mlchung · 2022-01-26T01:23:16Z

/integrate

openjdk · 2022-01-26T01:24:26Z

Going to push as commit a183bfb.
Since your change was applied there have been 47 commits pushed to the master branch:

2eab86b: 8213905: reflection not working for type annotations applied to exception types in the inner class constructor
295c047: 8279242: Reflection newInstance() error message when constructor has no access modifiers could use improvement
841eae6: 8269542: JDWP: EnableCollection support is no longer spec compliant after JDK-8255987
76fe03f: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
cebaad1: 8280041: Retry loop issues in java.io.ClassCache
cbe8395: 8280168: Add Objects.toIdentityString
f4575e4: 8279946: (ch) java.nio.channels.FileChannel tryLock and write methods are missing @throws NonWritableChannelException
674a97b: 8280396: G1: Full gc mark stack draining should prefer to make work available to other threads
fe77250: 8280414: Memory leak in DefaultProxySelector
496baad: 8280030: [REDO] Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
... and 37 more: https://git.openjdk.java.net/jdk/compare/c1e4f3dd1b42474c9abc22c7b981a98f9c36e0d5...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-26T01:24:43Z

@mlchung Pushed as commit a183bfb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8280377: MethodHandleProxies does not correctly invoke default method…

56542fc

…s with varags

openjdk bot added the rfr Pull request is ready for review label Jan 21, 2022

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 21, 2022

AlanBateman approved these changes Jan 22, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 22, 2022

mlchung added 3 commits January 24, 2022 14:57

Should not perform access check on the interface as specified

5e4fe4b

Add new regression test

912eb22

revert MethodHandlesProxiesTest change

5d1f347

DasBrain suggested changes Jan 24, 2022

View reviewed changes

fix accident argument ordering after edit

30f1a05

AlanBateman reviewed Jan 25, 2022

View reviewed changes

AlanBateman approved these changes Jan 25, 2022

View reviewed changes

add comment

3f31d37

openjdk bot added the integrated Pull request has been integrated label Jan 26, 2022

openjdk bot closed this Jan 26, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 26, 2022

mlchung deleted the JDK-8280377 branch February 10, 2022 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8280377: MethodHandleProxies does not correctly invoke default methods with varags #7185

8280377: MethodHandleProxies does not correctly invoke default methods with varags #7185

mlchung commented Jan 21, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 21, 2022

openjdk bot commented Jan 21, 2022

mlbridge bot commented Jan 21, 2022 •

edited

Loading

liach commented Jan 21, 2022 •

edited

Loading

mlchung commented Jan 21, 2022

openjdk bot commented Jan 22, 2022 •

edited

Loading

DasBrain commented Jan 22, 2022 •

edited

Loading

mlchung commented Jan 24, 2022 •

edited

Loading

DasBrain Jan 24, 2022

mlchung Jan 24, 2022

DasBrain Jan 24, 2022

mlchung Jan 24, 2022 •

edited

Loading

AlanBateman Jan 25, 2022

AlanBateman Jan 25, 2022

mlchung Jan 25, 2022

AlanBateman Jan 25, 2022

DasBrain commented Jan 25, 2022

liach commented Jan 26, 2022

mlchung commented Jan 26, 2022

mlchung commented Jan 26, 2022

openjdk bot commented Jan 26, 2022

openjdk bot commented Jan 26, 2022

8280377: MethodHandleProxies does not correctly invoke default methods with varags #7185

8280377: MethodHandleProxies does not correctly invoke default methods with varags #7185

Conversation

mlchung commented Jan 21, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 21, 2022

openjdk bot commented Jan 21, 2022

mlbridge bot commented Jan 21, 2022 • edited Loading

Webrevs

liach commented Jan 21, 2022 • edited Loading

mlchung commented Jan 21, 2022

openjdk bot commented Jan 22, 2022 • edited Loading

DasBrain commented Jan 22, 2022 • edited Loading

mlchung commented Jan 24, 2022 • edited Loading

DasBrain Jan 24, 2022

Choose a reason for hiding this comment

mlchung Jan 24, 2022

Choose a reason for hiding this comment

DasBrain Jan 24, 2022

Choose a reason for hiding this comment

mlchung Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

AlanBateman Jan 25, 2022

Choose a reason for hiding this comment

AlanBateman Jan 25, 2022

Choose a reason for hiding this comment

mlchung Jan 25, 2022

Choose a reason for hiding this comment

AlanBateman Jan 25, 2022

Choose a reason for hiding this comment

DasBrain commented Jan 25, 2022

liach commented Jan 26, 2022

mlchung commented Jan 26, 2022

mlchung commented Jan 26, 2022

openjdk bot commented Jan 26, 2022

openjdk bot commented Jan 26, 2022

mlchung commented Jan 21, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 21, 2022 •

edited

Loading

liach commented Jan 21, 2022 •

edited

Loading

openjdk bot commented Jan 22, 2022 •

edited

Loading

DasBrain commented Jan 22, 2022 •

edited

Loading

mlchung commented Jan 24, 2022 •

edited

Loading

mlchung Jan 24, 2022 •

edited

Loading