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.

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

bootstrap: run preload prior to --frozen-intrinsics #28940

Closed
wants to merge 7 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Aug 2, 2019

This is used to allow people to run polyfills.

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

test/parallel/test-preload.js Outdated Show resolved Hide resolved
test/fixtures/intrinsic-mutation.js Show resolved Hide resolved
test/parallel/test-preload.js Outdated Show resolved Hide resolved
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Credit to @ljharb for suggesting this :)

@bmeck bmeck force-pushed the require-flag-before-freeze branch from 547e58b to 46c7534 Compare August 2, 2019 16:13
@bmeck bmeck changed the title bootstap: run preload prior to --frozen-intrinsics bootstrap: run preload prior to --frozen-intrinsics Aug 2, 2019
@nodejs-github-bot
Copy link
Collaborator

@ljharb
Copy link
Member

ljharb commented Aug 2, 2019

One quick nonblocker question - how might i feature detect that a) this flag was invoked, whether via NODE_OPTIONS or a cli flag), as well as b) that this PR’s change was available, from runtime JS?

@bmeck
Copy link
Member Author

bmeck commented Aug 2, 2019

@ljharb you can't detect if you are in a preload reliably to my knowledge, but you can detect the flag via process.execArgv (assuming something hasn't hidden that flag ahead of your check). You cannot test the ordering change here via runtime ops and I would be opposed to adding a way to check that since we do reorder things for experimental stuff all the time.

@nodejs-github-bot
Copy link
Collaborator

@ljharb
Copy link
Member

ljharb commented Aug 3, 2019

@bmeck process.execArgv won't contain the flag if it's set via NODE_OPTIONS, tho, i believe?

I'm not asking to test the ordering, more asking "how can i determine if my module that requires --require and --frozen-intrinsics is being used on a compatible version of node, so that I can give a helpful error message"

@joyeecheung
Copy link
Member

joyeecheung commented Aug 4, 2019

What would one do if they need to preload modules for purposes other than polyfilling but do not want the preloaded modules (or the modules they depend on) mess with the intrinsics?

@bmeck
Copy link
Member Author

bmeck commented Aug 4, 2019

@ljharb I suspect the use case you are talking about is having polyfills that have assurances that intrinsics may not be modified by other preloads so they can use intrinsics directly instead of saving cached copies.

@joyeecheung I suspect the use case you are talking about is replacing things like fs.readFile.

The answer to both is that it is not possible to have both mutable shared state and assurances about the integrity of that shared state. In general preloads are of conflicting interests in this timing and always have been the case regarding these 2 use cases.

Only preloads that do not harm the integrity of the runtime (by mutating core, or improperly polyfilling) should be loaded in general. To this end, preload modules should probably be whitelisted when using policies, but that is out of scope of this PR. Users of these could always freeze things themselves if they want to ensure other preloads cannot touch them, but that would prevent preloads from composing or patching improper behavior of other preloads.

Additionally, as core is hardened over time (using primordials and the like) it is likely to gain a similar flag to --frozen-intrinsics and mutating core modules can probably be seen as increasingly similar in nature.

@joyeecheung
Copy link
Member

@bmeck Can't we make the timing configurable? e.g. adding values to --frozen-intrinsics to control that.

@bmeck
Copy link
Member Author

bmeck commented Aug 4, 2019

@Joyee we could add my more times, but the problem would remain in the mutable timeframe so I don't think it actually does much / feel a different approach would be better (though I don't have a suggestion)

@devsnek
Copy link
Member

devsnek commented Aug 13, 2019

would --require=x --frozen-intrinsics --require=y, where the ordering is preserved, be too contrived of a solution?

@bmeck
Copy link
Member Author

bmeck commented Aug 13, 2019

@devsnek what is the use case, --require does stuff before user code and is generally mucking around mutating globals and the runtime. I'd expect it to be privileged since side effects are the main purpose of them.

@devsnek
Copy link
Member

devsnek commented Aug 13, 2019

@bmeck, I was just replying to the above question about configurable timing.

@bmeck
Copy link
Member Author

bmeck commented Aug 13, 2019

I think a separate PR adding an API that freezes intrinsics (noop after first freeze) would be fine for people not wishing to use the flag; but that has different implications than a well defined timing and ability to audit/grant policy configuration for what runs prior to the freeze. Perhaps exposing that API could be a separate PR if people want to control timing manually?

@sam-github
Copy link
Contributor

sam-github commented Aug 13, 2019

I don't have strong feelings on this, I just notice that an API would satisfy all timings.

// freeze.js
blah.freeze() // name TBD

satisfies:

node --require polyfill.js --require freeze.js --require not-allowed-to-polfill.js app.js

and with node app.js:

// app.js
require("polyfill")
blah.freeze() // name TBD
require("not-allowed-to-polyfill")
// ...

or any other time. I tend towards exposing mechanism, and leaving policy to users.

@bmeck
Copy link
Member Author

bmeck commented Aug 13, 2019

@sam-github while it has similar effects it does not have the same overall impact on policy configuration. Exposing via API requires programmer vigilance on understanding when it may be called rather than providing a clear timing. freeze.js may not even freeze things if polyfill.js is corrupted somehow and virtualized the freeze() API. I'm not clear on what we gain by leaving it up to users to understand how to protect freeze() and audit when freeze() could occur and. It seems like for education, configuration, and audit purposes having a well defined contract would be preferred.

@bmeck bmeck added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 13, 2019
@Trott
Copy link
Member

Trott commented Aug 20, 2019

Any reason to wait on landing this? @bmeck @joyeecheung

@joyeecheung
Copy link
Member

@Trott My previous comments are non-blocking.

@bmeck
Copy link
Member Author

bmeck commented Aug 20, 2019

@Trott seems like it is fine to land as nothing seems blocking.

bmeck added a commit that referenced this pull request Aug 20, 2019
This is used to allow people to run polyfills.

Co-Authored-By: Anna Henningsen <github@addaleax.net>

PR-URL: #28940
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bmeck
Copy link
Member Author

bmeck commented Aug 20, 2019

Landed in 85898e0

@bmeck bmeck closed this Aug 20, 2019
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
This is used to allow people to run polyfills.

Co-Authored-By: Anna Henningsen <github@addaleax.net>

PR-URL: #28940
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 19, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 23, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 27, 2019
codebytere added a commit to electron/electron that referenced this pull request Oct 14, 2019
codebytere added a commit to electron/electron that referenced this pull request Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants