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

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
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).
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR
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.

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

This comment has been minimized.

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.

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.

Copy link
Member Author

commented Apr 17, 2019

Landed in 83d1ca7

joyeecheung added a commit that referenced this pull request Apr 17, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.