Promises allow vm.runInContext timeout to be escaped

[Macil]

The timeout property on many of the vm module's functions isn't completely foolproof. In the following example, some sandboxed code executed by vm.runInNewContext with a timeout of 5ms schedules an infinite loop to run after a promise resolves, and then synchronously executes an infinite loop. The synchronous infinite loop is killed by the timeout, but then the scheduled loop fires and never ends.

vm.runInNewContext(
  'Promise.resolve().then(()=>{while(1)console.log("foo", Date.now());}); while(1)console.log(Date.now())',
  {console:{log(){console.log.apply(console,arguments);}}},
  {timeout:5}
);

Some output:

1442966735705
...
1442966735710
1442966735710
1442966735710
1442966735710
1442966735710
Error: Script execution timed out.
    at Error (native)
    at ContextifyScript.Script.runInNewContext (vm.js:18:15)
    at Object.exports.runInNewContext (vm.js:49:17)
    at repl:1:4
    at REPLServer.defaultEval (repl.js:164:27)
    at bound (domain.js:250:14)
    at REPLServer.runBound [as eval] (domain.js:263:12)
    at REPLServer.<anonymous> (repl.js:392:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
> foo 1442966735715
foo 1442966735716
foo 1442966735716
foo 1442966735716
foo 1442966735716
... [continues forever]

I'm not really sure what a good solution for this would be, if it's possible, and whether the vm module intends to stand up to this sort of thing. At the very least I think vm's docs should have a note that the timeout property works by best-effort or something and isn't completely foolproof, so no one thinks it's safe to try to use it to sandbox user code in a guaranteed timely manner.

[apaprocki]

I added the original vm timeout capability. This gets pretty nasty.. the watchdog timer can only cover the execution time of the actual script passed in. If that script, during its execution, interacts with the Node/uv event queue and queues up more asynchronous callbacks (process.nextTick, setTimeout, setInterval) then those will execute later and the watchdog object will have already been destructed if the script executed in less time than the timeout.

One possible way this could be implemented is if the libuv could support something like mentioned in the comments interposed into the actual vm code:

    if (timeout != -1) {
      // query and save uv main loop reference count
      Watchdog wd(env, timeout);
      result = script->Run();
      // keep spinning uv event loop until reference count equals saved value (soft blocking)
    } else {

If the main loop ref count is equal to begin with, it will not block, the watchdog will destruct and everything will be fine. If the ref count is not equal, then that should indicate outstanding events have a ref on the loop and blocking should allow the watchdog to work as intended.

A side effect from this change would be that code setting up an indefinite listener would always hit the timeout. You would only be able to execute code that completely and fully finishes executing, including any interaction(s) with the event loop.

@bnoordhuis can you comment on the possibility of doing something like what I mentioned above in uv? I'm just throwing out that idea, but there may be other reasons why that is not possible. The only other way I could think of is very nasty and involves injecting wrappers for any functions that can add to the event queue and force a check against some pre-computed timestamp.

[vkurchatkin]

We can probably handle this by triggering microtask queue flush after vm invocation, though it uses C++ APIs so I'm not sure it can be interrupted properly.

[bnoordhuis]

All contexts share the same microtask queue (it's a per-isolate property) so I don't think this can be easily fixed, the promise callbacks can always enqueue new callbacks, ad infinitum.

[vkurchatkin]

@bnoordhuis right, there is now way to flush only microtask added inside of the new context. didn't think of that.

[apaprocki]

@bnoordhuis what I was thinking was that outstanding callbacks would ref the uv event loop, no? So if a way was added to block until ref count reached a desired value, then it would allow the main thread to block, thus allowing the watchdog to do its thing. If the code kept enqueuing callbacks and the count never reached the initial value then it would rightfully timeout, but behavior outside of the vm timeout case would not be affected in any way.

[bnoordhuis]

I don't think that could work. Contexts share the event loop. You could wait¹ until the event loop's reference count drops below a certain threshold but that doesn't tell you to what contexts events were delivered, unless you add bookkeeping to every call into the VM.

I don't think we want to go down that road and I'm not sure if it would even be enough. Microtasks, for example, are mostly outside of our control - they're driven by V8 - so there would still be loopholes.

1. In reality you can't right now because uv_run() is not re-entrant.

[vkurchatkin]

Closing, there's nothing that can be done about it

[ChALkeR]

If there is no reasonable way to fix this, then the documentation should have a notice about timeout not beeing reliable in some cases (and about the shared microtask queue, perhaps).

[vkurchatkin]

@ChALkeR oh, ok. saying that it's not reliable is not correct though, we should try to explain what it does and does not