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.

Sign up for GitHub

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

Jump to bottom

timers: make setImmediate() immune to tampering #17736

Closed
wants to merge 2 commits into from
Closed

timers: make setImmediate() immune to tampering #17736

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 18, 2017 •
edited
Loading

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/

@bnoordhuis
timers: make setImmediate() immune to tampering
c385829 
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 bnoordhuis mentioned this pull request Dec 18, 2017
Closed
@bnoordhuis
Copy link
Member Author

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

Fishrock123
Fishrock123 reviewed Dec 18, 2017
View reviewed changes
test/parallel/test-timer-immediate.js Outdated
@@ -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
@bnoordhuis @apapirovski
timers: make setImmediate() immune to tampering
ad02e0d 
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
@bnoordhuis @MylesBorins
timers: make setImmediate() immune to tampering
1e5571f 
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
Merged
@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 apapirovski mentioned this pull request Jan 31, 2018
Closed
3 tasks
@apapirovski apapirovski mentioned this pull request Feb 23, 2018
Closed
3 tasks
@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
@bnoordhuis @apapirovski
timers: make setImmediate() immune to tampering
cf39f43 
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
@bnoordhuis @addaleax
timers: make setImmediate() immune to tampering
839a3f7 
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
@bnoordhuis @addaleax
timers: make setImmediate() immune to tampering
81da433 
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
@bnoordhuis @MylesBorins
timers: make setImmediate() immune to tampering
8474f86 
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
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 31, 2018 •
edited
Loading

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
Reviewers

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@bnoordhuis @apapirovski @MylesBorins @jasnell @addaleax @Fishrock123 @cjihrig @tniessen @nodejs-github-bot