JDK-8218061 : Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for static final field
  • Type: CSR
  • Component: core-libs
  • Sub-Component: java.lang.invoke
  • Priority: P4
  • Status: Closed
  • Resolution: Approved
  • Fix Versions: 13
  • Submitted: 2019-01-30
  • Updated: 2019-03-28
  • Resolved: 2019-03-20
Related Reports
CSR :  
Description
Summary
-------

Lookup.unreflectSetter(Field) will now throw an IllegalAccessException for static final fields, regardless of whether setAccessible(boolean) has been set to true.

Problem
-------

The problem is that, if setAccessible(boolean) has been set to true for a static final field, you can use Lookup.unreflectSetter(Field) to obtain a setter MethodHandle for the field.  The original intent was to allow write access only if `Field::set` allows.  This is correct behaviour for a final field, but not a field which is both static and final.  Field::set does not allow setting a static final field even its accessible flag is set.   

A test case illustrating the bug will be attached, for clarity.

Solution
--------

Change `Lookup.unreflectSetter(Field)` to throw IAE if the specified `Field` object is a static final field.  Also clarify the specification of field setter functions `findSetter`, `findStaticSetter`, `unreflectSetter` w.r.t. access control on a final field and the special case of Field object.

Proposed solution, includes a code change (a simple check on the field for static and final, followed by a throw) and some comment changes to improve spec clarity. They can be found on the web, here:

http://hg.openjdk.java.net/jdk/jdk/rev/49c4b23d8d0a

The method ordering change, and the annotation additions, should not affect functionality, and are purely for readability.

Specification
-------------

The detailed changes can be found in the webrev. The parts of the webrev relevant to this CSR are:
- The behavioral change caused by the static final field check+throw
- The comment changes clarifying what the code will do.

The full webrev zip containing the current set of proposed changes will be attached to this issue.

Spec Changes
-------------

The spec in `java.lang.invoke.MethodHandles.Lookup` class, `Lookup::findSetter`,
`Lookup::fndStaticSetter` and `Lookup::unreflectSetter` are updated to clarify
that IAE will be thrown if the field has no write access.

src/java.base/share/classes/java/lang/invoke/MethodHandles.java
```
@@ -420,10 +420,14 @@
      * A lookup can fail, because
      * the containing class is not accessible to the lookup class, or
      * 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:
      * <ul>
      * <li>NoSuchMethodException &mdash; if a method is requested but does not exist

@@ -1436,10 +1440,11 @@
          * @param name the field's name
          * @param type the field's type
          * @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
          * @see #findVarHandle(Class, String, Class)
          */

@@ -1559,10 +1564,11 @@
          * @param name the field's name
          * @param type the field's type
          * @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
          */
         public MethodHandle findStaticSetter(Class<?> refc, String name, Class<?> type) throws NoSuchFieldException, IllegalAccessException {

@@ -1838,36 +1844,37 @@
 
         /**
          * Produces a method handle giving read access to a reflected field.
          * The type of the method handle will have a return type of the field's
          * value type.
-         * If the field is static, the method handle will take no arguments.
+         * If the field is {@code static}, the method handle will take no arguments.
          * Otherwise, its single argument will be the instance containing
          * the field.
-         * 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 load values from 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
          */
```
...Further changes for this file are code only, and can be found in the webrev...
Comments
The parent issue is resolved, and a release note has been created. It is my first release note, so it could be worth reviewing it and making sure it was done correctly.
28-03-2019

Moving to Approved. Please consider a release note for the parent issue.
20-03-2019

I think we're good to go on the full changeset. Are there additional actions required to progress the CSR?
08-03-2019

[~dholmes] Yes. The javadoc change in Lookup.findSetter and findStaticSetter is solely spec clarification. Note that not MethodHandle.findSetter.
21-02-2019

The only behavioral change in this changeset is to MethodHandle.unreflectSetter(), plus the MethodHandle test class. MethodHandle.findSetter has no code changes.
21-02-2019

Is the change to MethodHandle.findSetter simply documenting existing behaviour?
21-02-2019

[~jrose] and [~dholmes], since you were involved with discussions on the bug, please review this CSR. Thanks.
19-02-2019

[~afarley] I suggest to cut-n-paste the diff (javadoc chanage) into the specification section. The spec diff is small enough that it can be inlined in the specification section. The code change is not necessary for CSR review and so inlining the spec change in the specification section will also save you the step to upload the webrev.zip on any revision.
31-01-2019