8206240: java.lang.Class.newInstance() is causing caller to leak

Reviewed-by: alanb

src/java.base/share/classes/java/lang/Class.java

@@ -64,7 +64,6 @@ import jdk.internal.HotSpotIntrinsicCandidate;
 import jdk.internal.loader.BootLoader;
 import jdk.internal.loader.BuiltinClassLoader;
 import jdk.internal.misc.Unsafe;
-import jdk.internal.misc.VM;
 import jdk.internal.module.Resources;
 import jdk.internal.reflect.CallerSensitive;
 import jdk.internal.reflect.ConstantPool;
@@ -540,11 +539,9 @@ public final class Class<T> implements java.io.Serializable,
             checkMemberAccess(sm, Member.PUBLIC, Reflection.getCallerClass(), false);
         }
 
-        // NOTE: the following code may not be strictly correct under
-        // the current Java memory model.
-
         // Constructor lookup
-        if (cachedConstructor == null) {
+        Constructor<T> tmpConstructor = cachedConstructor;
+        if (tmpConstructor == null) {
             if (this == Class.class) {
                 throw new IllegalAccessException(
                     "Can not call newInstance() on the Class for java.lang.Class"
@@ -555,9 +552,7 @@ public final class Class<T> implements java.io.Serializable,
                 final Constructor<T> c = getReflectionFactory().copyConstructor(
                     getConstructor0(empty, Member.DECLARED));
                 // Disable accessibility checks on the constructor
-                // since we have to do the security check here anyway
+                // access check is done with the true caller
-                // (the stack depth is wrong for the Constructor's
-                // security check to work)
                 java.security.AccessController.doPrivileged(
                     new java.security.PrivilegedAction<>() {
                         public Void run() {
@@ -565,32 +560,24 @@ public final class Class<T> implements java.io.Serializable,
                                 return null;
                             }
                         });
-                cachedConstructor = c;
+                cachedConstructor = tmpConstructor = c;
             } catch (NoSuchMethodException e) {
                 throw (InstantiationException)
                     new InstantiationException(getName()).initCause(e);
             }
         }
-        Constructor<T> tmpConstructor = cachedConstructor;
+
-        // Security check (same as in java.lang.reflect.Constructor)
-        Class<?> caller = Reflection.getCallerClass();
-        if (newInstanceCallerCache != caller) {
-            int modifiers = tmpConstructor.getModifiers();
-            Reflection.ensureMemberAccess(caller, this, this, modifiers);
-            newInstanceCallerCache = caller;
-        }
-        // Run constructor
         try {
-            return tmpConstructor.newInstance((Object[])null);
+            Class<?> caller = Reflection.getCallerClass();
+            return getReflectionFactory().newInstance(tmpConstructor, null, caller);
         } catch (InvocationTargetException e) {
             Unsafe.getUnsafe().throwException(e.getTargetException());
             // Not reached
             return null;
         }
     }
-    private transient volatile Constructor<T> cachedConstructor;
-    private transient volatile Class<?>       newInstanceCallerCache;
 
+    private transient volatile Constructor<T> cachedConstructor;
 
     /**
      * Determines if the specified {@code Object} is assignment-compatible

src/java.base/share/classes/java/lang/reflect/AccessibleObject.java

@@ -27,6 +27,7 @@ package java.lang.reflect;
 
 import java.lang.annotation.Annotation;
 import java.lang.invoke.MethodHandle;
+import java.lang.ref.WeakReference;
 import java.security.AccessController;
 
 import jdk.internal.misc.VM;
@@ -567,21 +568,68 @@ public class AccessibleObject implements AnnotatedElement {
     // Shared access checking logic.
 
     // For non-public members or members in package-private classes,
-    // it is necessary to perform somewhat expensive security checks.
+    // it is necessary to perform somewhat expensive access checks.
-    // If the security check succeeds for a given class, it will
+    // If the access check succeeds for a given class, it will
     // always succeed (it is not affected by the granting or revoking
     // of permissions); we speed up the check in the common case by
     // remembering the last Class for which the check succeeded.
     //
-    // The simple security check for Constructor is to see if
+    // The simple access check for Constructor is to see if
     // the caller has already been seen, verified, and cached.
-    // (See also Class.newInstance(), which uses a similar method.)
     //
-    // A more complicated security check cache is needed for Method and Field
+    // A more complicated access check cache is needed for Method and Field
-    // The cache can be either null (empty cache), a 2-array of {caller,targetClass},
+    // The cache can be either null (empty cache), {caller,targetClass} pair,
     // or a caller (with targetClass implicitly equal to memberClass).
-    // In the 2-array case, the targetClass is always different from the memberClass.
+    // In the {caller,targetClass} case, the targetClass is always different
-    volatile Object securityCheckCache;
+    // from the memberClass.
+    volatile Object accessCheckCache;
+
+    private static class Cache {
+        final WeakReference<Class<?>> callerRef;
+        final WeakReference<Class<?>> targetRef;
+
+        Cache(Class<?> caller, Class<?> target) {
+            this.callerRef = new WeakReference<>(caller);
+            this.targetRef = new WeakReference<>(target);
+        }
+
+        boolean isCacheFor(Class<?> caller, Class<?> refc) {
+            return callerRef.get() == caller && targetRef.get() == refc;
+        }
+
+        static Object protectedMemberCallerCache(Class<?> caller, Class<?> refc) {
+            return new Cache(caller, refc);
+        }
+    }
+
+    /*
+     * Returns true if the previous access check was verified for the
+     * given caller accessing a protected member with an instance of
+     * the given targetClass where the target class is different than
+     * the declaring member class.
+     */
+    private boolean isAccessChecked(Class<?> caller, Class<?> targetClass) {
+        Object cache = accessCheckCache;  // read volatile
+        if (cache instanceof Cache) {
+            return ((Cache) cache).isCacheFor(caller, targetClass);
+        }
+        return false;
+    }
+
+    /*
+     * Returns true if the previous access check was verified for the
+     * given caller accessing a static member or an instance member of
+     * the target class that is the same as the declaring member class.
+     */
+    private boolean isAccessChecked(Class<?> caller) {
+        Object cache = accessCheckCache;  // read volatile
+        if (cache instanceof WeakReference) {
+            @SuppressWarnings("unchecked")
+            WeakReference<Class<?>> ref = (WeakReference<Class<?>>) cache;
+            return ref.get() == caller;
+        }
+        return false;
+    }
 
     final void checkAccess(Class<?> caller, Class<?> memberClass,
                            Class<?> targetClass, int modifiers)
@@ -603,21 +651,13 @@ public class AccessibleObject implements AnnotatedElement {
         if (caller == memberClass) {  // quick check
             return true;             // ACCESS IS OK
         }
-        Object cache = securityCheckCache;  // read volatile
         if (targetClass != null // instance member or constructor
             && Modifier.isProtected(modifiers)
             && targetClass != memberClass) {
-            // Must match a 2-list of { caller, targetClass }.
+            if (isAccessChecked(caller, targetClass)) {
-            if (cache instanceof Class[]) {
+                return true;         // ACCESS IS OK
-                Class<?>[] cache2 = (Class<?>[]) cache;
-                if (cache2[1] == targetClass &&
-                    cache2[0] == caller) {
-                    return true;     // ACCESS IS OK
-                }
-                // (Test cache[1] first since range check for [1]
-                // subsumes range check for [0].)
             }
-        } else if (cache == caller) {
+        } else if (isAccessChecked(caller)) {
             // Non-protected case (or targetClass == memberClass or static member).
             return true;             // ACCESS IS OK
         }
@@ -642,14 +682,9 @@ public class AccessibleObject implements AnnotatedElement {
         Object cache = (targetClass != null
                         && Modifier.isProtected(modifiers)
                         && targetClass != memberClass)
-                        ? new Class<?>[] { caller, targetClass }
+                        ? Cache.protectedMemberCallerCache(caller, targetClass)
-                        : caller;
+                        : new WeakReference<>(caller);
-
+        accessCheckCache = cache;         // write volatile
-        // Note:  The two cache elements are not volatile,
-        // but they are effectively final.  The Java memory model
-        // guarantees that the initializing stores for the cache
-        // elements will occur before the volatile write.
-        securityCheckCache = cache;         // write volatile
         return true;
     }
 

src/java.base/share/classes/java/lang/reflect/Constructor.java

@@ -476,18 +476,27 @@ public final class Constructor<T> extends Executable {
         throws InstantiationException, IllegalAccessException,
                IllegalArgumentException, InvocationTargetException
     {
-        if (!override) {
+        Class<?> caller = override ? null : Reflection.getCallerClass();
-            Class<?> caller = Reflection.getCallerClass();
+        return newInstanceWithCaller(initargs, !override, caller);
+    }
+
+    /* package-private */
+    T newInstanceWithCaller(Object[] args, boolean checkAccess, Class<?> caller)
+        throws InstantiationException, IllegalAccessException,
+               InvocationTargetException
+    {
+        if (checkAccess)
             checkAccess(caller, clazz, clazz, modifiers);
-        }
+
         if ((clazz.getModifiers() & Modifier.ENUM) != 0)
             throw new IllegalArgumentException("Cannot reflectively create enum objects");
+
         ConstructorAccessor ca = constructorAccessor;   // read volatile
         if (ca == null) {
             ca = acquireConstructorAccessor();
         }
         @SuppressWarnings("unchecked")
-        T inst = (T) ca.newInstance(initargs);
+        T inst = (T) ca.newInstance(args);
         return inst;
     }
 

src/java.base/share/classes/java/lang/reflect/ReflectAccess.java

@@ -159,4 +159,10 @@ class ReflectAccess implements jdk.internal.reflect.LangReflectAccess {
     public <T extends AccessibleObject> T getRoot(T obj) {
         return (T) obj.getRoot();
     }
+
+    public <T> T newInstance(Constructor<T> ctor, Object[] args, Class<?> caller)
+        throws IllegalAccessException, InstantiationException, InvocationTargetException
+    {
+        return ctor.newInstanceWithCaller(args, true, caller);
+    }
 }

src/java.base/share/classes/jdk/internal/reflect/LangReflectAccess.java

@@ -118,4 +118,8 @@ public interface LangReflectAccess {
 
     /** Gets the root of the given AccessibleObject object; null if arg is the root */
     public <T extends AccessibleObject> T getRoot(T obj);
+
+    /** Returns a new instance created by the given constructor with access check */
+    public <T> T newInstance(Constructor<T> ctor, Object[] args, Class<?> caller)
+        throws IllegalAccessException, InstantiationException, InvocationTargetException;
 }

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

@@ -398,6 +398,12 @@ public class ReflectionFactory {
         return langReflectAccess().getExecutableSharedParameterTypes(ex);
     }
 
+    public <T> T newInstance(Constructor<T> ctor, Object[] args, Class<?> caller)
+        throws IllegalAccessException, InstantiationException, InvocationTargetException
+    {
+        return langReflectAccess().newInstance(ctor, args, caller);
+    }
+
     //--------------------------------------------------------------------------
     //
     // Routines used by serialization

test/jdk/java/lang/StackWalker/ReflectionFrames.java

@@ -48,6 +48,16 @@ import static org.testng.Assert.*;
 
 public class ReflectionFrames {
     final static boolean verbose = false;
+    final static Class<?> REFLECT_ACCESS = findClass("java.lang.reflect.ReflectAccess");
+    final static Class<?> REFLECTION_FACTORY = findClass("jdk.internal.reflect.ReflectionFactory");
+
+    private static Class<?> findClass(String cn) {
+        try {
+            return Class.forName(cn);
+        } catch (ClassNotFoundException e) {
+            throw new AssertionError(e);
+        }
+    }
 
     /**
      * This test invokes new StackInspector() directly from
@@ -326,6 +336,8 @@ public class ReflectionFrames {
         assertEquals(obj.collectedFrames,
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
+                             Constructor.class.getName()
+                                 +"::newInstanceWithCaller",
                              Constructor.class.getName()
                                  +"::newInstance",
                              StackInspector.Caller.class.getName()
@@ -354,6 +366,8 @@ public class ReflectionFrames {
         assertEquals(obj.collectedFrames,
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
+                             Constructor.class.getName()
+                                 +"::newInstanceWithCaller",
                              Constructor.class.getName()
                                  +"::newInstance",
                              StackInspector.Caller.class.getName()
@@ -386,6 +400,8 @@ public class ReflectionFrames {
         assertEquals(obj.collectedFrames,
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
+                             Constructor.class.getName()
+                                 +"::newInstanceWithCaller",
                              Constructor.class.getName()
                                  +"::newInstance",
                              StackInspector.Caller.class.getName()
@@ -436,15 +452,19 @@ public class ReflectionFrames {
         assertEquals(obj.collectedFrames,
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
+                             REFLECT_ACCESS.getName()
+                                 +"::newInstance",
+                             REFLECTION_FACTORY.getName()
+                                 +"::newInstance",
                              Class.class.getName()
                                  +"::newInstance",
                              StackInspector.Caller.class.getName()
                                  +"::create",
                              ReflectionFrames.class.getName()
                                  +"::testNewInstance"));
-        // Because Class.newInstance is not filtered, then the
+        // Because implementation frames are not filtered, then the
-        // caller is Class.class
+        // caller is ReflectAccess.class
-        assertEquals(obj.cls, Class.class);
+        assertEquals(obj.cls, REFLECT_ACCESS);
         assertEquals(obj.filtered, 0);
 
         // Calls the StackInspector.reflect method through reflection
@@ -464,6 +484,10 @@ public class ReflectionFrames {
         assertEquals(obj.collectedFrames,
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
+                             REFLECT_ACCESS.getName()
+                                 +"::newInstance",
+                             REFLECTION_FACTORY.getName()
+                                 +"::newInstance",
                              Class.class.getName()
                                  +"::newInstance",
                              StackInspector.Caller.class.getName()
@@ -473,9 +497,9 @@ public class ReflectionFrames {
                              ReflectionFrames.class.getName()
                                  +"::testNewInstance"));
 
-        // Because Class.newInstance is not filtered, then the
+        // Because implementation frames are not filtered, then the
-        // caller is Class.class
+        // caller is ReflectAccess.class
-        assertEquals(obj.cls, Class.class);
+        assertEquals(obj.cls, REFLECT_ACCESS);
         assertEquals(obj.filtered, 0);
 
         // Calls the StackInspector.handle method through reflection
@@ -495,6 +519,10 @@ public class ReflectionFrames {
         assertEquals(obj.collectedFrames,
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
+                             REFLECT_ACCESS.getName()
+                                 +"::newInstance",
+                             REFLECTION_FACTORY.getName()
+                                 +"::newInstance",
                              Class.class.getName()
                                  +"::newInstance",
                              StackInspector.Caller.class.getName()
@@ -504,9 +532,9 @@ public class ReflectionFrames {
                              ReflectionFrames.class.getName()
                                  +"::testNewInstance"));
 
-        // Because Class.newInstance is not filtered, then the
+        // Because implementation frames are not filtered, then the
-        // caller is Class.class
+        // caller is ReflectAccess.class
-        assertEquals(obj.cls, Class.class);
+        assertEquals(obj.cls, REFLECT_ACCESS);
         assertEquals(obj.filtered, 0);
 
         // Sets a non-default walker configured to show
@@ -529,6 +557,8 @@ public class ReflectionFrames {
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
                              Constructor.class.getName()
+                                 +"::newInstanceWithCaller",
+                             REFLECT_ACCESS.getName()
                                  +"::newInstance",
                              Class.class.getName()
                                  +"::newInstance",
@@ -538,9 +568,9 @@ public class ReflectionFrames {
                                  +"::invoke",
                              ReflectionFrames.class.getName()
                                  +"::testNewInstance"));
-        // Because Class.newInstance is not filtered, then the
+        // Because implementation frames are not filtered, then the
-        // caller is Class.class
+        // caller is ReflectAccess.class
-        assertEquals(obj.cls, Class.class);
+        assertEquals(obj.cls, REFLECT_ACCESS);
         assertNotEquals(obj.filtered, 0);
 
         // Calls the StackInspector.reflect method through reflection
@@ -557,10 +587,13 @@ public class ReflectionFrames {
         obj = ((StackInspector)StackInspector.Caller.class
                              .getMethod("reflect", How.class)
                              .invoke(null, How.CLASS));
+        System.out.println(obj.collectedFrames);
         assertEquals(obj.collectedFrames,
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
                              Constructor.class.getName()
+                                 +"::newInstanceWithCaller",
+                             REFLECT_ACCESS.getName()
                                  +"::newInstance",
                              Class.class.getName()
                                  +"::newInstance",
@@ -575,9 +608,9 @@ public class ReflectionFrames {
                              ReflectionFrames.class.getName()
                                  +"::testNewInstance"));
 
-        // Because Class.newInstance is not filtered, then the
+        // Because implementation frames are not filtered, then the
-        // caller is Class.class
+        // caller is ReflectAccess.class
-        assertEquals(obj.cls, Class.class);
+        assertEquals(obj.cls, REFLECT_ACCESS);
         assertNotEquals(obj.filtered, 0);
 
         // Calls the StackInspector.handle method through reflection
@@ -598,6 +631,8 @@ public class ReflectionFrames {
                      List.of(StackInspector.class.getName()
                                  +"::<init>",
                              Constructor.class.getName()
+                                 +"::newInstanceWithCaller",
+                             REFLECT_ACCESS.getName()
                                  +"::newInstance",
                              Class.class.getName()
                                  +"::newInstance",
@@ -611,9 +646,9 @@ public class ReflectionFrames {
                              ReflectionFrames.class.getName()
                                  +"::testNewInstance"));
 
-        // Because Class.newInstance is not filtered, then the
+        // Because implementation frames are not filtered, then the
-        // caller is Class.class
+        // caller is ReflectAccess.class
-        assertEquals(obj.cls, Class.class);
+        assertEquals(obj.cls, REFLECT_ACCESS);
         assertNotEquals(obj.filtered, 0);
     }
 

test/jdk/java/lang/reflect/callerCache/AccessTest.java

@@ -149,4 +149,11 @@ public class AccessTest {
             super("privateStaticFinalField");
         }
     }
+
+    public static class NewInstance implements Callable<Object> {
+        public Object call() throws Exception {
+            return Members.class.newInstance();
+        }
+    }
+
 }

test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java

@@ -77,7 +77,8 @@ public class ReflectionCallerCacheTest {
             { "AccessTest$PublicFinalField"},
             { "AccessTest$PrivateFinalField"},
             { "AccessTest$PublicStaticFinalField"},
-            { "AccessTest$PrivateStaticFinalField"}
+            { "AccessTest$PrivateStaticFinalField"},
+            { "AccessTest$NewInstance"}
         };
     }
 

test/jdk/jdk/modules/open/Basic.java

@@ -98,6 +98,9 @@ public class Basic {
         ctor.setAccessible(true);
         ctor.newInstance();
 
+        // Class::newInstance
+        clazz.newInstance();
+
         // method handles
         findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());
         findNoArgConstructorAndInvoke(clazz, MethodHandles.lookup());
@@ -122,6 +125,12 @@ public class Basic {
         ctor.setAccessible(true);
         ctor.newInstance();
 
+        // Class::newInstance
+        try {
+            clazz.newInstance();
+            assertTrue(false);
+        } catch (IllegalAccessException expected) { }
+
         // method handles
         try {
             findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());
@@ -150,6 +159,9 @@ public class Basic {
         ctor.setAccessible(true);
         ctor.newInstance();
 
+        // Class::newInstance
+        clazz.newInstance();
+
         // method handles
         findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());
         findNoArgConstructorAndInvoke(clazz, MethodHandles.lookup());
@@ -174,6 +186,12 @@ public class Basic {
         ctor.setAccessible(true);
         ctor.newInstance();
 
+        // Class::newInstance
+        try {
+            clazz.newInstance();
+            assertTrue(false);
+        } catch (IllegalAccessException expected) { }
+
         // method handles
         try {
             findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());
@@ -200,6 +218,7 @@ public class Basic {
         // core reflection
         Class<?> clazz = q.PublicType.class;
         clazz.getConstructor().newInstance();
+        clazz.newInstance();
 
         // method handles
         findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());
@@ -226,6 +245,12 @@ public class Basic {
         ctor.setAccessible(true);
         ctor.newInstance();
 
+        // Class::newInstance
+        try {
+            clazz.newInstance();
+            assertTrue(false);
+        } catch (IllegalAccessException expected) { }
+
         // method handles
         try {
             findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());
@@ -256,6 +281,12 @@ public class Basic {
             assertTrue(false);
         } catch (InaccessibleObjectException expected) { }
 
+        // Class::newInstance
+        try {
+            clazz.newInstance();
+            assertTrue(false);
+        } catch (IllegalAccessException expected) { }
+
         // method handles
         try {
             findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());
@@ -288,6 +319,12 @@ public class Basic {
             assertTrue(false);
         } catch (InaccessibleObjectException expected) { }
 
+        // Class::newInstance
+        try {
+            clazz.newInstance();
+            assertTrue(false);
+        } catch (IllegalAccessException expected) { }
+
         // method handles
         try {
             findNoArgConstructorAndInvoke(clazz, MethodHandles.publicLookup());