★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/27234Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

commented Apr 15, 2019 •
edited

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
@joyeecheung
src: disallow calling env-dependent methods during bootstrap 
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).
Verified
This commit was signed with a verified signature.
joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
Loading status checks…
aba0729
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 15, 2019

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3255

@nodejs-github-bot nodejs-github-bot added the C++ label Apr 15, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 15, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22441/
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 15, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22445/
@addaleax
addaleax approved these changes Apr 15, 2019
View changes

@joyeecheung joyeecheung added review wanted process labels Apr 15, 2019

@jasnell
jasnell approved these changes Apr 15, 2019
View changes

@joyeecheung joyeecheung added author ready and removed review wanted labels Apr 15, 2019

@BridgeAR
BridgeAR approved these changes Apr 15, 2019
View changes
Copy link
Member

left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 16, 2019

@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
bnoordhuis approved these changes Apr 16, 2019
View changes
@bnoordhuis

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 16, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 16, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 17, 2019 •
edited

Landed in 83d1ca7

@joyeecheung joyeecheung closed this Apr 17, 2019

joyeecheung added a commit that referenced this pull request Apr 17, 2019
@joyeecheung
src: disallow calling env-dependent methods during bootstrap 
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Verified
This commit was signed with a verified signature.
joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
Loading status checks…
83d1ca7
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis

@jasnell jasnell

@addaleax addaleax

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
C++ author ready process
Projects
None yet
Milestone
No milestone
6 participants
@joyeecheung @nodejs-github-bot @bnoordhuis @jasnell @addaleax @BridgeAR
You can’t perform that action at this time.