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

timers: make setImmediate() immune to tampering #17736

Closed
wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 18, 2017

Make setImmediate() immune to process global tampering by removing
the dependency on the process._immediateCallback property.

Fixes: #17681
CI: https://ci.nodejs.org/job/node-test-commit/14914/
CI: https://ci.nodejs.org/job/node-test-commit/14918/

Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Fixes: nodejs#17681
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 18, 2017
@bnoordhuis
Copy link
Member Author

Interesting... CI linting fails but make lint passes for me locally.

@@ -1,5 +1,6 @@
'use strict';
const common = require('../common');
common.globalCheck = false;
// eslint-disable-next-line no-global-assign
process = {}; // Boom!
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can make it not need the comment by doing global.process = {} instead?

@apapirovski
Copy link
Member

Landed in ad02e0d

apapirovski pushed a commit that referenced this pull request Dec 26, 2017
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
@MylesBorins
Copy link
Contributor

This is breaking on v9.x, would you be able to manually backport?

/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release -J \
		default \
		addons addons-napi \
		doctool
=== release test-timer-immediate ===
Path: parallel/test-timer-immediate
module.js:703
  process._tickCallback();
          ^

TypeError: process._tickCallback is not a function
    at Function.Module.runMain (module.js:703:11)
    at startup (bootstrap_node.js:193:16)
    at bootstrap_node.js:617:3

@apapirovski
Copy link
Member

@MylesBorins this depends on #17198 which was defensively marked as semver-major but is something I personally disagree with. If you could have a look and give your opinion, that would be appreciated.

apapirovski pushed a commit to apapirovski/node that referenced this pull request Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

PR-URL: nodejs#17736
Fixes: nodejs#17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Feb 27, 2018
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 31, 2018

Should this be backported to v8.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.

This is potentially changing to behavior, even if not wanted behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants