8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields

Reviewed-by: mchung

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

@@ -422,6 +422,10 @@ public class MethodHandles {
      * 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:
@@ -1438,6 +1442,7 @@ assertEquals(""+l, (String) MH_this.invokeExact(subl)); // Listie method
          * @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
@@ -1561,6 +1566,7 @@ assertEquals(""+l, (String) MH_this.invokeExact(subl)); // Listie method
          * @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
@@ -1840,10 +1846,10 @@ return mh1;
          * 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
@@ -1857,8 +1863,43 @@ return mh1;
         public MethodHandle unreflectGetter(Field f) throws IllegalAccessException {
             return unreflectField(f, false);
         }
+
+        /**
+         * 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 {@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 {@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 {@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,
+         *         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 {
+            return unreflectField(f, true);
+        }
+
         private MethodHandle unreflectField(Field f, boolean isSetter) throws IllegalAccessException {
             MemberName field = new MemberName(f, isSetter);
+            if (isSetter && field.isStatic() && field.isFinal())
+                throw field.makeAccessException("static final field has no write access", this);
             assert(isSetter
                     ? MethodHandleNatives.refKindIsSetter(field.getReferenceKind())
                     : MethodHandleNatives.refKindIsGetter(field.getReferenceKind()));
@@ -1867,28 +1908,6 @@ return mh1;
             return lookup.getDirectFieldNoSecurityManager(field.getReferenceKind(), f.getDeclaringClass(), field);
         }
 
-        /**
-         * 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
-         * 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,
-         * access checking is performed immediately on behalf of the lookup class.
-         * <p>
-         * If the field is 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 NullPointerException if the argument is null
-         */
-        public MethodHandle unreflectSetter(Field f) throws IllegalAccessException {
-            return unreflectField(f, true);
-        }
-
         /**
          * Produces a VarHandle giving access to a reflected field {@code f}
          * of type {@code T} declared in a class of type {@code R}.

test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java

@@ -22,6 +22,7 @@
  */
 
 /* @test
+ * @bug 8216558
  * @summary unit tests for java.lang.invoke.MethodHandles
  * @library /test/lib /java/lang/invoke/common
  * @compile MethodHandlesTest.java MethodHandlesGeneralTest.java remote/RemoteExample.java
@@ -664,6 +665,7 @@ public class MethodHandlesGeneralTest extends MethodHandlesTest {
         boolean testNPE  = ((testMode0 & TEST_NPE) != 0);
         int testMode = testMode0 & ~(TEST_SETTER | TEST_BOUND | TEST_NPE);
         boolean positive = positive0 && !testNPE;
+        boolean isFinal;
         boolean isStatic;
         Class<?> fclass;
         String   fname;
@@ -671,12 +673,14 @@ public class MethodHandlesGeneralTest extends MethodHandlesTest {
         Field f = (fieldRef instanceof Field ? (Field)fieldRef : null);
         if (f != null) {
             isStatic = Modifier.isStatic(f.getModifiers());
+            isFinal  = Modifier.isFinal(f.getModifiers());
             fclass   = f.getDeclaringClass();
             fname    = f.getName();
             ftype    = f.getType();
         } else {
             Object[] scnt = (Object[]) fieldRef;
             isStatic = (Boolean)  scnt[0];
+            isFinal  = false;
             fclass   = (Class<?>) scnt[1];
             fname    = (String)   scnt[2];
             ftype    = (Class<?>) scnt[3];
@@ -720,18 +724,21 @@ public class MethodHandlesGeneralTest extends MethodHandlesTest {
                 ?   NoSuchFieldException.class
                 :   IllegalAccessException.class,
                 noAccess);
+            if (((testMode0 & TEST_SETTER) != 0) && (isFinal && isStatic)) return; // Final static field setter test failed as intended.
             if (verbosity >= 5)  ex.printStackTrace(System.out);
         }
         if (verbosity >= 3)
-            System.out.println("find"+(isStatic?"Static":"")+(isGetter?"Getter":"Setter")+" "+fclass.getName()+"."+fname+"/"+ftype
+            System.out.println((((testMode0 & TEST_UNREFLECT) != 0)?"unreflect":"find")
-                               +" => "+mh
+                              +(((testMode0 & TEST_FIND_STATIC) != 0)?"Static":"")
-                               +(noAccess == null ? "" : " !! "+noAccess));
+                              +(isGetter?"Getter":"Setter")
+                              +" "+fclass.getName()+"."+fname+"/"+ftype
+                              +" => "+mh
+                              +(noAccess == null ? "" : " !! "+noAccess));
         if (positive && !testNPE && noAccess != null)  throw new RuntimeException(noAccess);
         assertEquals(positive0 ? "positive test" : "negative test erroneously passed", positive0, mh != null);
+        assertFalse("Setter methods should throw an exception if passed a final static field.", ((testMode0 & TEST_SETTER) != 0) && (isFinal && isStatic));
         if (!positive && !testNPE)  return; // negative access test failed as expected
         assertEquals((isStatic ? 0 : 1)+(isGetter ? 0 : 1), mh.type().parameterCount());
-
-
         assertSame(mh.type(), expType);
         //assertNameStringContains(mh, fname);  // This does not hold anymore with LFs
         HasFields fields = new HasFields();
@@ -778,8 +785,7 @@ public class MethodHandlesGeneralTest extends MethodHandlesTest {
                     }
                 }
                 assertEquals(sawValue, expValue);
-                if (f != null && f.getDeclaringClass() == HasFields.class
+                if (f != null && f.getDeclaringClass() == HasFields.class && !isFinal) {
-                    && !Modifier.isFinal(f.getModifiers())) {
                     Object random = randomArg(ftype);
                     f.set(fields, random);
                     expValue = random;
@@ -813,8 +819,8 @@ public class MethodHandlesGeneralTest extends MethodHandlesTest {
                 }
             }
         }
-        if (f != null && f.getDeclaringClass() == HasFields.class) {
+        if ((f != null) && (f.getDeclaringClass() == HasFields.class) && !isFinal) {
-            f.set(fields, value);  // put it back
+            f.set(fields, value);  // put it back if we changed it.
         }
         if (testNPE) {
             if (caughtEx == null || !(caughtEx instanceof NullPointerException))
@@ -862,7 +868,6 @@ public class MethodHandlesGeneralTest extends MethodHandlesTest {
 
     public void testSetter(int testMode) throws Throwable {
         Lookup lookup = PRIVATE;  // FIXME: test more lookups than this one
-        startTest("unreflectSetter");
         for (Object[] c : HasFields.CASES) {
             boolean positive = (c[1] != Error.class);
             testSetter(positive, lookup, c[0], c[1], testMode);

test/jdk/java/lang/invoke/MethodHandlesTest.java

@@ -545,14 +545,14 @@ public abstract class MethodHandlesTest {
     }
 
     public static class HasFields {
-        boolean fZ = false;
+        boolean iZ = false;
-        byte fB = (byte)'B';
+        byte iB = (byte)'B';
-        short fS = (short)'S';
+        short iS = (short)'S';
-        char fC = 'C';
+        char iC = 'C';
-        int fI = 'I';
+        int iI = 'I';
-        long fJ = 'J';
+        long iJ = 'J';
-        float fF = 'F';
+        float iF = 'F';
-        double fD = 'D';
+        double iD = 'D';
         static boolean sZ = true;
         static byte sB = 1+(byte)'B';
         static short sS = 1+(short)'S';
@@ -561,11 +561,21 @@ public abstract class MethodHandlesTest {
         static long sJ = 1+'J';
         static float sF = 1+'F';
         static double sD = 1+'D';
+        final static boolean fsZ = false;
+        final static byte fsB = 2+(byte)'B';
+        final static short fsS = 2+(short)'S';
+        final static char fsC = 2+'C';
+        final static int fsI = 2+'I';
+        final static long fsJ = 2+'J';
+        final static float fsF = 2+'F';
+        final static double fsD = 2+'D';
 
-        Object fL = 'L';
+        Object iL = 'L';
-        String fR = "R";
+        String iR = "R";
         static Object sL = 'M';
         static String sR = "S";
+        final static Object fsL = 'N';
+        final static String fsR = "T";
 
         static final Object[][] CASES;
         static {
@@ -579,14 +589,16 @@ public abstract class MethodHandlesTest {
             };
             HasFields fields = new HasFields();
             for (Object[] t : types) {
-                for (int kind = 0; kind <= 1; kind++) {
+                for (int kind = 0; kind <= 2; kind++) {
                     boolean isStatic = (kind != 0);
+                    boolean isFinal  = (kind == 2);
                     char btc = (Character)t[0];
-                    String name = (isStatic ? "s" : "f") + btc;
+                    String name = (isStatic ? "s" : "i") + btc;
+                    if (isFinal) name = "f" + name;
                     Class<?> type = (Class<?>) t[1];
                     Object value;
                     Field field;
-                        try {
+                    try {
                         field = HasFields.class.getDeclaredField(name);
                     } catch (NoSuchFieldException | SecurityException ex) {
                         throw new InternalError("no field HasFields."+name);
@@ -599,11 +611,14 @@ public abstract class MethodHandlesTest {
                     if (type == float.class) {
                         float v = 'F';
                         if (isStatic)  v++;
+                        if (isFinal)   v++;
                         assertTrue(value.equals(v));
                     }
+                    if (isFinal && isStatic) field.setAccessible(true);
                     assertTrue(name.equals(field.getName()));
                     assertTrue(type.equals(field.getType()));
                     assertTrue(isStatic == (Modifier.isStatic(field.getModifiers())));
+                    assertTrue(isFinal  == (Modifier.isFinal(field.getModifiers())));
                     cases.add(new Object[]{ field, value });
                 }
             }

test/jdk/java/lang/invoke/VarHandles/accessibility/TestFieldLookupAccessibility.java

@@ -22,7 +22,7 @@
  */
 
 /* @test
- * @bug 8152645
+ * @bug 8152645 8216558
  * @summary test field lookup accessibility of MethodHandles and VarHandles
  * @compile TestFieldLookupAccessibility.java
  *          pkg/A.java pkg/B_extends_A.java pkg/C.java
@@ -96,6 +96,12 @@ public class TestFieldLookupAccessibility {
             Object lookup(MethodHandles.Lookup l, Field f) throws Exception {
                 return l.unreflectGetter(cloneAndSetAccessible(f));
             }
+
+            // Setting the accessibility bit of a Field grants access under
+            // all conditions for MethodHandle getters.
+            Set<String> inaccessibleFields(Set<String> inaccessibleFields) {
+                return new HashSet<>();
+            }
         },
         MH_UNREFLECT_SETTER() {
             Object lookup(MethodHandles.Lookup l, Field f) throws Exception {
@@ -103,13 +109,27 @@ public class TestFieldLookupAccessibility {
             }
 
             boolean isAccessible(Field f) {
-                return f.isAccessible() || !Modifier.isFinal(f.getModifiers());
+                return f.isAccessible() && !Modifier.isStatic(f.getModifiers()) || !Modifier.isFinal(f.getModifiers());
             }
         },
         MH_UNREFLECT_SETTER_ACCESSIBLE() {
             Object lookup(MethodHandles.Lookup l, Field f) throws Exception {
                 return l.unreflectSetter(cloneAndSetAccessible(f));
             }
+
+            boolean isAccessible(Field f) {
+                return !(Modifier.isStatic(f.getModifiers()) && Modifier.isFinal(f.getModifiers()));
+            }
+
+            // Setting the accessibility bit of a Field grants access to non-static
+            // final fields for MethodHandle setters.
+            Set<String> inaccessibleFields(Set<String>inaccessibleFields) {
+                Set<String> result = new HashSet<>();
+                inaccessibleFields.stream()
+                                  .filter(f -> (f.contains("static") && f.contains("final")))
+                                  .forEach(result::add);
+                return result;
+            }
         },
         VH() {
             Object lookup(MethodHandles.Lookup l, Field f) throws Exception {
@@ -142,6 +162,10 @@ public class TestFieldLookupAccessibility {
             return true;
         }
 
+        Set<String> inaccessibleFields(Set<String> inaccessibleFields) {
+            return new HashSet<>(inaccessibleFields);
+        }
+
         static Field cloneAndSetAccessible(Field f) throws Exception {
             // Clone to avoid mutating source field
             f = f.getDeclaringClass().getDeclaredField(f.getName());
@@ -180,7 +204,7 @@ public class TestFieldLookupAccessibility {
     @Test(dataProvider = "lookupProvider")
     public void test(FieldLookup fl, Class<?> src, MethodHandles.Lookup l, Set<String> inaccessibleFields) {
         // Add to the expected failures all inaccessible fields due to accessibility modifiers
-        Set<String> expected = new HashSet<>(inaccessibleFields);
+        Set<String> expected = fl.inaccessibleFields(inaccessibleFields);
         Map<Field, Throwable> actual = new HashMap<>();
 
         for (Field f : fields(src)) {
@@ -202,12 +226,7 @@ public class TestFieldLookupAccessibility {
                 collect(Collectors.toSet());
         if (!actualFieldNames.equals(expected)) {
             if (actualFieldNames.isEmpty()) {
-                // Setting the accessibility bit of a Field grants access under
+                Assert.assertEquals(actualFieldNames, expected, "No accessibility failures:");
-                // all conditions for MethodHander getters and setters
-                if (fl != FieldLookup.MH_UNREFLECT_GETTER_ACCESSIBLE &&
-                    fl != FieldLookup.MH_UNREFLECT_SETTER_ACCESSIBLE) {
-                    Assert.assertEquals(actualFieldNames, expected, "No accessibility failures:");
-                }
             }
             else {
                 Assert.assertEquals(actualFieldNames, expected, "Accessibility failures differ:");