8202113: Reflection API is causing caller classes to leak
authormchung
Fri, 11 May 2018 14:21:46 -0700
changeset 50091 05979f6ba560
parent 50090 94e11b6edcdd
child 50092 0e42d3120e51
child 56543 2352538d2f6e
8202113: Reflection API is causing caller classes to leak Reviewed-by: alanb, plevart
src/java.base/share/classes/java/lang/reflect/AccessibleObject.java
src/java.base/share/classes/java/lang/reflect/Constructor.java
src/java.base/share/classes/java/lang/reflect/Executable.java
src/java.base/share/classes/java/lang/reflect/Field.java
src/java.base/share/classes/java/lang/reflect/Method.java
src/java.base/share/classes/java/lang/reflect/ReflectAccess.java
src/java.base/share/classes/jdk/internal/reflect/LangReflectAccess.java
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java
test/jdk/java/lang/reflect/callerCache/AccessTest.java
test/jdk/java/lang/reflect/callerCache/Members.java
test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java
--- a/src/java.base/share/classes/java/lang/reflect/AccessibleObject.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/java/lang/reflect/AccessibleObject.java	Fri May 11 14:21:46 2018 -0700
@@ -564,7 +564,6 @@
         throw new AssertionError("All subclasses should override this method");
     }
 
-
     // Shared access checking logic.
 
     // For non-public members or members in package-private classes,
@@ -674,4 +673,13 @@
         }
         return printStackWhenAccessFails;
     }
+
+    /**
+     * Returns the root AccessibleObject; or null if this object is the root.
+     *
+     * All subclasses override this method.
+     */
+    AccessibleObject getRoot() {
+        throw new InternalError();
+    }
 }
--- a/src/java.base/share/classes/java/lang/reflect/Constructor.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/java/lang/reflect/Constructor.java	Fri May 11 14:21:46 2018 -0700
@@ -103,11 +103,8 @@
     // occur in annotation code.
     private Constructor<T>      root;
 
-    /**
-     * Used by Excecutable for annotation sharing.
-     */
     @Override
-    Executable getRoot() {
+    Constructor<T> getRoot() {
         return root;
     }
 
--- a/src/java.base/share/classes/java/lang/reflect/Executable.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/java/lang/reflect/Executable.java	Fri May 11 14:21:46 2018 -0700
@@ -56,11 +56,6 @@
     abstract byte[] getAnnotationBytes();
 
     /**
-     * Accessor method to allow code sharing
-     */
-    abstract Executable getRoot();
-
-    /**
      * Does the Executable have generic information.
      */
     abstract boolean hasGenericInformation();
@@ -602,7 +597,7 @@
         if ((declAnnos = declaredAnnotations) == null) {
             synchronized (this) {
                 if ((declAnnos = declaredAnnotations) == null) {
-                    Executable root = getRoot();
+                    Executable root = (Executable)getRoot();
                     if (root != null) {
                         declAnnos = root.declaredAnnotations();
                     } else {
--- a/src/java.base/share/classes/java/lang/reflect/Field.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/java/lang/reflect/Field.java	Fri May 11 14:21:46 2018 -0700
@@ -1128,6 +1128,11 @@
         }
     }
 
+    @Override
+    Field getRoot() {
+        return root;
+    }
+
     /**
      * @throws NullPointerException {@inheritDoc}
      * @since 1.5
--- a/src/java.base/share/classes/java/lang/reflect/Method.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/java/lang/reflect/Method.java	Fri May 11 14:21:46 2018 -0700
@@ -198,11 +198,8 @@
         checkCanSetAccessible(caller, clazz);
     }
 
-    /**
-     * Used by Excecutable for annotation sharing.
-     */
     @Override
-    Executable getRoot() {
+    Method getRoot() {
         return root;
     }
 
--- a/src/java.base/share/classes/java/lang/reflect/ReflectAccess.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/java/lang/reflect/ReflectAccess.java	Fri May 11 14:21:46 2018 -0700
@@ -154,4 +154,9 @@
     public <T> Constructor<T> copyConstructor(Constructor<T> arg) {
         return arg.copy();
     }
+
+    @SuppressWarnings("unchecked")
+    public <T extends AccessibleObject> T getRoot(T obj) {
+        return (T) obj.getRoot();
+    }
 }
--- a/src/java.base/share/classes/jdk/internal/reflect/LangReflectAccess.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/jdk/internal/reflect/LangReflectAccess.java	Fri May 11 14:21:46 2018 -0700
@@ -115,4 +115,7 @@
 
     /** Makes a "child" copy of a Constructor */
     public <T> Constructor<T> copyConstructor(Constructor<T> arg);
+
+    /** Gets the root of the given AccessibleObject object; null if arg is the root */
+    public <T extends AccessibleObject> T getRoot(T obj);
 }
--- a/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java	Thu May 10 13:34:44 2018 -0700
+++ b/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java	Fri May 11 14:21:46 2018 -0700
@@ -39,7 +39,6 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Modifier;
-import java.security.Permission;
 import java.security.PrivilegedAction;
 import java.util.Objects;
 import java.util.Properties;
@@ -172,6 +171,15 @@
      */
     public FieldAccessor newFieldAccessor(Field field, boolean override) {
         checkInitted();
+
+        Field root = langReflectAccess.getRoot(field);
+        if (root != null) {
+            // FieldAccessor will use the root unless the modifiers have
+            // been overrridden
+            if (root.getModifiers() == field.getModifiers() || !override) {
+                field = root;
+            }
+        }
         return UnsafeFieldAccessorFactory.newFieldAccessor(field, override);
     }
 
@@ -185,6 +193,12 @@
             }
         }
 
+        // use the root Method that will not cache caller class
+        Method root = langReflectAccess.getRoot(method);
+        if (root != null) {
+            method = root;
+        }
+
         if (noInflation && !ReflectUtil.isVMAnonymousClass(method.getDeclaringClass())) {
             return new MethodAccessorGenerator().
                 generateMethod(method.getDeclaringClass(),
@@ -214,6 +228,13 @@
             return new InstantiationExceptionConstructorAccessorImpl
                 ("Can not instantiate java.lang.Class");
         }
+
+        // use the root Constructor that will not cache caller class
+        Constructor<?> root = langReflectAccess.getRoot(c);
+        if (root != null) {
+            c = root;
+        }
+
         // Bootstrapping issue: since we use Class.newInstance() in
         // the ConstructorAccessor generation process, we have to
         // break the cycle here.
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/lang/reflect/callerCache/AccessTest.java	Fri May 11 14:21:46 2018 -0700
@@ -0,0 +1,171 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.concurrent.Callable;
+
+/**
+ * Each nested class tests a member of a specific access.
+ *
+ * Caller class is cached when setAccessible is not used to suppress the access
+ * check and only access allowed.  If not accessible, caller class is not cached.
+ */
+public class AccessTest {
+    public static class PublicConstructor implements Callable<Object> {
+        public Object call() throws Exception {
+            Constructor c = Members.class.getConstructor();
+            return c.newInstance();
+        }
+    }
+
+    public static class PublicMethod extends Members implements Callable<Void> {
+        public Void call() throws Exception {
+            Method m = Members.class.getDeclaredMethod("publicMethod");
+            m.invoke(new PublicMethod());
+            return null;
+        }
+    }
+
+    public static class ProtectedMethod extends Members implements Callable<Void> {
+        public Void call() throws Exception {
+            Method m = Members.class.getDeclaredMethod("protectedMethod");
+            m.invoke(new ProtectedMethod());
+            return null;
+        }
+    }
+
+    /*
+     * private field is not accessible.  So caller class is not cached.
+     */
+    public static class PrivateMethod extends Members implements Callable<Void> {
+        public Void call() throws Exception {
+            Method m = Members.class.getDeclaredMethod("privateMethod");
+            try {
+                m.invoke(new ProtectedMethod());
+            } catch (IllegalAccessException e) {
+            }
+            return null;
+        }
+    }
+
+    public static class PublicField extends Members implements Callable<Void> {
+        public Void call() throws Exception {
+            Field f = Members.class.getDeclaredField("publicField");
+            f.get(new PublicField());
+            return null;
+        }
+    }
+
+    public static class ProtectedField extends Members implements Callable<Void> {
+        public Void call() throws Exception {
+            Field f = Members.class.getDeclaredField("protectedField");
+            f.get(new ProtectedField());
+            return null;
+        }
+    }
+
+    /*
+     * private field is not accessible.  So caller class is not cached.
+     */
+    public static class PrivateField implements Callable<Void> {
+        public Void call() throws Exception {
+            Field f = Members.class.getDeclaredField("privateField");
+            try {
+                f.get(new Members());
+            } catch (IllegalAccessException e) {
+            }
+            return null;
+        }
+    }
+
+    /*
+     * Validate final field
+     */
+    public static class FinalField implements Callable<Void> {
+        final Field f;
+        final boolean isStatic;
+        public FinalField(String name) throws Exception {
+            this.f = Members.class.getDeclaredField(name);
+            this.isStatic = Modifier.isStatic(f.getModifiers());
+            if (!Modifier.isFinal(f.getModifiers())) {
+                throw new RuntimeException("not a final field");
+            }
+            makeFinalNonFinal(f);
+        }
+        public Void call() throws Exception {
+            Members obj = isStatic ? null : new Members();
+            try {
+                f.set(obj, 20);
+                checkValue(obj, 20);
+            } catch (IllegalAccessException e) {
+                throw e;
+            }
+            return null;
+        }
+
+        void checkValue(Object obj, int expected) throws Exception {
+            int value = (int) f.get(obj);
+            if (value != expected) {
+                throw new RuntimeException("unexpectd value: " + value);
+            }
+        }
+    }
+
+    public static class PublicFinalField extends FinalField {
+        public PublicFinalField() throws Exception {
+            super("publicFinalField");
+        }
+    }
+
+    public static class PrivateFinalField extends FinalField {
+        public PrivateFinalField() throws Exception {
+            super("privateFinalField");
+        }
+    }
+
+    public static class PublicStaticFinalField extends FinalField {
+        public PublicStaticFinalField() throws Exception {
+            super("publicStaticFinalField");
+        }
+    }
+
+    public static class PrivateStaticFinalField extends FinalField {
+        public PrivateStaticFinalField() throws Exception {
+            super("privateStaticFinalField");
+        }
+    }
+
+    private static void makeFinalNonFinal(Field f) throws ReflectiveOperationException {
+        Field modifiers = Field.class.getDeclaredField("modifiers");
+        modifiers.setAccessible(true);
+        modifiers.set(f, modifiers.getInt(f) & ~Modifier.FINAL);
+        f.setAccessible(true);
+
+        if (Modifier.isFinal(f.getModifiers())) {
+            throw new RuntimeException("should be a non-final field");
+        }
+    }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/lang/reflect/callerCache/Members.java	Fri May 11 14:21:46 2018 -0700
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+public class Members {
+    public Members() {}
+    protected Members(boolean b) {}
+    private Members(int i) {}
+
+    public void publicMethod() {}
+    protected void protectedMethod() {}
+    private void privateMethod() {}
+
+    public Object publicField = new Object();
+    protected Object protectedField = new Object();
+    private Object privateField = new Object();
+
+    public final int publicFinalField = 10;
+    private final int privateFinalField = 10;
+    public static final int publicStaticFinalField = 10;
+    private static final int privateStaticFinalField = 10;
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java	Fri May 11 14:21:46 2018 -0700
@@ -0,0 +1,175 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8202113
+ * @summary Test the caller class loader is not kept strongly reachable
+  *         by reflection API
+ * @library /test/lib/
+ * @build ReflectionCallerCacheTest Members jdk.test.lib.compiler.CompilerUtils
+ * @run testng/othervm ReflectionCallerCacheTest
+ */
+
+import java.io.IOException;
+import java.lang.ref.Cleaner;
+import java.lang.ref.WeakReference;
+import java.lang.reflect.*;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.function.BooleanSupplier;
+
+import jdk.test.lib.compiler.CompilerUtils;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ReflectionCallerCacheTest {
+    private static final Path CLASSES = Paths.get("classes");
+    private static final ReflectionCallerCacheTest TEST = new ReflectionCallerCacheTest();
+
+    @BeforeTest
+    public void setup() throws IOException {
+        String src = System.getProperty("test.src", ".");
+        String classpath = System.getProperty("test.classes", ".");
+        boolean rc = CompilerUtils.compile(Paths.get(src, "AccessTest.java"), CLASSES, "-cp", classpath);
+        if (!rc) {
+            throw new RuntimeException("fail compilation");
+        }
+    }
+    @DataProvider(name = "memberAccess")
+    public Object[][] memberAccess() {
+        return new Object[][] {
+            { "AccessTest$PublicConstructor" },
+            { "AccessTest$PublicMethod" },
+            { "AccessTest$PublicField" },
+            { "AccessTest$ProtectedMethod" },
+            { "AccessTest$ProtectedField" },
+            { "AccessTest$PrivateMethod" },
+            { "AccessTest$PrivateField"},
+            { "AccessTest$PublicFinalField"},
+            { "AccessTest$PrivateFinalField"},
+            { "AccessTest$PublicStaticFinalField"},
+            { "AccessTest$PrivateStaticFinalField"}
+        };
+    }
+
+    // Keep the root of the reflective objects strongly reachable
+    private final Constructor<?> publicConstructor;
+    private final Method publicMethod;
+    private final Method protectedMethod;
+    private final Method privateMethod;
+    private final Field publicField;
+    private final Field protectedField;
+    private final Field privateField;
+
+    ReflectionCallerCacheTest() {
+        try {
+            this.publicConstructor = Members.class.getConstructor();
+            this.publicMethod = Members.class.getDeclaredMethod("publicMethod");
+            this.publicField = Members.class.getDeclaredField("publicField");
+            this.protectedMethod = Members.class.getDeclaredMethod("protectedMethod");
+            this.protectedField = Members.class.getDeclaredField("protectedField");
+            this.privateMethod = Members.class.getDeclaredMethod("privateMethod");
+            this.privateField = Members.class.getDeclaredField("privateField");
+        } catch (ReflectiveOperationException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    @Test(dataProvider = "memberAccess")
+    private void load(String classname) throws Exception {
+        WeakReference<?> weakLoader = loadAndRunClass(classname);
+
+        // Force garbage collection to trigger unloading of class loader
+        new ForceGC().await(() -> weakLoader.get() == null);
+
+        if (weakLoader.get() != null) {
+            throw new RuntimeException("Class " + classname + " not unloaded!");
+        }
+    }
+
+    private WeakReference<?> loadAndRunClass(String classname) throws Exception {
+        try (TestLoader loader = new TestLoader()) {
+            // Load member access class with custom class loader
+            Class<?> c = Class.forName(classname, true, loader);
+            // access the reflective member
+            Callable callable = (Callable) c.newInstance();
+            callable.call();
+            return new WeakReference<>(loader);
+        }
+    }
+
+    static class TestLoader extends URLClassLoader {
+        static URL[] toURLs() {
+            try {
+                return new URL[] { CLASSES.toUri().toURL() };
+            } catch (MalformedURLException e) {
+                throw new Error(e);
+            }
+        }
+
+        TestLoader() {
+            super("testloader", toURLs(), ClassLoader.getSystemClassLoader());
+        }
+    }
+
+    /**
+     * Utility class to invoke System.gc()
+     */
+    static class ForceGC {
+        private  final CountDownLatch cleanerInvoked = new CountDownLatch(1);
+        private  final Cleaner cleaner = Cleaner.create();
+
+        ForceGC() {
+            cleaner.register(new Object(), () -> cleanerInvoked.countDown());
+        }
+
+        void doit() {
+            try {
+                for (int i = 0; i < 10; i++) {
+                    System.gc();
+                    if (cleanerInvoked.await(1L, TimeUnit.SECONDS)) {
+                        return;
+                    }
+                }
+            } catch (InterruptedException unexpected) {
+                throw new AssertionError("unexpected InterruptedException");
+            }
+        }
+
+        void await(BooleanSupplier s) {
+            for (int i = 0; i < 10; i++) {
+                if (s.getAsBoolean()) return;
+                doit();
+            }
+            throw new AssertionError("failed to satisfy condition");
+        }
+    }
+}