lib: make Error objects instantiation less vulnerable to prototype pollution

[aduh95]

Having a more robust Error instantiation makes sure the user would get the actual error rather than an unrelated one if the prototype was altered somewhere else.

Before this change:

$ node -e 'Object.defineProperty(Object.prototype, "code", {set(){throw new Error}});fs.readFile()'              
[eval]:1
Object.defineProperty(Object.prototype, "code", {set(){throw new Error}});fs.readFile()
                                                       ^

Error
    at TypeError.set ([eval]:1:62)
    at new NodeError (node:internal/errors:401:16)
    at __node_internal_ (node:internal/validators:421:11)
    at maybeCallback (node:fs:169:3)
    at Object.readFile (node:fs:372:14)
    at [eval]:1:78
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:307:38)
    at node:internal/process/execution:83:21
    at [eval]-wrapper:6:24

Node.js v19.3.0

After this change:

$ node -e 'Object.defineProperty(Object.prototype, "code", {set(){throw new Error}});fs.readFile()'
node:internal/validators:421
    throw new ERR_INVALID_ARG_TYPE(name, 'Function', value);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "cb" argument must be of type function. Received undefined
    at maybeCallback (node:fs:169:3)
    at Object.readFile (node:fs:372:14)
    at [eval]:1:78
    at Script.runInThisContext (node:vm:128:12)
    at Object.runInThisContext (node:vm:306:38)
    at node:internal/process/execution:83:21
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:82:62)
    at evalScript (node:internal/process/execution:104:10)
    at node:internal/main/eval_string:50:3 {
  code: 'ERR_INVALID_ARG_TYPE'
}

Node.js v20.0.0-pre

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/loaders
• @nodejs/modules
• @nodejs/net
• @nodejs/test_runner

[ljharb]

Suggested change

	const codes = ObjectCreate(null);
	const codes = { __proto__: null };

this is equally robust and avoids a function call.

[JakobJingleheimer]

I do prefer this, but I know Antoine prefers the other, and the diff is pretty "6 of one".

[ljharb]

Why the preference?

Even setting aside that Object.create is a regretted addition to the JS language, it's still got the potential overhead of a function call, as compared to the very straightforward syntax that node already uses with objects initialized with more than one property. Why is "no properties" a special case that requires a function call?

[RaisinTen]

it's still got the potential overhead of a function call

I don't think that should be a concern here because lib/internal/errors.js is already a part of the snapshot, so this code won't get executed at startup, rather the context would get deserialized from the snapshot.

[ljharb]

ok, fair - it’s still indirection, and it’s using an API call for something that’s available with syntax.

[GeoffreyBooth]

This is a style question that’s out of scope for this PR. If someone feels strongly about this, they should open a new PR that updates our eslint to enforce a particular style.

[ljharb]

yes thanks, i'm sure that won't get bogged down in blocks, i'll get right on that

[ljharb]

opened here: #46083

[ljharb]

(that was landed, so this can be resolved)

[ljharb]

Suggested change

	const descriptors = ObjectCreate(null);
	const descriptors = { __proto__: null };

[aduh95]

Benchmak CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1287/

Results

                               confidence improvement accuracy (*)   (**)  (***)
error/error.js n=10000000                     -2.16 %       ±2.30% ±3.06% ±3.99%
error/node-error.js n=10000000        ***     -8.85 %       ±2.53% ±3.37% ±4.39%

[BridgeAR]

I do not think we should do this. It increases the implementation complexity signficantly and reduces the performance of a critical part of the application.

[mcollina]

The slowdown is not acceptable in this specific case.
Errors are already a slow path for us given all the modifications we do to them, and we should not slow them down further.