Should internal modules be affected by monkey-patching?

[hashseed]

Many internal modules call methods defined on prototype objects that can be modified (or monkey-patched) in userland. For example, by overriding Object.prototype.getOwnProperty, it is possible to affect async hooks.

One way to fix this is to keep copies of these functions at boostrap, and use these untampered copies inside internal modules.

Optionally, these copies can be exported through a new internal module so that user code can get hold of untampered copies if they intend to.

Advantages of this approach:

• The way internal modules are implemented remain implementation details and can be changed without risking breaking user code that somehow rely on monkey-patching.
• Internal modules behave predictably.
• Usage of JavaScript builtins no longer introduce hidden APIs to override behavior to internal modules.

Advantages of current code:

• Internal modules can be monkey-patched, e.g. for instrumentation.
• Internal modules implementation are easier to read.
• No additional overhead during bootstrap.

I think it is desirable to have consensus over whether internal modules should be affected by monkey-patching. This does not have to be a either/or decision.

Coming from the JavaScript spec, there are JavaScript builtins that are affected by monkey-patching too, intentionally. For example, by overriding RegExp.prototype.exec, the behavior of other RegExp builtins can be changed. Yet another example is the species constructor. Generally though, JavaScript builtins call into internal builtins that cannot be altered.

I think it is important to clearly specify and document which global state (i.e. result of monkey-patching) affect behavior of internal modules rather than having implicit APIs though monkey-patching. Personally, I think that - unless specified otherwise - monkey-patching should not affect internal modules. It needs to be a conscious design choice.

FWIW this problem would become very apparent if Node.js had a formal specification for its internal modules :)

Ref: #18773
CC: @benjamingr @devsnek

[benjamingr]

I think one of the most important points that you made in #18773 is:

All APIs that V8 offer in C++ use the original builtin. Including v8::Object::GetOwnPropertyNames. Let's say, just hypothetically, Node.js core wants to move the implementation of some functions from JS to C++, it then must not use v8::Object::GetOwnPropertyNames, but instead load and call global.Object.getOwnPropertyNames. Otherwise it would observably change behavior.

That is, every time we move code from/to C++ we break any patching userland has for the said call.

Now, another good thing to consider is that there have been cases where overriding built-ins has been tremendously useful when polyfilling - for example https://github.com/drses/weak-map/blob/master/weak-map.js (the WeakMap polyfill).

[devsnek]

if users want to patch fs.readFile or something that's cool (although I'm personally apposed to it without hooks) but changing Object.prototype.toString shouldnt change how fs.readFile works

[cjihrig]

I'm in favor of Node core's internals being resistant to monkey patching. I don't think we need to expose the untampered functions to users though.

[benjamingr]

@devsnek

if users want to patch fs.readFile or something that's cool (although I'm personally apposed to it without hooks) but changing Object.prototype.toString shouldnt change how far.readFile works

Probably, but it's more subtle than that - going back to the the WeakMap example case - users changed GetOwnPropertyNames since a new ("hidden") key was added to an object. That key had to be "secret" and not impact other methods - so if util.inspect was "tamper proof" then it would have been impossible to polyfill WeakMap entirely.

---

Note: Another thing we can do is to explicitly specify that changing built ins is dangerous and doing so is not supported in Node.js .

I'm not sure that's a good idea as it violates "least surprise" and the current behavior is unspecified anyway. Note that fixing this and ensuring builtins are used should be a pretty big code change and will create overhead in maintaining the code since basically built-in instance methods won't be used.

If we do this we need to make sure it doesn't impact performance (or improves it, since technically it's provable which function it is now), and that it's not problematic maintenance wise.

[mscdex]

I feel like this is a deep rabbit hole. Anyone could override anything, so now any and all object/prototype methods need to be saved up front, which is crazy because there are so many and they're used throughout the codebase. For example: string.trim(), string.toLowerCase(), string.slice(), Object.keys(), Number.isNaN(), etc.

Even if it was decided it's a good idea to do this, we'd need to also maintain a giant list of methods we use anywhere in node core.

[hashseed]

@mscdex are you concerned about memory use or time spent upon startup? One way to do it is to simply shallow clone these prototype objects.

[mscdex]

@hashseed I'm not even considering performance implications at this point. Just having to touch almost every line of code in node core, getting new collaborators to understand the change, maintaining these methods/prototypes, etc.

[benjamingr]

I wonder if there is a way from V8 land to "lock" the way Node core uses these objects - but I can't think of anything out of the box.

[bnoordhuis]

Just having to touch almost every line of code in node core

I don't have a problem with that if it's for the greater good. Most of it is rote search-and-replace, perhaps even mechanical replace.

[jasnell]

Could make for a good Code-and-Learn exercise :-)

[apapirovski]

My position on this aligns with @mscdex. There's some parts that are not completely unintuitive because people have likely seen hasOwnProperty.call(obj, 'something') before, but then there's stuff like Promise.prototype.then monkey-patching and needing to have PromiseThen that is honestly going to be nightmare to explain to new contributors.

Also, it's very likely that uncurryThis carries a performance penalty so switching over the entirety of core to use it will lead to a broad performance penalty for the whole of Node.js.

if we're going down this route, my preference is to try & find a path that leads to the core being able to lock or have our own versions of these primitives, as mentioned by @benjamingr.

I wonder if there is a way from V8 land to "lock" the way Node core uses these objects - but I can't think of anything out of the box.

I know this is a more complicated route (that might not be feasible with what we have available right now) but at least it would keep the code maintainable and easy to understand for new contributors. Having to write PromiseThen(promise, fn); is — to me — not an appealing proposition.

(And if we're comparing this to the C++ layer, having our own versions of Object, Promise, etc. would more closely resemble the C++ side than storing references to all the namespaced & prototype functions.)

---

Also, to further elaborate, this would still need to go hand in hand with being more defensive on anything being passed in from the users. For example, if a user is allowed to pass in a settings object then we shouldn't assume that it has hasOwnProperty or that it even has a non-null prototype.

This aligns with the recent transition to using Reflect.apply on user provided callbacks.

[hashseed]

having our own versions of Object, Promise, etc. would more closely resemble the C++ side than storing references to all the namespaced & prototype functions

This does not work, if you take user-defined objects as arguments and leak internal versions via return values.