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

[jasnell]

@nodejs/documentation

[Knighton910]

we will put it on the docs todo list 📄

[benjamingr]

We need to fix this. cc @chrisdickinson

We can wrap .then calls for example which would still be than doing nothing.

[jeacott1]

doesnt even need a while loop. throwing an exception in .then with no catch is enough.

[laverdet]

Wouldn't this work?

1. Flush microtasks
2. Set up watchdogs
3. Run user script
4. Run microtasks again
5. Disable watchdogs

RunMicrotasks() will run until the job queue is 0, even if more jobs are scheduled or TerminateExecution() is called. In the TerminateExecution() case the jobs will just be canceled.

Obviously if you had any resolved promises their callbacks will be triggered by the vm library, I'm not sure how bad of a side effect that would be considered.

This seems to fix timeout for runaway promises:

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index ff66ffdaaa..c19d8e242e 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -857,22 +857,27 @@ class ContextifyScript : public BaseObject {
     Local<Value> result;
     bool timed_out = false;
     bool received_signal = false;
+    env->isolate()->RunMicrotasks();
     if (break_on_sigint && timeout != -1) {
       Watchdog wd(env->isolate(), timeout);
       SigintWatchdog swd(env->isolate());
       result = script->Run();
+      env->isolate()->RunMicrotasks();
       timed_out = wd.HasTimedOut();
       received_signal = swd.HasReceivedSignal();
     } else if (break_on_sigint) {
       SigintWatchdog swd(env->isolate());
       result = script->Run();
+      env->isolate()->RunMicrotasks();
       received_signal = swd.HasReceivedSignal();
     } else if (timeout != -1) {
       Watchdog wd(env->isolate(), timeout);
       result = script->Run();
+      env->isolate()->RunMicrotasks();
       timed_out = wd.HasTimedOut();
     } else {
       result = script->Run();
+      env->isolate()->RunMicrotasks();
     }
 
     if (try_catch->HasCaught()) {
@@ -896,6 +901,16 @@ class ContextifyScript : public BaseObject {
       try_catch->ReThrow();
 
       return false;
+    } else if (timed_out || received_signal) {
+      // `isolate->IsExecutionTerminating()` returns false which I suspect is a
+      // bug in v8, but `CancelTerminateExecution()` still works
+      env->isolate()->CancelTerminateExecution();
+      if (timed_out) {
+        env->ThrowError("Script execution timed out.");
+      } else {
+        env->ThrowError("Script execution interrupted.");
+      }
+      return false;
     }
 
     if (result.IsEmpty()) {

[Trott]

edit: so it looks like i made a miscalculation. emitting an error just throws, but its still inside the microtask. this leads to what i would consider a bug in v8, where uncaught exceptions in microtasks don't trigger our fatal exception handler. i've opened a bug report.

@devsnek Can you add a link to the bug report?

[Trott]

edit: so it looks like i made a miscalculation. emitting an error just throws, but its still inside the microtask. this leads to what i would consider a bug in v8, where uncaught exceptions in microtasks don't trigger our fatal exception handler. i've opened a bug report.

@devsnek Can you add a link to the bug report?

Is this the right bug report? https://bugs.chromium.org/p/v8/issues/detail?id=8465

[devsnek]

@Trott maybe? I also opened https://crbug.com/v8/8326 a bit ago.

the feature was shimmed in 2caf079

[Trott]

the feature was shimmed in 2caf079

Does that mean it may now be possible to fix things so that https://github.com/nodejs/node/blob/27dc74fdd0950d39d175145400a70174244870d9/test/known_issues/test-vm-timeout-escape-queuemicrotask.js is no longer a known_issue test and passes?

[devsnek]

@Trott it will still fail stopping at 5ms, but it should throw at 100ms instead of silently stopping.

[lundibundi]

The issue itself is a wontfix as noted before and was documented by 5e5a945.
Feel free to reopen if there is anything else to do here.

[addaleax]

Fwiw, I’m working on a fix for this, for the Promise case, so I’ll reopen this.

(The queueMicrotask() and nextTick() examples are not really fixable, but I wouldn’t consider them bugs or actual issues either, just misunderstandings about what those functions do.)