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

src: disallow calling env-dependent methods during bootstrap #27234

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 15, 2019

These cannot be preserved correctly in v8 snapshot. Currently
none of these are called during bootstrap, this adds assertions
to make sure future contributors do not accidentally call
these in the wrong time.

Consider this, on the machine that builds releases:

process.cwd();  // "/home/iojs/build/workspace/"

User downloads this binary to their machine:

$ cd ~/
$ pwd  # "/User/foo"
$ node -p "process.cwd()" # Cached "/home/iojs/build/workspace/"

This patch only adds checks in methods that get states from the
environment - it's not likely that the setters would be called
during bootstrap, and if they are called, we'll just ignore them
and whatever tests that test the change would fail when snapshot
is enabled. However the getters may be called in order
to persist information into strings and that would be harder
to catch (the test is only likely to test the format of these
strings which won't be useful).

Refs: #27224

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

These cannot be preserved correctly in v8 snapshot. Currently
none of these are called during bootstrap, this adds assertions
to make sure future contributors do not accidentally call
these in the wrong time.

Consider this, on the machine that builds releases:

```
process.cwd();  // "/home/iojs/build/workspace/"
```

User downloads this binary to their machine:

```
$ cd ~/
$ pwd  // "/User/foo"
$ node -p "process.cwd()" // "/home/iojs/build/workspace/"
```

This patch only adds checks in methods that get states from the
environment - it's not likely that the setters would be called
during bootstrap, and if they are called, we'll just ignore them
and whatever tests that test the change would fail when snapshot
is enabled. However the getters may be called in order
to persist information into strings and that would be harder
to catch (the test is only likely to test the format of these
strings which won't be useful).
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 15, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added review wanted PRs that need reviews. process Issues and PRs related to the process subsystem. labels Apr 15, 2019
@joyeecheung joyeecheung added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels Apr 15, 2019
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. Would it also be sufficient to use a DCHECK instead? Our CI should at least detect these and the check does not have to be executed in the default case anymore. But I do not know if this check is expensive or not (I guess it's just a simple state check?).

@joyeecheung
Copy link
Member Author

@BridgeAR The check should be cheap enough, at least negligible in the context of the methods affected. Since most people do not develop with debug builds locally (I believe), using CHECK helps them discover this sooner.

@bnoordhuis
Copy link
Member

Is process.title another candidate? Calling its getter or setter during bootstrapping is probably wrong (it'll be blown away) and if caching was ever added for whatever reason, it'd be doubly wrong.

@joyeecheung
Copy link
Member Author

@bnoordhuis The process properties that depend on run time states (e.g. process.title, process.pid) are now only added during pre-execution in PatchProcessObject - so for instance, in bootstrap/node.js process.pid is undefined.

The process methods are still included in the bootstrap because the function themselves do not depend on run time states, only the results of their invocation do - I guess we could also attach the accessor properties earlier, but until we have snapshot enabled there is not much difference in terms of overhead.

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 17, 2019

Landed in 83d1ca7

joyeecheung added a commit that referenced this pull request Apr 17, 2019
These cannot be preserved correctly in v8 snapshot. Currently
none of these are called during bootstrap, this adds assertions
to make sure future contributors do not accidentally call
these in the wrong time.

Consider this, on the machine that builds releases:

```
process.cwd();  # "/home/iojs/build/workspace/"
```

If `process.cwd()` is cached as in
#27224, when the user
downloads this binary to their machine:

```
$ cd ~/
$ pwd  # "/User/foo"
$ node -p "process.cwd()" # "/home/iojs/build/workspace/"
```

This patch only adds checks in methods that get states from the
environment - it's not likely that the setters would be called
during bootstrap, and if they are called, we'll just ignore them
and whatever tests that test the change would fail when snapshot
is enabled. However the getters may be called in order
to persist information into strings and that would be harder
to catch (the test is only likely to test the format of these
strings which won't be useful).

PR-URL: #27234
Refs: #27224
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants