8172726: ForkJoin common pool retains a reference to the thread context class loader
authordl
Tue, 07 Feb 2017 00:38:22 -0800
changeset 43545 fdecd1d14d53
parent 43544 457673db6798
child 43546 ac56d0069424
8172726: ForkJoin common pool retains a reference to the thread context class loader Reviewed-by: martin, psandoz, chegar
jdk/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
jdk/src/java.base/share/classes/java/util/concurrent/ForkJoinWorkerThread.java
jdk/test/java/util/concurrent/tck/ForkJoinPool9Test.java
jdk/test/java/util/concurrent/tck/JSR166TestCase.java
jdk/test/java/util/concurrent/tck/tck.policy
--- a/jdk/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java	Mon Feb 06 22:23:46 2017 +0100
+++ b/jdk/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java	Tue Feb 07 00:38:22 2017 -0800
@@ -40,6 +40,7 @@
 import java.lang.invoke.VarHandle;
 import java.security.AccessController;
 import java.security.AccessControlContext;
+import java.security.Permission;
 import java.security.Permissions;
 import java.security.PrivilegedAction;
 import java.security.ProtectionDomain;
@@ -132,24 +133,30 @@
  *  </tr>
  * </table>
  *
- * <p>The common pool is by default constructed with default
- * parameters, but these may be controlled by setting the following
- * {@linkplain System#getProperty system properties}:
+ * <p>The parameters used to construct the common pool may be controlled by
+ * setting the following {@linkplain System#getProperty system properties}:
  * <ul>
  * <li>{@code java.util.concurrent.ForkJoinPool.common.parallelism}
  * - the parallelism level, a non-negative integer
  * <li>{@code java.util.concurrent.ForkJoinPool.common.threadFactory}
- * - the class name of a {@link ForkJoinWorkerThreadFactory}
+ * - the class name of a {@link ForkJoinWorkerThreadFactory}.
+ * The {@linkplain ClassLoader#getSystemClassLoader() system class loader}
+ * is used to load this class.
  * <li>{@code java.util.concurrent.ForkJoinPool.common.exceptionHandler}
- * - the class name of a {@link UncaughtExceptionHandler}
+ * - the class name of a {@link UncaughtExceptionHandler}.
+ * The {@linkplain ClassLoader#getSystemClassLoader() system class loader}
+ * is used to load this class.
  * <li>{@code java.util.concurrent.ForkJoinPool.common.maximumSpares}
  * - the maximum number of allowed extra threads to maintain target
  * parallelism (default 256).
  * </ul>
- * If a {@link SecurityManager} is present and no factory is
- * specified, then the default pool uses a factory supplying
- * threads that have no {@link Permissions} enabled.
- * The system class loader is used to load these classes.
+ * If no thread factory is supplied via a system property, then the
+ * common pool uses a factory that uses the system class loader as the
+ * {@linkplain Thread#getContextClassLoader() thread context class loader}.
+ * In addition, if a {@link SecurityManager} is present, then
+ * the common pool uses a factory supplying threads that have no
+ * {@link Permissions} enabled.
+ *
  * Upon any error in establishing these settings, default parameters
  * are used. It is possible to disable or limit the use of threads in
  * the common pool by setting the parallelism property to zero, and/or
@@ -638,20 +645,38 @@
          *
          * @param pool the pool this thread works in
          * @return the new worker thread, or {@code null} if the request
-         *         to create a thread is rejected.
+         *         to create a thread is rejected
          * @throws NullPointerException if the pool is null
          */
         public ForkJoinWorkerThread newThread(ForkJoinPool pool);
     }
 
+    static AccessControlContext contextWithPermissions(Permission ... perms) {
+        Permissions permissions = new Permissions();
+        for (Permission perm : perms)
+            permissions.add(perm);
+        return new AccessControlContext(
+            new ProtectionDomain[] { new ProtectionDomain(null, permissions) });
+    }
+
     /**
      * Default ForkJoinWorkerThreadFactory implementation; creates a
-     * new ForkJoinWorkerThread.
+     * new ForkJoinWorkerThread using the system class loader as the
+     * thread context class loader.
      */
     private static final class DefaultForkJoinWorkerThreadFactory
         implements ForkJoinWorkerThreadFactory {
+        private static final AccessControlContext ACC = contextWithPermissions(
+            new RuntimePermission("getClassLoader"),
+            new RuntimePermission("setContextClassLoader"));
+
         public final ForkJoinWorkerThread newThread(ForkJoinPool pool) {
-            return new ForkJoinWorkerThread(pool);
+            return AccessController.doPrivileged(
+                new PrivilegedAction<>() {
+                    public ForkJoinWorkerThread run() {
+                        return new ForkJoinWorkerThread(
+                            pool, ClassLoader.getSystemClassLoader()); }},
+                ACC);
         }
     }
 
@@ -3244,26 +3269,20 @@
          * An ACC to restrict permissions for the factory itself.
          * The constructed workers have no permissions set.
          */
-        private static final AccessControlContext INNOCUOUS_ACC;
-        static {
-            Permissions innocuousPerms = new Permissions();
-            innocuousPerms.add(modifyThreadPermission);
-            innocuousPerms.add(new RuntimePermission(
-                                   "enableContextClassLoaderOverride"));
-            innocuousPerms.add(new RuntimePermission(
-                                   "modifyThreadGroup"));
-            INNOCUOUS_ACC = new AccessControlContext(new ProtectionDomain[] {
-                    new ProtectionDomain(null, innocuousPerms)
-                });
-        }
+        private static final AccessControlContext ACC = contextWithPermissions(
+            modifyThreadPermission,
+            new RuntimePermission("enableContextClassLoaderOverride"),
+            new RuntimePermission("modifyThreadGroup"),
+            new RuntimePermission("getClassLoader"),
+            new RuntimePermission("setContextClassLoader"));
 
         public final ForkJoinWorkerThread newThread(ForkJoinPool pool) {
-            return AccessController.doPrivileged(new PrivilegedAction<>() {
-                public ForkJoinWorkerThread run() {
-                    return new ForkJoinWorkerThread.
-                        InnocuousForkJoinWorkerThread(pool);
-                }}, INNOCUOUS_ACC);
+            return AccessController.doPrivileged(
+                new PrivilegedAction<>() {
+                    public ForkJoinWorkerThread run() {
+                        return new ForkJoinWorkerThread.
+                            InnocuousForkJoinWorkerThread(pool); }},
+                ACC);
         }
     }
-
 }
--- a/jdk/src/java.base/share/classes/java/util/concurrent/ForkJoinWorkerThread.java	Mon Feb 06 22:23:46 2017 +0100
+++ b/jdk/src/java.base/share/classes/java/util/concurrent/ForkJoinWorkerThread.java	Tue Feb 07 00:38:22 2017 -0800
@@ -36,6 +36,8 @@
 package java.util.concurrent;
 
 import java.security.AccessControlContext;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 import java.security.ProtectionDomain;
 
 /**
@@ -88,11 +90,26 @@
     }
 
     /**
+     * Version for use by the default pool.  Supports setting the
+     * context class loader.  This is a separate constructor to avoid
+     * affecting the protected constructor.
+     */
+    ForkJoinWorkerThread(ForkJoinPool pool, ClassLoader ccl) {
+        super("aForkJoinWorkerThread");
+        super.setContextClassLoader(ccl);
+        this.pool = pool;
+        this.workQueue = pool.registerWorker(this);
+    }
+
+    /**
      * Version for InnocuousForkJoinWorkerThread.
      */
-    ForkJoinWorkerThread(ForkJoinPool pool, ThreadGroup threadGroup,
+    ForkJoinWorkerThread(ForkJoinPool pool,
+                         ClassLoader ccl,
+                         ThreadGroup threadGroup,
                          AccessControlContext acc) {
         super(threadGroup, null, "aForkJoinWorkerThread");
+        super.setContextClassLoader(ccl);
         ThreadLocalRandom.setInheritedAccessControlContext(this, acc);
         ThreadLocalRandom.eraseThreadLocals(this); // clear before registering
         this.pool = pool;
@@ -179,20 +196,21 @@
 
     /**
      * A worker thread that has no permissions, is not a member of any
-     * user-defined ThreadGroup, and erases all ThreadLocals after
+     * user-defined ThreadGroup, uses the system class loader as
+     * thread context class loader, and erases all ThreadLocals after
      * running each top-level task.
      */
     static final class InnocuousForkJoinWorkerThread extends ForkJoinWorkerThread {
         /** The ThreadGroup for all InnocuousForkJoinWorkerThreads */
         private static final ThreadGroup innocuousThreadGroup =
-                java.security.AccessController.doPrivileged(
-                    new java.security.PrivilegedAction<>() {
-                        public ThreadGroup run() {
-                            ThreadGroup group = Thread.currentThread().getThreadGroup();
-                            for (ThreadGroup p; (p = group.getParent()) != null; )
-                                group = p;
-                            return new ThreadGroup(group, "InnocuousForkJoinWorkerThreadGroup");
-                        }});
+            java.security.AccessController.doPrivileged(
+                new java.security.PrivilegedAction<>() {
+                    public ThreadGroup run() {
+                        ThreadGroup group = Thread.currentThread().getThreadGroup();
+                        for (ThreadGroup p; (p = group.getParent()) != null; )
+                            group = p;
+                        return new ThreadGroup(group, "InnocuousForkJoinWorkerThreadGroup");
+                    }});
 
         /** An AccessControlContext supporting no privileges */
         private static final AccessControlContext INNOCUOUS_ACC =
@@ -202,7 +220,10 @@
                 });
 
         InnocuousForkJoinWorkerThread(ForkJoinPool pool) {
-            super(pool, innocuousThreadGroup, INNOCUOUS_ACC);
+            super(pool,
+                  ClassLoader.getSystemClassLoader(),
+                  innocuousThreadGroup,
+                  INNOCUOUS_ACC);
         }
 
         @Override // to erase ThreadLocals
@@ -210,11 +231,6 @@
             ThreadLocalRandom.eraseThreadLocals(this);
         }
 
-        @Override // to always report system loader
-        public ClassLoader getContextClassLoader() {
-            return ClassLoader.getSystemClassLoader();
-        }
-
         @Override // to silently fail
         public void setUncaughtExceptionHandler(UncaughtExceptionHandler x) { }
 
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/util/concurrent/tck/ForkJoinPool9Test.java	Tue Feb 07 00:38:22 2017 -0800
@@ -0,0 +1,81 @@
+/*
+ * 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.
+ */
+
+/*
+ * This file is available under and governed by the GNU General Public
+ * License version 2 only, as published by the Free Software Foundation.
+ * However, the following notice accompanied the original version of this
+ * file:
+ *
+ * Written by Doug Lea and Martin Buchholz with assistance from
+ * members of JCP JSR-166 Expert Group and released to the public
+ * domain, as explained at
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.VarHandle;
+import java.util.concurrent.CompletableFuture;
+
+import junit.framework.Test;
+import junit.framework.TestSuite;
+
+public class ForkJoinPool9Test extends JSR166TestCase {
+    public static void main(String[] args) {
+        main(suite(), args);
+    }
+
+    public static Test suite() {
+        return new TestSuite(ForkJoinPool9Test.class);
+    }
+
+    /**
+     * Check handling of common pool thread context class loader
+     */
+    public void testCommonPoolThreadContextClassLoader() throws Throwable {
+        if (!testImplementationDetails) return;
+        VarHandle CCL =
+            MethodHandles.privateLookupIn(Thread.class, MethodHandles.lookup())
+            .findVarHandle(Thread.class, "contextClassLoader", ClassLoader.class);
+        ClassLoader systemClassLoader = ClassLoader.getSystemClassLoader();
+        boolean haveSecurityManager = (System.getSecurityManager() != null);
+        CompletableFuture.runAsync(
+            () -> {
+                assertSame(systemClassLoader,
+                           Thread.currentThread().getContextClassLoader());
+                assertSame(systemClassLoader,
+                           CCL.get(Thread.currentThread()));
+                if (haveSecurityManager)
+                    assertThrows(
+                        SecurityException.class,
+                        () -> System.getProperty("foo"),
+                        () -> Thread.currentThread().setContextClassLoader(null));
+
+                // TODO ?
+//                 if (haveSecurityManager
+//                     && Thread.currentThread().getClass().getSimpleName()
+//                     .equals("InnocuousForkJoinWorkerThread"))
+//                     assertThrows(SecurityException.class, /* ?? */);
+            }).join();
+    }
+
+}
--- a/jdk/test/java/util/concurrent/tck/JSR166TestCase.java	Mon Feb 06 22:23:46 2017 +0100
+++ b/jdk/test/java/util/concurrent/tck/JSR166TestCase.java	Tue Feb 07 00:38:22 2017 -0800
@@ -46,6 +46,7 @@
  * @summary JSR-166 tck tests (whitebox tests allowed)
  * @build *
  * @modules java.base/java.util.concurrent:open
+ *          java.base/java.lang:open
  *          java.management
  * @run junit/othervm/timeout=1000
  *      -Djsr166.testImplementationDetails=true
@@ -59,6 +60,9 @@
  *      -Djava.util.concurrent.ForkJoinPool.common.parallelism=1
  *      -Djava.util.secureRandomSeed=true
  *      JSR166TestCase
+ * @run junit/othervm/timeout=1000/policy=tck.policy
+ *      -Djsr166.testImplementationDetails=true
+ *      JSR166TestCase
  */
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
@@ -584,6 +588,7 @@
                 "AtomicReference9Test",
                 "AtomicReferenceArray9Test",
                 "ExecutorCompletionService9Test",
+                "ForkJoinPool9Test",
             };
             addNamedTestClasses(suite, java9TestClassNames);
         }
@@ -594,7 +599,7 @@
     /** Returns list of junit-style test method names in given class. */
     public static ArrayList<String> testMethodNames(Class<?> testClass) {
         Method[] methods = testClass.getDeclaredMethods();
-        ArrayList<String> names = new ArrayList<String>(methods.length);
+        ArrayList<String> names = new ArrayList<>(methods.length);
         for (Method method : methods) {
             if (method.getName().startsWith("test")
                 && Modifier.isPublic(method.getModifiers())
@@ -700,7 +705,7 @@
      * The first exception encountered if any threadAssertXXX method fails.
      */
     private final AtomicReference<Throwable> threadFailure
-        = new AtomicReference<Throwable>(null);
+        = new AtomicReference<>(null);
 
     /**
      * Records an exception so that it can be rethrown later in the test
@@ -1262,7 +1267,7 @@
         }
         public void refresh() {}
         public String toString() {
-            List<Permission> ps = new ArrayList<Permission>();
+            List<Permission> ps = new ArrayList<>();
             for (Enumeration<Permission> e = perms.elements(); e.hasMoreElements();)
                 ps.add(e.nextElement());
             return "AdjustablePolicy with permissions " + ps;
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/util/concurrent/tck/tck.policy	Tue Feb 07 00:38:22 2017 -0800
@@ -0,0 +1,15 @@
+grant {
+    // Permissions j.u.c. needs directly
+    permission java.lang.RuntimePermission "modifyThread";
+    permission java.lang.RuntimePermission "getClassLoader";
+    permission java.lang.RuntimePermission "setContextClassLoader";
+    permission java.util.PropertyPermission "*", "read";
+    // Permissions needed to change permissions!
+    permission java.security.SecurityPermission "getPolicy";
+    permission java.security.SecurityPermission "setPolicy";
+    permission java.security.SecurityPermission "setSecurityManager";
+    // Permissions needed by the junit test harness
+    permission java.lang.RuntimePermission "accessDeclaredMembers";
+    permission java.io.FilePermission "<<ALL FILES>>", "read";
+    permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
+};