Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: do not add .domain to promises in VM contexts #15695

Closed
wants to merge 1 commit into from
Closed

src: do not add .domain to promises in VM contexts #15695

wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Sep 30, 2017 •
edited
Loading

The promises are still tracked, and their handlers will still execute in the correct domain. The creation domain is now simply hidden, in order to prevent objects from outside from leaking into a VM context.

Fixes (and see more context at): #15673

This is technically a breaking change, as it removes a property from created Promise objects under certain circumstances. However, I would argue that this should be landed as semver-patch, for the following reasons:

  • This feature breaks the contract of the VM module that, if one doesn't pass in any objects from the outside, the VM context is fully independent. In other words, it can be a bug of security implications despite the fact that we advertise VM module as not a full sandbox, which is duly noted both in documentation and by @bnoordhuis in Promise's domain property allows escaping VM #15673 (comment).
  • The core feature of domains -- preserving information across async boundaries -- is still available even after this PR.
  • Domain support for promises was introduced in v8.0.0, which means that most of the ecosystem probably have not yet started using the .domain property on promises.
  • Even if they have, the presence of .domain on Promises is inconsistent at best. Right now, the property exists only if it was created in a running domain, which means that modules that do not have control over domains (i.e. pretty much all modules except for the very core of a program) cannot depend on the presence of this property.
  • However, once v8.x goes LTS, this behavior will be near impossible to change.
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
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 30, 2017
@TimothyGu TimothyGu added domain Issues and PRs related to the domain subsystem. vm Issues and PRs related to the vm subsystem. labels Sep 30, 2017
addaleax
addaleax approved these changes Oct 1, 2017
View reviewed changes
doc/api/domain.md Outdated
@@ -1,6 +1,13 @@
# Domain
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/REPLACEME
description: `Promise`s created in VM contexts no longer have a `.domain`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the YAML parser has issues with values that start with a backtick? Can you double-check this works?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@TimothyGu
Copy link
Member Author

@bnoordhuis, do you have an opinion on this PR?

@addaleax
Copy link
Member

addaleax commented Oct 4, 2017

CI: https://ci.nodejs.org/job/node-test-commit/12783/

@TimothyGu
src: do not add .domain to promises in VM contexts
fd18704 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

Fixes: nodejs#15673
@TimothyGu
Copy link
Member Author

CI is green. Okay to land this with semver-patch? @nodejs/tsc

@addaleax
Copy link
Member

addaleax commented Oct 4, 2017

+1 to semver-patch

@TimothyGu
Copy link
Member Author

Landed in 8068577.

@TimothyGu TimothyGu closed this Oct 4, 2017
@TimothyGu TimothyGu deleted the domain-vm branch October 4, 2017 19:28
TimothyGu added a commit to TimothyGu/node that referenced this pull request Oct 4, 2017
@TimothyGu
src: do not add .domain to promises in VM contexts
8068577 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

PR-URL: nodejs#15695
Fixes: nodejs#15673
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

Not landing clean on 8.x, could you please backport?

@TimothyGu TimothyGu mentioned this pull request Oct 7, 2017
Closed
2 tasks
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
@TimothyGu @addaleax
src: do not add .domain to promises in VM contexts
8d727f1 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

PR-URL: nodejs/node#15695
Fixes: nodejs/node#15673
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@TimothyGu TimothyGu mentioned this pull request Oct 16, 2017
Closed
4 tasks
TimothyGu added a commit to TimothyGu/node that referenced this pull request Oct 22, 2017
@TimothyGu
src: do not add .domain to promises in VM contexts
3a7411f 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

Backport-PR-URL: REPLACEME
PR-URL: nodejs#15695
Fixes: nodejs#15673
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
evanlucas pushed a commit that referenced this pull request Oct 23, 2017
@TimothyGu @evanlucas
src: do not add .domain to promises in VM contexts
0b04869 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

Backport-PR-URL: #16074
PR-URL: #15695
Fixes: #15673
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. domain Issues and PRs related to the domain subsystem. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@TimothyGu @addaleax @MylesBorins @bnoordhuis @jasnell @cjihrig @lance @nodejs-github-bot