8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields
Reviewed-by: mchung
--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java Tue Mar 26 14:43:23 2019 -0700
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java Tue Mar 26 15:53:36 2019 -0700
@@ -422,6 +422,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:
@@ -1438,6 +1442,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
@@ -1561,6 +1566,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
@@ -1840,10 +1846,10 @@
* 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 @@
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()));
@@ -1868,28 +1909,6 @@
}
/**
- * 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}.
* The VarHandle's variable type is {@code T}.
--- a/test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java Tue Mar 26 14:43:23 2019 -0700
+++ b/test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java Tue Mar 26 15:53:36 2019 -0700
@@ -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 @@
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 @@
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 @@
? 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
- +" => "+mh
- +(noAccess == null ? "" : " !! "+noAccess));
+ System.out.println((((testMode0 & TEST_UNREFLECT) != 0)?"unreflect":"find")
+ +(((testMode0 & TEST_FIND_STATIC) != 0)?"Static":"")
+ +(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 @@
}
}
assertEquals(sawValue, expValue);
- if (f != null && f.getDeclaringClass() == HasFields.class
- && !Modifier.isFinal(f.getModifiers())) {
+ if (f != null && f.getDeclaringClass() == HasFields.class && !isFinal) {
Object random = randomArg(ftype);
f.set(fields, random);
expValue = random;
@@ -813,8 +819,8 @@
}
}
}
- if (f != null && f.getDeclaringClass() == HasFields.class) {
- f.set(fields, value); // put it back
+ if ((f != null) && (f.getDeclaringClass() == HasFields.class) && !isFinal) {
+ f.set(fields, value); // put it back if we changed it.
}
if (testNPE) {
if (caughtEx == null || !(caughtEx instanceof NullPointerException))
@@ -862,7 +868,6 @@
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);
--- a/test/jdk/java/lang/invoke/MethodHandlesTest.java Tue Mar 26 14:43:23 2019 -0700
+++ b/test/jdk/java/lang/invoke/MethodHandlesTest.java Tue Mar 26 15:53:36 2019 -0700
@@ -545,14 +545,14 @@
}
public static class HasFields {
- boolean fZ = false;
- byte fB = (byte)'B';
- short fS = (short)'S';
- char fC = 'C';
- int fI = 'I';
- long fJ = 'J';
- float fF = 'F';
- double fD = 'D';
+ boolean iZ = false;
+ byte iB = (byte)'B';
+ short iS = (short)'S';
+ char iC = 'C';
+ int iI = 'I';
+ long iJ = 'J';
+ float iF = 'F';
+ double iD = 'D';
static boolean sZ = true;
static byte sB = 1+(byte)'B';
static short sS = 1+(short)'S';
@@ -561,11 +561,21 @@
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';
- String fR = "R";
+ Object iL = 'L';
+ 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 @@
};
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 @@
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 });
}
}
--- a/test/jdk/java/lang/invoke/VarHandles/accessibility/TestFieldLookupAccessibility.java Tue Mar 26 14:43:23 2019 -0700
+++ b/test/jdk/java/lang/invoke/VarHandles/accessibility/TestFieldLookupAccessibility.java Tue Mar 26 15:53:36 2019 -0700
@@ -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 @@
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 @@
}
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 @@
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 @@
@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 @@
collect(Collectors.toSet());
if (!actualFieldNames.equals(expected)) {
if (actualFieldNames.isEmpty()) {
- // Setting the accessibility bit of a Field grants access under
- // 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:");
- }
+ Assert.assertEquals(actualFieldNames, expected, "No accessibility failures:");
}
else {
Assert.assertEquals(actualFieldNames, expected, "Accessibility failures differ:");