Using Thread.stop() on carefully locked down, but untrusted, code

Question

I'm aware that Thread.stop() is deprecated, and for good reason: it is not, in general, safe. But that doesn't mean that it is never safe... as far as I can see, it is safe in the context in which I want to use it; and, as far as I can see, I have no other option.

The context is a third-party plug-in for a two-player strategy game: chess will do as the working example. The third-party code needs to be given a current board state, and (say) 10 seconds to decide on its move. It can return its move and terminate within the allowable time, or it can, whenever it wants to, signal its current preferred move; if the time limit expires, it should be stopped in its tracks, and its most recent preferred move should be played.

Writing the plug-in to stop gracefully on request is not an option: I need to be able to take arbitrary untrusted third-party plug-ins. So I have to have some way of forcibly terminating it.

Here's what I'm doing to lock it down:

1. The classes for the plug-in get put into their own thread, into their own thread group.
2. They are loaded with a class loader that has a highly restrictive SecurityManager in place: all the classes can do is number crunching.
3. The classes don't get a reference to any other threads, so they can't use Thread.join() on anything they haven't created.
4. The plug-in gets only two references to objects from the host system. One is the state of the chess board; this is a deep copy, and is thrown away afterwards, so it doesn't matter if it gets into an inconsistent state. The other is a simple object that allows the plug-in to set its current preferred move.
5. I'm checking when CPU time has exceeded the limit, and I'm periodically checking that at least one thread of the plug-in is doing something (so that it can't sleep indefinitely and avoid CPU time ever hitting the limit).
6. The preferred move doesn't have enough state to be inconsistent, but in any case it is carefully and defensively cloned after it is returned, and the one that was returned is discarded. By this point, there is nothing left in the host system to which the plug-in had a reference: no threads, no object instances.

In consequence, it seems, the plug-in can't leave anything in an inconsistent state (except for any objects it might create, which will then be discarded); and it can't affect any other threads (except for any threads it spawns, which will be in the same ThreadGroup, and so will also be killed off).

It looks to me as though the reasons that Thread.stop() is deprecated don't apply here (by design). Have I missed anything? Is there still danger here? Or have I isolated things carefully enough that there can't be a problem?

And is there a better way? The only alternative, I think, is to start up a whole new JVM to run the untrusted code, and forcibly kill the process when it's no longer wanted, but that has a thousand other problems (expensive, fragile, OS-dependent).

Please note: I'm not interested in answers along the lines of "Ooh, it's deprecated for a reason, you want to watch it, mate." I know it's deprecated for a reason, and I completely understand why it is not safe to be let out of the cage in general. What I am asking is whether there is a specific reason for thinking that it is unsafe in this context.

For what it's worth, here is the (abridged) relevant bit of the code:

public void playMoveInternal(GameState game) throws IllegalMoveException,
        InstantiationException, IllegalAccessException,
        IllegalMoveSpecificationException {
    ThreadGroup group = new ThreadGroup("playthread group");
    Thread playthread = null;
    group.setMaxPriority(Thread.MIN_PRIORITY);
    GameMetaData meta = null;
    StrategyGamePlayer player = null;
    try {
        GameState newgame = (GameState) game.clone();
        SandboxedURLClassLoader loader = new SandboxedURLClassLoader(
          // recreating this each time means static fields don't persist
                urls[newgame.getCurPlayer() - 1], playerInterface);
        Class<?> playerClass = loader.findPlayerClass();
        GameTimer timer = new GameTimer(
                newgame.getCurPlayer() == 1 ? timelimit : timelimit2);
          // time starts ticking here!
        meta = new GameMetaData((GameTimer) timer.clone());
        try {
            player = (StrategyGamePlayer) playerClass.newInstance();
        } catch (Exception e) {
            System.err.println("Couldn't create player module instance!");
            e.printStackTrace();
            game.resign(GameMoveType.MOVE_ILLEGAL);
            return;
        }
        boolean checkSleepy = true;
        playthread = new Thread(group, new MoveMakerThread(player, meta,
                newgame), "MoveMaker thread");
        int badCount = 0;
        playthread.start();
        try {
            while ((timer.getTimeRemaining() > 0) && (playthread.isAlive())
                    && (!stopping) && (!forceMove)) {
                playthread.join(50);
                if (checkSleepy) {
                    Thread.State thdState = playthread.getState();
                    if ((thdState == Thread.State.TIMED_WAITING)
                            || (thdState == Thread.State.WAITING)) {
                        // normally, main thread will be busy
                        Thread[] allThreads = new Thread[group
                                .activeCount() * 2];
                        int numThreads = group.enumerate(allThreads);
                        boolean bad = true;
                        for (int i = 0; i < numThreads; i++) {
                            // check some player thread somewhere is doing something
                            thdState = allThreads[i].getState();
                            if ((thdState != Thread.State.TIMED_WAITING)
                                    && (thdState != Thread.State.WAITING)) {
                                bad = false;
                                break; // found a good thread, so carry on
                            }
                        }
                        if ((bad) && (badCount++ > 100))
                            // means player has been sleeping for an expected 5
                            // sec, which is naughty
                            break;
                    }
                }
            }
        } catch (InterruptedException e) {
            System.err.println("Interrupted: " + e);
        }
    } catch (Exception e) {
        System.err.println("Couldn't play the game: " + e);
        e.printStackTrace();
    }
    playthread.destroy();
    try {
        Thread.sleep(1000);
    } catch (Exception e) {
    }
    group.stop();
    forceMove = false;
    try {
        if (!stopping)
            try {
                if (!game.isLegalMove(meta.getBestMove())) {
                    game.resign(GameMoveType.MOVE_ILLEGAL);
                }
                else
                    game.makeMove((GameMove) (meta.getBestMove().clone()));
                // We rely here on the isLegalMove call to make sure that
                // the return type is the right (final) class so that the clone()
                // call can't execute dodgy code
            } catch (IllegalMoveException e) {
                game.resign(GameMoveType.MOVE_ILLEGAL);
            } catch (NullPointerException e) {
                // didn't ever choose a move to make
                game.resign(GameMoveType.MOVE_OUT_OF_TIME);
            }
    } catch (Exception e) {
        e.printStackTrace();
        game.resign(GameMoveType.MOVE_OUT_OF_TIME);
    }
}

Answer 1

While I can imagine scenarios where code that you have carefully reviewed may be safely stopped with Thread.stop(), you are talking about the opposite, code that you distrust so much that you are attempting to restrict its actions by a SecurityManager.

So what can go wrong? First of all, stopping does not work more reliable than interruption. While it is easier to ignore an interruption, a simple catch(Throwable t){} is enough to make Thread.stop() not work anymore and such a catch may appear in code even without the intention of preventing Thread.stop() from working, though it is bad coding style. But coders wanting to ignore Thread.stop() might even add a catch(Throwable t){} and/or catch(ThreadDeath t){} intentionally.

Speaking of bad coding style, locking should always be implemented using try … finally … to ensure that the lock is guaranteed to be freed even in the exceptional case. If the stopped code failed to do this right, the lock may remain locked when the thread will be stopped.

The bottom line is that within the same JVM you can’t protect yourself against untrusted code. The JVM is the sandbox which protects the outside. Just one simple example: a malicious code could start eating up memory by performing lots of small memory allocations. There is nothing you can do within the JVM to protect some code running in the same JVM against it. You can only enforce limits for the entire JVM.

So the only solution is to run the plugins in a different JVM.