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

[Mandy Chung]

On 3/25/19 11:36 AM, Adam Farley8 wrote:
> Addendum: URL for new webrev: 
> http://cr.openjdk.java.net/~afarley/8216558.1/webrev/

Thanks for sending a versioned webrev.
> > What is a USA test?
>
> UNREFLECT_SETTER_ACCESSIBLE.
>
> I was trying to be brief, and I lost readability.
>
> Will re-upload with the expanded name.
>
> >
> > Why the assertion becomes conditional?
>
> Because we only need this check if the test is USA.
>
> Previously, setting a variable as accessible gave us a setter without 
> errors 100% of the time.
>
> The test expects this, and anticipates an empty list of variables 
> which threw errors when passed to unreflectSetter.
>
> Since we now throw an exception when calling the unreflectSetter 
> method, even when the variable is accessible, this check ensures that 
> the only variables throwing an exception are the static final fields.
>
> > 
> > Have you considered another way doing it?
>
> Yep, several. This seemed the simplest.
>
> Other options I considered were:
>
> --------------
>
> 1) Changing the test to store a list of variables, rather than just 
> their names, so the "isAccessible" method could be used instead.
>
> Discarded because it makes the array compare slower for every test.
>
> 2) Editing the array of anticipated excludeds during test execution, 
> to remove the exceptions we don't expect to see.
>
> Discarded because the change seemed too bulky to accommodate a single 
> test.
>
> 3) Make the "isAccessible" methods smarter, and rely on them 
> exclusively to determine the list of anticipated exceptions.
>
> Ditto
>
> ---------------------
>
> Option 3 could work. I prefer the one-liner stream check.
>
> Thoughts?


Each test case has a set of inaccessible fields under normal access checks.
For MH_UNREFLECT_GETTER_ACCESSIBLE and MH_UNREFLECT_SETTER_ACCESSIBLE
cases, before the patch, all fields are accessible as setAccessible(true)
is called on all fields.  That's the special casing for these cases and
expect actualFieldNames is empty.

This patch now changes the set of inaccessible fields for unreflectSetter
(static final fields are inaccessible).  A cleaner way to fix this is
to filter a given set of inaccessible fields and return the set of
inaccessible fields applicable to a FieldLookup case.

Attached is my suggested patch.  It adds a new 
FieldLookup::inaccessibeFields
method and the XXX_ACCESSIBLE Lookup overrides it to do the filtering.

Mandy


diff --git 
a/test/jdk/java/lang/invoke/VarHandles/accessibility/TestFieldLookupAccessibility.java 
b/test/jdk/java/lang/invoke/VarHandles/accessibility/TestFieldLookupAccessibility.java
--- 
a/test/jdk/java/lang/invoke/VarHandles/accessibility/TestFieldLookupAccessibility.java
+++ 
b/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 @@
              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 MethodHandler 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,25 @@
              }

              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 MethodHandler setters
+            Set<String> inaccessibleFields(Set<String> 
inaccessibleFields) {
+                Set<String> result = new HashSet<>();
+                inaccessibleFields.stream().filter(f -> 
f.contains("_static_")).forEach(result::add);
+                return result;
+            }
          },
          VH() {
              Object lookup(MethodHandles.Lookup l, Field f) throws 
Exception {
@@ -142,6 +160,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 +202,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)) {
@@ -203,12 +225,9 @@
          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) {
+                // all conditions for MethodHandler getters
                      Assert.assertEquals(actualFieldNames, expected, 
"No accessibility failures:");
                  }
-            }
              else {
                  Assert.assertEquals(actualFieldNames, expected, 
"Accessibility failures differ:");
              }