asvd / jailed

execute untrusted code with custom permissions
MIT License
1k stars 69 forks source link

Timeout feature? #2

Closed gilbert closed 9 years ago

gilbert commented 9 years ago

First, thanks for making this, it's great.

What do you think about adding a timeout feature when making remote calls? For example:

plugin.remote.square(2, reportResult, {
  timeout: 2000,
  onTimeout: myTimeoutHandler
});

This would make guarding against untrusted code very convenient. Thoughts?

gilbert commented 9 years ago

The example I wrote wouldn't work out, but it should give you the right idea.

asvd commented 9 years ago

you mean something like the console demo works?

http://asvd.github.io/jailed/demos/web/console/

(meaning shutting down the plugin if the code works too long)

gilbert commented 9 years ago

Yup, exactly. I ended up having to do something like your example code.

Upon further thought, this API might be plausible (not to mention convenient):

plugin.remote.square(2, reportResult).timeout(2000, myTimeoutHandler)
asvd commented 9 years ago

This is really a topic for a discussion.

Currently I have the following vision. The timeout feature which is present in similar solutions (and therefore feels reasonable) works good for a bit different use-case, namely when one needs to execute / evalutate a piece of code safely somehow and get the result back. If the result is not delivered within the timeout, the code is considered to be void, and the calculation is interrupted.

In case of Jailed, you have an independent JS session running on its own (in a plugin). The session is fully controlled by the application in sence that the application may grant permissions or shut it down anytime. Timeout is an instance which is likely related to a single calculation act, not to the whole session. The application creates and destroys the session whenever it is needed for something, i.e. to perform a set of calculations.

Therefore I would prefer to avoid having this feature in the library and let the application developer manually describe the rules of when to create and destroy the plugin.

The console demo is exactly an example of how such protection can be implemented. Otherwise if adding this feature into the library, a lot of things become unlogical on my opinion.

Particularly in your example it is not very clear, what exactly should happen if the execution of the square() function has timed out. Should the whole plugin be destroyed? Why? Should it probably better be decided by a developer of a particular application? (on my opinion yes)

I would appreciate your thoughts, especially if you have a different opinion.

asvd commented 9 years ago

Some further thoughts:

The discussed use-case (evaluating a line of code with a timeout) can be implemented on top of Jailed. I have created some dirty sketch of a helper function for this (did not test the code, but it should make sence):

function evaluate(code, sCb, fCb, timeout) {
    var plugin = new jailed.DynamicPlugin(
        'application.setInterface({ evaluate: function(code, cb){cb(eval(code));} });'
    );

    plugin.whenConnected(function(){
        var process = function(result) {
            clearTimeout(failTimeout);
            sCb(result);
        }

        plugin.remote.evaluate(code, process);

        var failTimeout = setTimeout(function(){
            plugin.disconnect();
            fCb();
        }, timeout);
    });
}

// usage, giving that we have sCb() and fCb() defined
evaluate('2+2', sCb, fCb, 1000);

I think I should probably create a demo for the case.

gilbert commented 9 years ago

I agree with letting the application developer deciding when to create and destroy the session. I was actually thinking that .timeout() would not auto-destroy the session, but instead be more like registering an event listener.

Here's my current use case. I'm creating a bot game server that runs untrusted code written by two different people. During each turn I need to run a bot's play() method, but only give them 500ms to make their move. If a bot takes too long, its move gets defaulted, but it is still in the game.

As you and I agree, it's not a good idea to auto-destroy the session on timeout. However, it would be nice to know when a timeout happens, so the application can respond accordingly. In my latter example, myTimeoutHandler would be the code that receives and responds to the timeout event.

asvd commented 9 years ago

But what happens if a user has submitted an infinite loop? On my opinion that would hang up the plugin, and it should be restarted in any case. I think you should create a plugin for each move, transfer all needed data into the plugin, and then shut it down (either upon the move complete, or upon a timeout). So the solution for you is similar to the evaluate() function I just mentiond, but instead of evaluating any line of code it should calculate a move using the code of a user. Would that work?

gilbert commented 9 years ago

That's a good thought, but the bots need to stay alive so that they can retain any data structures / calculations based on previous turns. In the case of an infinite loop, I suppose the bot would continue to default until it loses the game.

asvd commented 9 years ago

I think the main game loop should run outside of the plugin (in the main application). Just transfer all the needed data into each plugin for each move.

asvd commented 9 years ago

Ah, I guess I misunderstood, sorry. The user provides not only the move function, right?

gilbert commented 9 years ago

Right, they provide a function that assumes it has its own closure for data persistence across turns. I guess it's possible to provide a data store API to the bot, but that might defeat the purpose of the sandbox (or be too much trouble).

asvd commented 9 years ago

You can also provide the game data object as an argument for the move function. To be honest, I do not see why it could be dangerous, maybe only because of the rules of the game. At least one can access the closured variables in the same way as the arguments.

Another option would be to create subworker / subprocess from inside the plugin. There is already a sandbox, you only need a timeout for the move() function call without the need to protect it once again.

gilbert commented 9 years ago

Yes, they are already receiving game data, I'm talking about extra data that the bots generate themselves. Regardless, it could be that my use case isn't common enough to warrant this feature :) Can you tell me more about your subworker suggestion?

asvd commented 9 years ago

Your case is some real application of Jailed, so my point here is to understand the case in order to probably make Jailed a bit more useful or general.

I meant that you can for instance create a separate plugin for each move of a user, and transfer all the data needed for calculating the next move into that plugin, and then launch the move() function there. This way you will be able to kill that plugin upon a timeout, in case it has not responded.

But the question is what else (in addition to calculating the next step using the move() function) is written by the users, and should therefore be launched somewhere outside of a temporary plugin created for the purpose of a single move.