Why can't I shutdown my own ExecutorService under a SecurityManager?

Question

Under the default security manager, if I create an ExecutorService (ThreadPoolExecutor in this case), I cannot shut it down, shutdown() just calls checkPermission("modifyThread") and thus immediately dies:

import java.util.concurrent.*;

class A {
    public static void main( String[] args) {
        Thread ct = Thread.currentThread();
        System.out.println("current thread: " + ct);
        ct.checkAccess(); // we have access to our own thread...
        ThreadPoolExecutor tpe = new ThreadPoolExecutor(
            1, // one core thread
            1, // doesn't matter because queue is unbounded
            0, TimeUnit.SECONDS, // doesn't matter in this case
            new LinkedBlockingQueue<Runnable>(), /* unbound queue for
                                                  * our single thread */
            new ThreadFactory() {
                public Thread newThread(Runnable r) {
                    // obviously never gets called as we don't add any work
                    System.out.println("making thread");
                    return new Thread(r);
                }
            }
        );
        tpe.shutdown(); // raises security exception
    }
}

Sun JDK:

$ java -Djava.security.manager A current thread: Thread[main,5,main] Exception in thread "main" java.security.AccessControlException: access denied (java.lang.RuntimePermission modifyThread) at java.security.AccessControlContext.checkPermission(AccessControlContext.java:323) at java.security.AccessController.checkPermission(AccessController.java:546) at java.lang.SecurityManager.checkPermission(SecurityManager.java:532) at java.util.concurrent.ThreadPoolExecutor.shutdown(ThreadPoolExecutor.java:1094) at A.main(A.java:22)

OpenJDK:

$ java -Djava.security.manager A current thread: Thread[main,5,main] Exception in thread "main" java.security.AccessControlException: access denied (java.lang.RuntimePermission modifyThread) at java.security.AccessControlContext.checkPermission(AccessControlContext.java:342) at java.security.AccessController.checkPermission(AccessController.java:553) at java.lang.SecurityManager.checkPermission(SecurityManager.java:549) at java.util.concurrent.ThreadPoolExecutor.checkShutdownAccess(ThreadPoolExecutor.java:711) at java.util.concurrent.ThreadPoolExecutor.shutdown(ThreadPoolExecutor.java:1351) at A.main(A.java:22)

Why??????? What are the security implications of creating a thread pool that only you control, and shutting it down? Is this a bug in the implementations, or am I missing something?

Let's see what the spec for ExecutorService.shutdown says...

Initiates an orderly shutdown in which previously submitted tasks are executed, but no new tasks will be accepted. Invocation has no additional effect if already shut down.

Throws: SecurityException - if a security manager exists and shutting down this ExecutorService may manipulate threads that the caller is not permitted to modify because it does not hold RuntimePermission("modifyThread"), or the security manager's checkAccess method denies access.

This... is about as vague as it gets. The spec says nothing about any "system threads" being made during the life-cycle of an ExecutorService and furthermore, it lets you supply your own threads which is proof that there should be no "system threads" involved when you do that. (As done above in my sample source)

It feels like the Java SE implementors saw that it's possible for shutdown to raise SecurityException, so they were just like, "oh okay I'll just add a random security check here for compliance"...

The thing is, reading over OpenJDK source (openjdk-6-src-b20-21_jun_2010), it turns out that the only way any thread is ever created, is by calling your supplied ThreadFactory (which is never called in my testcase since I don't create any work, and I don't call prestartCoreThread or preStartAllCoreThreads). The security check is thus done for no apparent reason in OpenJDK's ThreadPoolExecutor (as is done in sun-jdk-1.6 but I don't have the source):

/**
 * Initiates an orderly shutdown in which previously submitted
 * tasks are executed, but no new tasks will be accepted.
 * Invocation has no additional effect if already shut down.
 *
 * @throws SecurityException {@inheritDoc}
 */
public void shutdown() {
    final ReentrantLock mainLock = this.mainLock;
    mainLock.lock();
    try {
        checkShutdownAccess();
        advanceRunState(SHUTDOWN);
        interruptIdleWorkers();
        onShutdown(); // hook for ScheduledThreadPoolExecutor
    } finally {
        mainLock.unlock();
    }
    tryTerminate();
}

checkShutdownAccess is called before doing anything...

/**
 * If there is a security manager, makes sure caller has
 * permission to shut down threads in general (see shutdownPerm).
 * If this passes, additionally makes sure the caller is allowed
 * to interrupt each worker thread. This might not be true even if
 * first check passed, if the SecurityManager treats some threads
 * specially.
 */
private void checkShutdownAccess() {
    SecurityManager security = System.getSecurityManager();
    if (security != null) {
        security.checkPermission(shutdownPerm);
        final ReentrantLock mainLock = this.mainLock;
        mainLock.lock();
        try {
            for (Worker w : workers)
                security.checkAccess(w.thread);
        } finally {
            mainLock.unlock();
        }
    }
}

As you can see, it unconditionally invokes checkPermission(shutdownPerm) on the security manager.... shutdownPerm is defined as... private static final RuntimePermission shutdownPerm = new RuntimePermission("modifyThread");

...which makes absolutely no sense as far as I can tell because modifyThread implies access to system threads, and there are no system threads in play here, in fact, there are no threads at all because I didn't submit any work or prestart, and even if there were threads, they'd be my threads because I passed in a ThreadFactory. The spec doesn't say anything about magically dying, other than that if system threads are involved (they aren't), there could be a SecurityException.

Basically, why can't I just remove the line that checks access to system threads? I see no security implication calling for it. And how has nobody else come across this issue??? I've seen a post on an issue tracker where they "resolved" this issue by changing a call to shutdownNow to shutdown, obviously, that didn't fix it for them.

Glacier

'[...] it lets you supply your own threads which is proof that there should be no "system threads" involved [...]' Clearly there is a flaw in your reasoning there. But yes it could be done differently. This is how JSR166 defines the API. – Topmast

Hmm... sounds like something that might have to go to bugs.sun.com – Stylish

@Tom Hawtin: Ahh, I didn't know JSRs defined this part of SE, thanks for mentioning it. However isn't the API only specified by the javadoc in that JSR? In that case it still only says: "SecurityException - if a security manager exists and shutting down this ExecutorService may manipulate threads that the caller is not permitted to modify because it does not hold RuntimePermission("modifyThread"), or the security manager's checkAccess method denies access." I see no reason for it to manipulate system threads in my case, and checkAccess(Thread) by default only guards system threads. – Miramontes

Also the TCK doesn't explicitly check if SecurityException is raised anywhere. Why do you think there is a flaw in my reasoning? I see no reason for privileged code to run where it just needs to make some threads and "kill them" later. This is just forcing me to violate the principle of least privilege by granting modifyThread permission to untrusted code. Or am I still missing something? – Miramontes

Found more info here: cs.oswego.edu/pipermail/concurrency-interest/2009-August/… Sounds fishy... – Miramontes

Answer 1

It's quite simple: you can't do it in the main thread group. It's partly designed for applets. Copy from shutdown method idea why? You can freely use PrivilegedAction to call shutdown if that's an issue. Keep in mind that Thread.interrupt() as innocent it might look also throws SecurityException.

To answer the question: just make sure you grant your own code the permissions and you're happy. Alternatively "modifyThread" can be granted freely as well, it's used mostly by applets.

As for untrusted code: well, the untrusted code is not even supposed to deal w/ threads outside its ThreadGroup, so provide the API to create the ThreadPool, and allow shutdown for such created by the caller. You can GRANT the permission based on the caller.

Hope this helped a bit (the amount of question marks clearly shows desperation and utmost annoyance, though)

    /*
     * Conceptually, shutdown is just a matter of changing the
     * runState to SHUTDOWN, and then interrupting any worker
     * threads that might be blocked in getTask() to wake them up
     * so they can exit. Then, if there happen not to be any
     * threads or tasks, we can directly terminate pool via
     * tryTerminate.  Else, the last worker to leave the building
     * turns off the lights (in workerDone).
     *
     * But this is made more delicate because we must cooperate
     * with the security manager (if present), which may implement
     * policies that make more sense for operations on Threads
     * than they do for ThreadPools. This requires 3 steps:
     *
     * 1. Making sure caller has permission to shut down threads
     * in general (see shutdownPerm).
     *
     * 2. If (1) passes, making sure the caller is allowed to
     * modify each of our threads. This might not be true even if
     * first check passed, if the SecurityManager treats some
     * threads specially. If this check passes, then we can try
     * to set runState.
     *
     * 3. If both (1) and (2) pass, dealing with inconsistent
     * security managers that allow checkAccess but then throw a
     * SecurityException when interrupt() is invoked.  In this
     * third case, because we have already set runState, we can
     * only try to back out from the shutdown as cleanly as
     * possible. Some workers may have been killed but we remain
     * in non-shutdown state (which may entail tryTerminate from
     * workerDone starting a new worker to maintain liveness.)
     */

Mcelrath

Answer 2

Sounds like a lazy and/or safe implementation. Instead of checking if other threads are involved, it just assumes some are. Better to throw a security exception rather than leave a potential security hole.

Lohengrin