Assigning a value to Array.prototype[1] will result in an error ：node:internal/process/task_queues:77 callback();

[LoongZP]

Version

v20.14.0

Platform

Microsoft Windows NT 10.0.22631.0
x64

Subsystem

No response

What steps will reproduce the bug?

code:
Array.prototype[1] = 2
console.log(Array.prototype);

result:

How often does it reproduce? Is there a required condition?

When I knew that Array.prototype was an Array, I tried to assign a value to Array.prototype[x].
Assigning only Array.prototype[1] results in an error that will occur when reading Array.prototype.

What is the expected behavior? Why is that the expected behavior?

In Chrome, you will get the following results：

Array.prototype[1] = 2
console.log(Array.prototype);

result:
[1: 2, at: ƒ, concat: ƒ, copyWithin: ƒ, fill: ƒ, find: ƒ, …]

What do you see instead?

code:
Array.prototype[1] = 2
console.log(Array.prototype);

result:
Object(2) [ <1 empty item>, 2 ]
node:internal/process/task_queues:77
callback();
^
TypeError: callback is not a function
at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

Node.js v20.14.0

Additional information

No response

[RedYetiDev]

I'm unable to reproduce in v22.6.0, but I haven't tested in v20. @LoongZP can you reproduce in the latest version of v20? (v20.16.0)

➜  ~ node
Welcome to Node.js v22.6.0.
Type ".help" for more information.
> Array.prototype[1] = 2
2
> console.log(Array.prototype);
Object(2) [ <1 empty item>, 2 ]
undefined
>

[LoongZP]

@RedYetiDev
The strange thing is that I also have no error when executing in interactive mode in node, it only occurs when node xx.js, which I also found in v22.3.0.

[RedYetiDev]

I'll retest later, but for now could you try to reproduce in the latest version of those release lines?

v22.6.0
v20.16.0

[LoongZP]

@RedYetiDev
I also tried it in ubuntu (node v20.11.1.) and found this issue as well.

[LoongZP]

@RedYetiDev
I've tried the following:

	os	node  version	result
my coputer	Microsoft Windows NT 10.0.22631.0   x64	22.6.0	x
my coputer	Microsoft Windows NT 10.0.22631.0   x64	22.3.0	x
my coputer	Microsoft Windows NT 10.0.22631.0   x64	20.14.0	x
other's coputer	Linux version 5.15.0-118-generic (buildd@lcy02-amd64-103) (gcc (Ubuntu 9.4.0-1ubuntu120.04.2) 9.4.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #12820.04.1-Ubuntu SMP Wed Jul 17 13:41:17 UTC 2024	20.11.1	x
other's coputer	Microsoft Windows NT 10.0.22631.0   x64	20.9.9	x

NOTE:
It's strange that there is no problem with assigning a value to Array.prototype[0/2/3/4...].

[RedYetiDev]

Array.prototype[1] = 2;
console.log(Array.prototype);

$ node repro.js
Object(2) [ <1 empty item>, 2 ]
node:internal/process/task_queues:77
          callback();
          ^

TypeError: callback is not a function
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

Node.js v22.6.0

---

Array.prototype[0] = 2;
console.log(Array.prototype);

$ node repro.js
Object(1) [ 2 ]

[LoongZP]

@RedYetiDev
So what is the reason, it's a very strange mistake.

[moorthid2023]

I think leaving first index cause the issue

Array.prototype[0]=2;
let problem = Array.prototype;
console.log(problem);

C:\Users\Moorthi\De
Object(1) [ 2 ]

[LoongZP]

I think leaving first index cause the issue

Array.prototype[0]=2; let problem = Array.prototype; console.log(problem);

C:\Users\Moorthi\De Object(1) [ 2 ]

No, you can try the code below:

Array.prototype[2] = 2;
let problem = Array.prototype;
console.log(problem);

[RedYetiDev]

Array.prototype[2] = 2;
let problem = Array.prototype;
console.log(problem);

The failing line is:

node/lib/internal/process/task_queues.js

Line 85 in 8b0c699

callback();

So something is calling that line incorrectly when Array.prototype[1] isn't what it should be...

I've added the console label, as this isn't reproducible without calling console.log.

---

The following prefixes each behave differently:

// Crashes
Array.prototype[1] = 1;

// Hangs
Array.prototype[1] = null;

// Works normally
Array.prototype[1] = undefined;

The error can be reproduced without console (but this function is called from the console.log call):

process.nextTick(()=>{});

[RedYetiDev]

@nodejs/process

[benjamingr]

It's just missing primordials but honestly I think it's fine? As long as the error message is outputted clearly and correctly I don't think Node makes (or should make) guarantees regarding operating correctly if builtins are modified like this.

[tniessen]

Parts of the Node.js runtime are implemented in JavaScript, and thus inherently susceptible to manipulations of built-in prototypes by user code. I don't think this is considered a bug by most collaborators.

[RedYetiDev]

I've added wontfix given the comments, feel free to adjust if needed.

[LoongZP]

Forgive me for not knowing much about the nodejs kernel, but I don't think it's a modification of the built-in function, Array.prototype is an object that I can add some functionality to. Although such code does not conform to the development specifications and is even somewhat stupid.
The problem now is that it gives an error message, but it doesn't help me locate the error in the code.
In the meantime, I gave Array.prototype[0/2/3....] There are no errors in the assignment, which I feel is not in line with the logic of the thinking and the consistency of the code.

[aduh95]

We should probably fix the following though:

$ echo 'Array.prototype[1]={callback(){console.log(1)}};console.log()' > repro.js
$ node repro.js

1
Error: async hook stack has become corrupted (actual: nan, expected: nan)
----- Native stack trace -----

 1: 0x100c0b70c node::AsyncHooks::FailWithCorruptedAsyncStack(double) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 2: 0x100c0b6cc node::AsyncHooks::FailWithCorruptedAsyncStack(double) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 3: 0x100bda210 node::AsyncWrap::PopAsyncContext(v8::FunctionCallbackInfo<v8::Value> const&) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 4: 0x100eda884 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, unsigned long*, int) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 5: 0x100eda030 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 6: 0x101834b24 Builtins_CEntry_Return1_ArgvOnStack_BuiltinExit [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 7: 0x1017ac3e4 Builtins_InterpreterEntryTrampoline [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 8: 0x1017ac3e4 Builtins_InterpreterEntryTrampoline [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
 9: 0x1017ac3e4 Builtins_InterpreterEntryTrampoline [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
10: 0x1017aa50c Builtins_JSEntryTrampoline [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
11: 0x1017aa1f4 Builtins_JSEntry [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
12: 0x100fe2830 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
13: 0x100fe2088 v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
14: 0x100e7e8dc v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
15: 0x100bcd08c node::InternalCallbackScope::Close() [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
16: 0x100bcca68 node::InternalCallbackScope::~InternalCallbackScope() [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
17: 0x100c69594 node::StartExecution(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
18: 0x100bd243c node::LoadEnvironment(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>, std::__1::function<void (node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Value>)>) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
19: 0x100cf6464 node::NodeMainInstance::Run(node::ExitCode*, node::Environment*) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
20: 0x100cf613c node::NodeMainInstance::Run() [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
21: 0x100c6ce94 node::Start(int, char**) [/nix/store/xr2x034ilq5g69n06hp7g54g0hawyl3n-nodejs-slim-20.15.1/bin/node]
22: 0x1849ae0e0 start [/usr/lib/dyld]

----- JavaScript stack trace -----

1: popAsyncContext (node:internal/async_hooks:559:12)
2: emitAfterScript (node:internal/async_hooks:521:3)
3: processTicksAndRejections (node:internal/process/task_queues:93:7)

[jazelly]

Maybe it's worth it to do a better error here for async_hooks? I don't think this can be completely prevented since userland can always pollute prototype.

[jakecastelli]

The reason why only Array.prototype[1] fails is because here, the first item (0 index) in the queue is a tick object which contains async hook info, so if you pollute the 0 index it will be overwritten, if you pollute index 2, because index 1 is undefined therefore it will just return, so the polluted item at index 2 never got retrieved.

Back to why this fails, because the polluted item does not have a async id, so it will crash when undefined is sent to binding layer through popAsyncContext function.

[jakecastelli]

My thought for fix this inline with @jazelly - IMO if async id is not a number (NaN) we should throw an error instead.

[joyeecheung]

Like any other (expected?) behavior caused by prototype pollution, essentially as long as there is some JS code expecting "querying a non-existent property leads to undefined", it can always be affected. Guarding higher level code isn't enough, it's mostly about guarding any non-existent/out-of-bound access (expect on user-provided objects).

Also IMO this isn't a bug most of the time, it's just an UX issue. At least it is better to keep console.log() working in the prototype polluted with accessors/traps, as it's possible for people to try to use it to debug unexpected accesses. In that case it's the implementation of console.log() that should be hardened. But it's not really worth it/possible to make all the other APIs work with prototype pollution, unless we try to move everything to C++ whereever possible.

[joyeecheung]

In the particular case of FixedCircularBuffer, it should just probably be guarded with a this.bottom < this.list.length bound check to avoid out-of-bound access.

node/lib/internal/fixed_queue.js

Lines 81 to 83 in d5dc540

	const nextItem = this.list[this.bottom];
	if (nextItem === undefined)
	return null;

[jakecastelli]

Just thought asking about the "out of bound access issue here" - the this.list will be kSize here which is 2048 when initialised. And this line here with bit mask should prevent the out of bound access issue.

[benjamingr]

We totally should fix the one Antoine pointed out (where node core dumps). Even if we don't do primordials everywhere for readability/performance we totally still should in error paths so users can tell why their code failed.

[joyeecheung]

new Array() doesn't actually initialize it, it'll need to be filled or otherwise it's still an out-of-bound access that leads to prototype lookup.

[joyeecheung]

Actually, this rings a bell - we should probably just forbid new Array with the linter. I made a similar argument against holey arrays in #52058 (comment) (it can also lead to worse performance in array accesses).

[targos]

we should probably just forbid new Array with the linter.

I agree. #54281 fixed a similar issue.

[jakecastelli]

it's still an out-of-bound access that leads to prototype lookup.

Much appreciated! Now I understood 🙏