# HG changeset patch # User Amir Shalem # Date 1524375548 -10800 # Sun Apr 22 08:39:08 2018 +0300 # Node ID 8b4a38092d6e1543181e0c17036344e3d70d7b39 # Parent 485677a0016f3ec8d49faf70f84f953aa4196a33 When using reflection API there are security checks to the callers (unless .setAccessible(true) is called before), those checks are cached in `Executable::securityCheckCache` field, and that field is pointing to the caller class (and its class loader). The problem occurs when the method is first invoked, and at that a JNI accessor is created (`NativeMethodAccessorImpl`). The bug is that instead of passing the root method, the code is passing the child method, which could contain such `securityCheckCache` cache. Before java9 this bug was happening only for protected method and constructors, because there was a quick path to avoid security cache (`Reflection.quickCheckMemberAccess`) which was removed (http://hg.openjdk.java.net/jdk/jdk/rev/9d0388c6b336) Since java9 it is happening for all variations. The following code `Thread.class.getMethod("currentThread").invoke(null)` will cause the caller to leak. For methods and constructors the path to root GC is kept as long as `NativeMethodAccessorImpl` (or `NativeConstructorAccessorImpl`) is kept, which is gone after inflationThreshold (15) calls to the method (or constuctor). Afterwards the accessor is turned into bytecode, and no longer holds reference to Java objects. But for fields the leak will stay forever, and will always leak the first caller, if that caller required security check. The fix is passing the parent to the accessor, since it contains the same information required, but won't cause any leaks for caller classes. diff --git a/src/java.base/share/classes/java/lang/reflect/Constructor.java b/src/java.base/share/classes/java/lang/reflect/Constructor.java --- a/src/java.base/share/classes/java/lang/reflect/Constructor.java +++ b/src/java.base/share/classes/java/lang/reflect/Constructor.java @@ -527,7 +527,8 @@ constructorAccessor = tmp; } else { // Otherwise fabricate one and propagate it up to the root - tmp = reflectionFactory.newConstructorAccessor(this); + tmp = reflectionFactory.newConstructorAccessor( + root != null ? root : this); setConstructorAccessor(tmp); } diff --git a/src/java.base/share/classes/java/lang/reflect/Field.java b/src/java.base/share/classes/java/lang/reflect/Field.java --- a/src/java.base/share/classes/java/lang/reflect/Field.java +++ b/src/java.base/share/classes/java/lang/reflect/Field.java @@ -1102,7 +1102,8 @@ fieldAccessor = tmp; } else { // Otherwise fabricate one and propagate it up to the root - tmp = reflectionFactory.newFieldAccessor(this, overrideFinalCheck); + tmp = reflectionFactory.newFieldAccessor( + root != null ? root : this, overrideFinalCheck); setFieldAccessor(tmp, overrideFinalCheck); } diff --git a/src/java.base/share/classes/java/lang/reflect/Method.java b/src/java.base/share/classes/java/lang/reflect/Method.java --- a/src/java.base/share/classes/java/lang/reflect/Method.java +++ b/src/java.base/share/classes/java/lang/reflect/Method.java @@ -632,7 +632,8 @@ methodAccessor = tmp; } else { // Otherwise fabricate one and propagate it up to the root - tmp = reflectionFactory.newMethodAccessor(this); + tmp = reflectionFactory.newMethodAccessor( + root != null ? root : this); setMethodAccessor(tmp); } diff --git a/test/hotspot/jtreg/compiler/classUnloading/accessorLeak/TestAccessorLeak.java b/test/hotspot/jtreg/compiler/classUnloading/accessorLeak/TestAccessorLeak.java new file mode 100644 --- /dev/null +++ b/test/hotspot/jtreg/compiler/classUnloading/accessorLeak/TestAccessorLeak.java @@ -0,0 +1,96 @@ +/* + * Copyright (c) 2014, 2017, 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 + * @summary "Tests unloading of callers with accessors" + * @library /test/lib / + * @modules java.base/jdk.internal.misc + * @build sun.hotspot.WhiteBox + compiler.classUnloading.accessorLeak.WorkerClass + * @run driver ClassFileInstaller sun.hotspot.WhiteBox + * sun.hotspot.WhiteBox$WhiteBoxPermission + * + * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI + * compiler.classUnloading.accessorLeak.TestAccessorLeak + */ + +package compiler.classUnloading.accessorLeak; + +import sun.hotspot.WhiteBox; + +import java.io.FilterOutputStream; +import java.io.OutputStream; +import java.io.PrintStream; +import java.lang.ref.WeakReference; +import java.lang.reflect.Constructor; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.net.URL; +import java.net.URLClassLoader; + +public class TestAccessorLeak { + private static final WhiteBox WHITE_BOX = WhiteBox.getWhiteBox(); + + private static final String WORKER_CLASS_NAME = "compiler.classUnloading.accessorLeak.WorkerClass"; + + public static void main(String[] args) throws Exception { + verifyNotLeaked(WORKER_CLASS_NAME + "$leakPublicConstructor"); + verifyNotLeaked(WORKER_CLASS_NAME + "$leakPublicMethod"); + verifyNotLeaked(WORKER_CLASS_NAME + "$leakPublicField"); + verifyNotLeaked(WORKER_CLASS_NAME + "$leakProtectedField"); + verifyNotLeaked(WORKER_CLASS_NAME + "$leakProtectedMethod"); + } + + private static void verifyNotLeaked(String classname) throws Exception { + WeakReference weakLoader = loadAndRunClass(classname); + + // Make sure we hold all of parent methods from freeing in GC + Constructor c = Object.class.getConstructor(); + Method m = Thread.class.getDeclaredMethod("currentThread"); + Field f = Thread.class.getDeclaredField("NORM_PRIORITY"); + Method m2 = PrintStream.class.getDeclaredMethod("setError"); + Field f2 = FilterOutputStream.class.getDeclaredField("out"); + + // Force garbage collection to trigger unloading of class loader + WHITE_BOX.fullGC(); + + if (weakLoader.get() != null) { + throw new RuntimeException("Class " + classname + " is leaking!"); + } + } + + private static WeakReference loadAndRunClass(String classname) throws Exception { + URL url = TestAccessorLeak.class.getProtectionDomain().getCodeSource().getLocation(); + URLClassLoader loader = new URLClassLoader(new URL[] {url}, null); + + // Load worker class with custom class loader + Class workerClass = Class.forName(classname, true, loader); + + workerClass.getDeclaredMethod("test").invoke(null); + + loader.close(); + + return new WeakReference<>(loader); + } +} diff --git a/test/hotspot/jtreg/compiler/classUnloading/accessorLeak/WorkerClass.java b/test/hotspot/jtreg/compiler/classUnloading/accessorLeak/WorkerClass.java new file mode 100644 --- /dev/null +++ b/test/hotspot/jtreg/compiler/classUnloading/accessorLeak/WorkerClass.java @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2014, 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. + */ + +/** + * Worker class that is dynamically loaded/unloaded by TestAccessorLeak. + * + * The methods and fields were randomly selected. They just had to have + * protected and public methods and fields, and to exists in parent + * class loader. + */ + +package compiler.classUnloading.accessorLeak; + +import java.io.ByteArrayOutputStream; +import java.io.FilterOutputStream; +import java.io.OutputStream; +import java.io.PrintStream; +import java.lang.ref.WeakReference; +import java.lang.reflect.Constructor; +import java.lang.reflect.Field; +import java.lang.reflect.Method; + +public class WorkerClass { + public static class leakPublicConstructor { + public static void test() throws Exception { + Constructor c = Object.class.getConstructor(); + c.newInstance(); + } + } + + public static class leakPublicMethod { + public static void test() throws Exception { + Method m = Thread.class.getDeclaredMethod("currentThread"); + m.invoke(null); + } + } + + public static class leakPublicField { + public static void test() throws Exception { + Field f = Thread.class.getDeclaredField("NORM_PRIORITY"); + f.get(null); + } + } + + public static class leakProtectedField extends PrintStream { + leakProtectedField(OutputStream out) { + super(out); + } + + public static void test() throws Exception { + PrintStream c = new leakProtectedField(new ByteArrayOutputStream()); + + Field f = FilterOutputStream.class.getDeclaredField("out"); + f.get(c); + } + } + + public static class leakProtectedMethod extends PrintStream { + leakProtectedMethod(OutputStream out) { + super(out); + } + + public static void test() throws Exception { + PrintStream c = new leakProtectedMethod(new ByteArrayOutputStream()); + + Method m = PrintStream.class.getDeclaredMethod("setError"); + m.invoke(c); + } + } +}