-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
lib: be robust when process global is clobbered #10135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,13 @@ | |
|
||
'use strict'; | ||
|
||
(function(process) { | ||
(function(global, process) { | ||
|
||
function startup(global, process) { | ||
// Expose the global object as a property on itself | ||
// (Allows you to set stuff on `global` from anywhere in JavaScript.) | ||
global.global = global; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this also be non-writable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thefourtheye Have you seen #10405 / https://tc39.github.io/proposal-global/? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh not yet. Thanks. I'll read that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (it should be non-enumerable though) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record: this is just the |
||
|
||
function startup() { | ||
const EventEmitter = NativeModule.require('events'); | ||
process._eventsCount = 0; | ||
|
||
|
@@ -196,7 +200,12 @@ | |
enumerable: false, | ||
configurable: true | ||
}); | ||
global.process = process; | ||
Object.defineProperty(global, 'process', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is needed / warranted. If someone wants to delete There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was requested by @sam-github in #10135 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I as well as @ljharb disagree with this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. basically, if we set this precedent I would be much more comfortable locking down all things that core could depend on, either by adding them to a closure ala https://github.com/bmeck/node/tree/no-globals or by freezing them on the global. There are many of these globals that break There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there many? https://nodejs.org/api/globals.html documents only a bit more than a dozen "globals", and half of them aren't global, they are module-scoped variables. Perhaps all the "globals" should be module-scoped variables? So that its impossible to mess with them globally? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this does not include prototype hijacking protection, thats raw global refs. |
||
value: process, | ||
writable: false, | ||
enumerable: true, | ||
configurable: true | ||
}); | ||
const util = NativeModule.require('util'); | ||
|
||
// Deprecate GLOBAL and root | ||
|
@@ -504,7 +513,7 @@ | |
}; | ||
|
||
NativeModule.wrapper = [ | ||
'(function (exports, require, module, __filename, __dirname) { ', | ||
'(function (exports, require, module, __filename, process) { ', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its confusing that there are two wrappers now, and they are different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I reading this correctly, internal modules no longer have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's the price of progress.
They never really did, it was always undefined because built-in modules don't live on disk.
I'm not against that per se but that's a bigger change and probably needs more discussion.
I believe it always was. It sort of has to be because it works relative to the calling script. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed it knew its current module using some other mechanism, but that makes sense. Also about __dirname. |
||
'\n});' | ||
]; | ||
|
||
|
@@ -520,7 +529,7 @@ | |
lineOffset: 0, | ||
displayErrors: true | ||
}); | ||
fn(this.exports, NativeModule.require, this, this.filename); | ||
fn(this.exports, NativeModule.require, this, this.filename, process); | ||
|
||
this.loaded = true; | ||
} finally { | ||
|
@@ -532,5 +541,5 @@ | |
NativeModule._cache[this.id] = this; | ||
}; | ||
|
||
startup(); | ||
startup(global, process); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/* eslint-disable no-global-assign */ | ||
'use strict'; | ||
|
||
require('../common'); | ||
const assert = require('assert'); | ||
|
||
assert.throws(() => process = null, /Cannot assign to read only property/); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should throw. Natives should be robust w/o relying on globals once startup finishes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The built-in libraries are, this checks that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that is appropriate to lock down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please give an example? I don't follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write this comment, I only moved it around. I can remove it if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it came from C++ code. I think we can remove this now.