JDK-8194554 : filterArguments runs multiple filters in the wrong order
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.lang.invoke
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2018-01-04
  • Updated: 2018-05-08
  • Resolved: 2018-01-17
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10 JDK 11
10.0.2Fixed 11 b01Fixed
Related Reports
Blocks :  
JDK-8200118 - [TESTBUG] String concat tests should test toString() application order
CSR :  
JDK-8201371 - filterArguments runs multiple filters in the wrong order
Sub Tasks
JDK-8201497 :  
Release Note: filterArguments Runs Multiple Filters in the Wrong Order - Closed
Description
If MethodHandles.filterArguments is used to apply two or more non-null filters to a method handle, those filters will be run in right-to-left order in the OpenJDK implementation.  The javadoc indicates that these filters are run in normal argument order, left-to-right; IBM's implementation correctly implements this specification.  If we have unit tests for this, they are probably using filter functions with no side effects, so it is impossible to detect the error.

Suggested fix:  Run the accumulation loop backwards over the array of filters.  And put in a regression test with side-effecting filters.

Links:
http://mail.openjdk.java.net/pipermail/mlvm-dev/2018-January/006789.html
http://mail.openjdk.java.net/pipermail/mlvm-dev/2018-January/006798.html
Comments
Mandy and Paul have acked the backport RFR. CSR is also approved. Does this mean I can push the change?
12-04-2018
RFR for jdk10 backport: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052441.html
05-04-2018
This is reasonable. Alexey, can you provide a link to the JDK10u backport codereview as the change cannot be backported as is. (presumably this will be a formality based on Pauls note. I would imagine the backport will consist of the fix minus the spec change)
05-04-2018
I think the risk to any existing MH users relying on the right-to-left order of side-effects is very small compared to the risk of users in general hitting a string concat issue (where it is more likely that a toString implementation may have side-effects). IIRC at the time a CSR was not created because it was considered an implementation bug, since the order is implied by the pseudocode. We erred on the side of the spec being unambiguous, but we can retroactively log a CSR to make the intent crystal clear (+ release note).
02-04-2018
This seems a risky change for an update release, doesn't it break existing code that uses two or more filters? For JDK 11 then I assume a CSR is needed, and a release note.
02-04-2018
Fix Request This issue causes the failure in String concat, which is seen in the wild, and can be captured with targeted tests, see JDK-8200118. It is important to have this fix in GA release.
23-03-2018
Suggested fix: diff --git a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java --- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java @@ -3760,6 +3760,7 @@ * specified in the elements of the {@code filters} array. * The first element of the filter array corresponds to the {@code pos} * argument of the target, and so on in sequence. + * The filter functions are invoked in left to right order. * <p> * Null arguments in the array are treated as identity functions, * and the corresponding arguments left unchanged. @@ -3830,11 +3831,10 @@ MethodHandle filterArguments(MethodHandle target, int pos, MethodHandle... filters) { filterArgumentsCheckArity(target, pos, filters); MethodHandle adapter = target; - int curPos = pos-1; // pre-incremented - for (MethodHandle filter : filters) { - curPos += 1; + for (int i = filters.length - 1; i >= 0; --i) { + MethodHandle filter = filters[i]; if (filter == null) continue; // ignore null elements of filters - adapter = filterArgument(adapter, curPos, filter); + adapter = filterArgument(adapter, pos + i, filter); } return adapter; } And add a unit test. API clarification only so no CCR needed.
04-01-2018