Repository navigation
Freezing EventTarget.prototype breaks MessageChannel #49259
Description
Version
v19.9.0
Platform
Linux WORKSTATION 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27 02:56:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
node:internal/worker/io:305:30
What steps will reproduce the bug?
Run the following snippet:
Object.freeze(EventTarget.prototype);
new MessageChannel();
How often does it reproduce? Is there a required condition?
It always reproduces
What is the expected behavior? Why is that the expected behavior?
Freezing prototypes should not affect the behaviour of native methods. In particular, user code should not interfere with internals.
What do you see instead?
node:internal/worker/io:305
eventEmitter[kNewListener] = function(size, type, ...args) {
^
TypeError: Cannot assign to read only property 'Symbol(kNewListener)' of object '#<MessagePort>'
at setupPortReferencing (node:internal/worker/io:305:30)
at MessagePort.oninit (node:internal/worker/io:232:3)
at Object.<anonymous> (/tmp/tmp.ddwmESs9uN.js:2:1)
at Module._compile (node:internal/modules/cjs/loader:1275:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1329:10)
at Module.load (node:internal/modules/cjs/loader:1133:32)
at Module._load (node:internal/modules/cjs/loader:972:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
at node:internal/main/run_main_module:23:47
Additional information
No response
Activity
bnoordhuis commentedon Aug 20, 2023
That's something node tries to do when it's reasonable but not at all costs. Trying to work around a tampered EventEmitter prototype likely impacts performance and is not a good trade-off.
Clobbering EventEmitter is setting yourself up for failure anyway. MessageChannel almost certainly isn't the only thing that's affected.
benjamingr commentedon Aug 20, 2023
@addaleax thoughts?
corrideat commentedon Aug 21, 2023
That's probably right, although in this case at least it does seem like an issue of leaking implementation internals into userspace.
Looking at the code where it fails, it seems fixable by using
Object.definePropertyinstead of assignment (see the 'override mistake'). Note: I haven't tested whether usingObject.definePropertywould fix this.If this theory is correct, the fix would be replacing
eventEmitter[kNewListener] = function(size, type, ...args) { (...)byObjectDefineProperty(eventEmitter, kNewListener, function(size, type, ...args) { (...), although the same needs to be done forkRemoveListeneras well askCurrentlyReceivingPorts. A more general fix but significantly larger change would be not storing these in the same object that is user-facing.Freezing the ports themselves also breaks things (in a different place) due to the same issue of modifying the underlying object from the implementation. Although I'd prefer that not to be the case either, I realise that that'd a much larger fix involving how functionality is implemented in Node, some of which may even be relied on by existing applications.
However, a stronger argument is perhaps that freezing prototypes, in general, and in this particular case, doesn't suddenly break the runtime unexpectedly in other platforms, such as browsers or Deno (since
EventTargetis part of the DOM).benjamingr commentedon Aug 21, 2023
@corrideat see https://www.youtube.com/watch?v=SOPC3aLoD4U for context
worker: protect against user mutating well-known prototypes
worker: protect against user mutating well-known prototypes
worker: protect against user mutating well-known prototypes
worker: protect against user mutating well-known prototypes
worker: protect against user mutating well-known prototypes