doc, test: document and test vm timeout escapes #23743

jasnell · 2018-10-18T23:44:40Z

Finally getting back to this one ;-)

Using process.nextTick(), Promise, or queueMicrotask(), it
is possible to escape the timeout set when running code with
vm.runInContext(), vm.runInThisContext(), and
vm.runInNewContext().

This documents the issue and adds three known_issues tests.

Refs: #3020

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-10-18T23:44:41Z

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1288/pipeline

jasnell · 2018-10-18T23:52:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17980/

lpinca · 2018-10-19T06:44:42Z

test/known_issues/test-vm-timeout-escape-nexttick.js

+    // it to fail reliably
+    if ((current - start) / NS_PER_MS >= 50n) {
+      throw new Error(
+        `escaped timeout at ${(current - start) / NS_PER_MS} milliseconds!`);


Nit: how about caching the result instead of recomputing it?

vsemozhetbyt · 2018-10-19T16:33:19Z

I've run the example in all supported versions, last nightly and canary and every time I've got:

Error: Script execution timed[ out after 5ms]

and then the node exited and no infinite loops happened. Would it be confusing for a reader that would test the example?

jasnell · 2018-10-19T17:01:31Z

@vsemozhetbyt ... I assume you're copying the example to a file then running it? If so, the unhandled error crashes the process. I've removed the second call to loop() in the example, which will prevent that and should show the infinite loop in action.

vsemozhetbyt · 2018-10-19T17:05:32Z

Yes, I often test examples in file scripts)

TimothyGu

In the documentation, I would additionally drive home the fact that the timeout is not a security sandboxing mechanism, and that a more comprehensive solution could be running the untrusted code in a complete separate process. Overall looks good.

jasnell · 2018-10-19T17:38:34Z

@TimothyGu ... there is already a warning in there for that, I believe. Doesn't mean it can't be made better but let's save that for a different PR.

jasnell · 2018-10-19T17:38:43Z

Please 👍 to fast-track

jasnell · 2018-10-23T19:57:26Z

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18083/

jasnell · 2018-10-23T21:18:43Z

Unfortunately the new tests here are flaky. Will have to investigate

jasnell · 2018-10-23T22:14:44Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/18089/

Using `process.nextTick()`, `Promise`, or `queueMicrotask()`, it is possible to escape the `timeout` set when running code with `vm.runInContext()`, `vm.runInThisContext()`, and `vm.runInNewContext()`. This documents the issue and adds three known_issues tests. Refs: nodejs#3020

These are known issues that can be flaky on certain platforms because they rely entirely on timing differences.

jasnell · 2018-10-24T15:07:08Z

Unfortunately, the known_issue tests appear to be inherently flaky due to the reliance on timing differences for them to fail. Marking them flaky in a separate commit. Not sure if there's a way to make them fail more reliably but having the known_issue tests in there, flaky or otherwise, is still generally a good idea.

New CI: https://ci.nodejs.org/job/node-test-pull-request/18117/

Using `process.nextTick()`, `Promise`, or `queueMicrotask()`, it is possible to escape the `timeout` set when running code with `vm.runInContext()`, `vm.runInThisContext()`, and `vm.runInNewContext()`. This documents the issue and adds three known_issues tests. Refs: #3020 PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

These are known issues that can be flaky on certain platforms because they rely entirely on timing differences. PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell · 2018-10-24T20:54:41Z

Landed in 5e5a945 and 095a602

Using `process.nextTick()`, `Promise`, or `queueMicrotask()`, it is possible to escape the `timeout` set when running code with `vm.runInContext()`, `vm.runInThisContext()`, and `vm.runInNewContext()`. This documents the issue and adds three known_issues tests. Refs: #3020 PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

These are known issues that can be flaky on certain platforms because they rely entirely on timing differences. PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Trott · 2018-11-06T06:29:13Z

Looks like the test may be flaky...https://ci.nodejs.org/job/node-test-commit-smartos/21423/nodes=smartos16-64/console I'll open an issue.

MylesBorins · 2018-11-26T22:16:24Z

will hold off on landing in LTS until it is clearly not flaky

Trott · 2018-11-27T05:41:33Z

Flakiness was addressed (partially at least?) in 8e3c5b5 and a4a2e9f.

Trott · 2018-11-27T05:42:23Z

test-vm-timeout-escape-promise is still marked as flaky in the status file. Not sure how often it actually flakes, though.

Using `process.nextTick()` or `Promise`, it is possible to escape the `timeout` set when running code with `vm.runInContext()`, `vm.runInThisContext()`, and `vm.runInNewContext()`. This documents the issue and adds two known_issues tests. Refs: #3020 PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

These are known issues that can be flaky on certain platforms because they rely entirely on timing differences. PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Using `process.nextTick()` or `Promise`, it is possible to escape the `timeout` set when running code with `vm.runInContext()`, `vm.runInThisContext()`, and `vm.runInNewContext()`. This documents the issue and adds two known_issues tests. Refs: #3020 PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

These are known issues that can be flaky on certain platforms because they rely entirely on timing differences. PR-URL: #23743 Refs: #3020 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell added doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem. known limitation Issues that are identified as known limitations. labels Oct 18, 2018

lpinca approved these changes Oct 19, 2018

View reviewed changes

TimothyGu approved these changes Oct 19, 2018

View reviewed changes

jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 19, 2018

vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 19, 2018

jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2018

jasnell force-pushed the doc-vm-timeout-escape2 branch from ac777ae to 5066c53 Compare October 23, 2018 22:13

addaleax removed the fast-track PRs that do not need to wait for 48 hours to land. label Oct 24, 2018

jasnell added 2 commits October 24, 2018 08:04

test: mark test-vm-timeout-* known issue tests flaky

47a2cb5

These are known issues that can be flaky on certain platforms because they rely entirely on timing differences.

jasnell force-pushed the doc-vm-timeout-escape2 branch from 5066c53 to 47a2cb5 Compare October 24, 2018 15:05

jasnell closed this Oct 24, 2018

MylesBorins added dont-land-on-v8.x labels Nov 26, 2018

codebytere added backported-to-v10.x and removed lts-watch-v10.x labels Dec 13, 2018

codebytere mentioned this pull request Jan 4, 2019

v10.15.1 proposal #25346

Merged

richardlau mentioned this pull request Jan 11, 2019

AIX - intermittent? - known_issues/test-vm-timeout-escape-promise #24519

Closed

sam-github added flaky-test Issues and PRs related to the tests with unstable failures on the CI. linux Issues and PRs related to the Linux platform. labels Jun 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc, test: document and test vm timeout escapes #23743

doc, test: document and test vm timeout escapes #23743

jasnell commented Oct 18, 2018

nodejs-github-bot commented Oct 18, 2018

jasnell commented Oct 18, 2018

lpinca Oct 19, 2018

vsemozhetbyt commented Oct 19, 2018

jasnell commented Oct 19, 2018

vsemozhetbyt commented Oct 19, 2018

TimothyGu left a comment

jasnell commented Oct 19, 2018

jasnell commented Oct 19, 2018

jasnell commented Oct 23, 2018

jasnell commented Oct 23, 2018

jasnell commented Oct 23, 2018

jasnell commented Oct 24, 2018

jasnell commented Oct 24, 2018

Trott commented Nov 6, 2018

MylesBorins commented Nov 26, 2018 •

edited

Loading

Trott commented Nov 27, 2018

Trott commented Nov 27, 2018

doc, test: document and test vm timeout escapes #23743

doc, test: document and test vm timeout escapes #23743

Conversation

jasnell commented Oct 18, 2018

Checklist

nodejs-github-bot commented Oct 18, 2018

jasnell commented Oct 18, 2018

lpinca Oct 19, 2018

Choose a reason for hiding this comment

vsemozhetbyt commented Oct 19, 2018

jasnell commented Oct 19, 2018

vsemozhetbyt commented Oct 19, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

jasnell commented Oct 19, 2018

jasnell commented Oct 19, 2018

jasnell commented Oct 23, 2018

jasnell commented Oct 23, 2018

jasnell commented Oct 23, 2018

jasnell commented Oct 24, 2018

jasnell commented Oct 24, 2018

Trott commented Nov 6, 2018

MylesBorins commented Nov 26, 2018 • edited Loading

Trott commented Nov 27, 2018

Trott commented Nov 27, 2018

MylesBorins commented Nov 26, 2018 •

edited

Loading