JDK-8216558 : Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for static final field
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.lang.invoke
  • Affected Version: 11
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2019-01-11
  • Updated: 2019-04-16
  • Resolved: 2019-03-26
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 13
13 b14Fixed
Related Reports
CSR :  
JDK-8218061 - Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for static final field
Relates :  
JDK-8220282 - Add MethodHandle tests on accessing final fields
Sub Tasks
JDK-8221613 :  
Release Note: Lookup.unreflectSetter(Field) Now Throws IllegalAccessException for Static Final Fields - Closed
Description
A setter MethodHandle for a final field seems counter intuitive, as final fields should not be able to be changed (whether they are accessible or not).

However, this operation is currently allowed if you call f.setAccessible(true) before calling Lookup.unreflectSetter(Field), resulting in a valid MethodHandle being returned. Also, the MethodHandle can be utilized to set the value of the reflected final field.

A testcase will be attached to this bug in short order, as well as the proposed fix.

This issue is seen on jdk11 and jdk12, so the fix should be merged into jdk11u, jdk12u, and head stream (jdk/jdk). 

http://cr.openjdk.java.net/~afarley/8216558/webrev/
Comments
The testcase has been recreated to better match the existing tests, and has been refined + attached to the webrev. http://cr.openjdk.java.net/~afarley/8216558/webrev/
08-03-2019
Update: the webrev (attached and linked) has been updated to include the testcase, which was integrated into the existing MethodHandle tests.
14-02-2019
CSR issue: https://bugs.openjdk.java.net/browse/JDK-8218061
30-01-2019
A CSR request will still be required for the spec clarifications and because of the behavioural change (even if making it spec compliant).
15-01-2019
For purposes of translation between Core Reflection and method handle APIs, `final` modifier checks are indeed treated as access control checks. The precedent for this was set with `AccessibleObject::setAccessible`. Any behavior that a `Field` with `setAccessible` set to true is available to its translated method handle. The transfer of enhanced unchecked behavior from an `AccessibleObject` to a method handle, via `unreflect*` operations on `Lookup`, is an intentional feature, not a bug. This feature is documented in the javadoc for `Lookup`, under the heading "Lookup Factory Methods". On the other hand, as Mandy has pointed out, a *static* field does *not* accept a `Field::set` operation, even if access restrictions has been removed. This is an extra difference between the access checking of static and non-static fields. (I suppose it is present in order that the more permissive non-static field checks can support serialization frameworks, while there is no corresponding need for relaxing static field checks.) I suggest this state of affairs be documented as follows: 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 @@ -421,6 +421,10 @@ * because the desired class member is missing, or because the * desired class member is not accessible to the lookup class, or * because the lookup object is not trusted enough to access the member. + * In the case of a field setter function on a {@code final} field, + * finality enforcement is treated as a kind of access control, + * and the lookup will fail, except in special cases of + * {@link Lookup#unreflectSetter Lookup.unreflectSetter}. * In any of these cases, a {@code ReflectiveOperationException} will be * thrown from the attempted lookup. The exact class will be one of * the following: @@ -1436,6 +1440,7 @@ * @return a method handle which can store values into the field * @throws NoSuchFieldException if the field does not exist * @throws IllegalAccessException if access checking fails, or if the field is {@code static} + * or {@code final} * @exception SecurityException if a security manager is present and it * <a href="MethodHandles.Lookup.html#secmgr">refuses access</a> * @throws NullPointerException if any argument is null @@ -1559,6 +1564,7 @@ * @return a method handle which can store values into the field * @throws NoSuchFieldException if the field does not exist * @throws IllegalAccessException if access checking fails, or if the field is not {@code static} + * or is {@code final} * @exception SecurityException if a security manager is present and it * <a href="MethodHandles.Lookup.html#secmgr">refuses access</a> * @throws NullPointerException if any argument is null @@ -1868,19 +1874,29 @@ /** * Produces a method handle giving write access to a reflected field. * The type of the method handle will have a void return type. - * If the field is static, the method handle will take a single + * If the field is {@code static}, the method handle will take a single * argument, of the field's value type, the value to be stored. * Otherwise, the two arguments will be the instance containing * the field, and the value to be stored. - * If the field's {@code accessible} flag is not set, + * If the {@code Field} object's {@code accessible} flag is not set, * access checking is performed immediately on behalf of the lookup class. * <p> - * If the field is static, and + * If the field is {@code final}, write access will not be + * allowed and access checking will fail, except under certain + * narrow circumstances documented for {@link Field#set Field.set}. + * A method handle is returned only if a corresponding call to + * the {@code Field} object's {@code set} method could return + * normally. In particular, fields which are both {@code static} + * and {@code final} may never be set. + * <p> + * If the field is {@code static}, and * if the returned method handle is invoked, the field's class will * be initialized, if it has not already been initialized. * @param f the reflected field * @return a method handle which can store values into the reflected field - * @throws IllegalAccessException if access checking fails + * @throws IllegalAccessException if access checking fails, + * or if the field is {@code final} and write access + * is not enabled on the {@code Field} object * @throws NullPointerException if the argument is null */ public MethodHandle unreflectSetter(Field f) throws IllegalAccessException {
15-01-2019
Thanks John. I had interpreted "bytecode behaviour" to mean e.g. putfield/getfield/putstatic/getstatic in all cases but in fact the documentation states: lookup.unreflectSetter(aField) => aField.set(thisOrNull, arg); which means the returned MH should provide the same capabilities as the Field object with regards to setting final fields. That in turns means no setting of a static final field, but as you noted offline the downside there is that without that capability you can't use a MH to set a static final field in the <clinit> where it should be permitted (at least under certain constraints). So perhaps the spec should actually be relaxed to match the current behaviour.
15-01-2019
> If the field's {@code accessible} flag is not set, access checking is performed immediately on behalf of the lookup class. I still maintain that this refers only to actual accessibility checks NOT to the additional capability to write to a final field! There is a disconnect between what can be done via reflection and what can be done via bytecode, when it comes to final fields. If MH is intended to act like bytecode then it cannot allow setting of a final field (instance or static) unless in a context where the setting of a final field is permitted ie an <init> or <clinit> method. There is also ambiguity in the above in that if the setAccessible field is set then it is not clear (to me) that no access checking will be performed. I would also argue that again, because MH is supposed to act like bytecode, it would be wrong to suppress access checks in the MH just because they were supressed in the field! There is no mention of this capability in the MH specification that I can see.
14-01-2019
[~afarley] Regardless of how we implement it, your proposal is that `Lookup::unreflectSetter` will throw IAE if the specified field is a static field. The current specification says: > If the field's {@code accessible} flag is not set, access checking is performed immediately on behalf of the lookup class. The proposal behavior (throwing IAE) should be specified and a CSR needs to be filed. I will support this change of behavior because we should disallow changing the value of static final field via reflection and align with the existing behavior of `Field::set`. Ideally we would also want to disallow changing the value of instance final fields. Unfortunately there are existing libraries that depend on this long-standing behavior and using setAccessible to change instance final fields. A suggested spec update is: @@ -1876,10 +1877,13 @@ * the field, and the value to be stored. * If the field's {@code accessible} flag is not set, * access checking is performed immediately on behalf of the lookup class. + * If the field is static and final, access checking is performed + * regardless of the field's {@code accessible} flag value.
14-01-2019
I've fused my original fix with Mandy's, to minimise the domino effect here. Modifying the methods downstream increases the amount of code affected by this change, and makes it harder to be sure we've found prospective problems. Basically, the new change is this line: + if(isSetter && (field.isFinal() && field.isStatic())) throw field.makeAccessException("unexpected set of a static final field", this); With a comment addition to unreflectSetter. Webrev: http://cr.openjdk.java.net/~afarley/8216558/webrev/ Also, I've had a request for a testcase. A minimal testcase's code is attached to this bug.
14-01-2019
There seem to be a number of spec issues around this. Shouldn't findStaticSetter say something about what happens when the field is final? Same for findSetter? This issue seems to be much bigger than just a simple bug fix. A CSR request will need to be filed.
11-01-2019
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 @@ -1862,8 +1862,9 @@ assert(isSetter ? MethodHandleNatives.refKindIsSetter(field.getReferenceKind()) : MethodHandleNatives.refKindIsGetter(field.getReferenceKind())); + boolean isStaticFinal = field.isStatic() && field.isFinal(); @SuppressWarnings("deprecation") - Lookup lookup = f.isAccessible() ? IMPL_LOOKUP : this; + Lookup lookup = f.isAccessible() && !isStaticFinal ? IMPL_LOOKUP : this; return lookup.getDirectFieldNoSecurityManager(field.getReferenceKind(), f.getDeclaringClass(), field); } the IAE message created by checkAccess will also need to be amended to print "static final field"
11-01-2019
Field.setAccessible(true) on static final fields has no effect in suppressing the language access check. If this Field object is a static final field, Field::set throws IAE. I agree that MethodHandle::unreflectSetter(field) should also fail. This would be a behavioral change and also require a spec change.
11-01-2019
Webrev: http://cr.openjdk.java.net/~afarley/8216558/webrev/
11-01-2019