Skip to content
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

The Node.js will stuck when exec Object.defineProperty(Array.prototype, '-1', {get: function(){return this[this.length - 1]}}) in REPL #36669

Closed
Me7426 opened this issue Dec 28, 2020 · 2 comments
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. stream Issues and PRs related to the stream subsystem.

Comments

@Me7426
Copy link

Me7426 commented Dec 28, 2020

What steps will reproduce the bug?

Object.defineProperty(Array.prototype, '-1', {get: function(){return this[this.length - 1]}})

When exec these code in REPL, the Node.js will let cpu occupancy rate become 100% and stuck itself.

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

100% reproduce.

What is the expected behavior?

What do you see instead?

> Object.defineProperty(Array.prototype, '-1', {get: function(){return this[this.length - 1]}})
Object(0) []
> 

Additional information

It can be reproduce in Node.js v14.8.0 on my Android's termux.

@aduh95
Copy link
Contributor

aduh95 commented Dec 28, 2020

I can reproduce this. Here is the output with the NODE_DEBUG=* in case that's useful:

REPL 38605: line "Object.defineProperty(Array.prototype, \"-1\", { get() { throw new Error(-1); }, enumerable: false });"
REPL 38605: eval "Object.defineProperty(Array.prototype, \"-1\", { get() { throw new Error(-1); }, enumerable: false });\n"
REPL 38605: finish null Object(0) []
STREAM 38603: readableAddChunk <Buffer 4f 62 6a 65 63 74 28 30 29 20 5b 5d 0a>
STREAM 38603: maybeReadMore read 0
STREAM 38603: read 0
STREAM 38603: need readable true
STREAM 38603: length less than watermark true
STREAM 38603: do read
NET 38603: _read
STREAM 38603: readableAddChunk <Buffer 3e 20>
STREAM 38603: maybeReadMore read 0
STREAM 38603: read 0
STREAM 38603: need readable true
STREAM 38603: length less than watermark true
STREAM 38603: do read
NET 38603: _read

For reference, here's the output when setting -2 instead of -1 on Array.prototype:

REPL 38468: line "Object.defineProperty(Array.prototype, \"-2\", { get() { throw new Error(-1); }, enumerable: false });"
REPL 38468: eval "Object.defineProperty(Array.prototype, \"-2\", { get() { throw new Error(-1); }, enumerable: false });\n"
REPL 38468: finish null Object(0) []
STREAM 38467: readableAddChunk <Buffer 4f 62 6a 65 63 74 28 30 29 20 5b 5d 0a>
STREAM 38467: maybeReadMore read 0
STREAM 38467: read 0
STREAM 38467: need readable true
STREAM 38467: length less than watermark true
STREAM 38467: do read
STREAM 38468: maybeReadMore read 0
STREAM 38468: read 0
NET 38467: _read
STREAM 38468: need readable true
STREAM 38468: length less than watermark true
STREAM 38468: do read
NET 38468: _read
STREAM 38467: readableAddChunk <Buffer 3e 20>
STREAM 38467: maybeReadMore read 0
STREAM 38467: read 0
STREAM 38467: need readable true
STREAM 38467: length less than watermark true
STREAM 38467: do read
NET 38467: _read

@aduh95 aduh95 added confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. stream Issues and PRs related to the stream subsystem. labels Dec 28, 2020
@Lxxyx
Copy link
Member

Lxxyx commented Dec 29, 2020

Try to find the problem through the Profile. And I found out why. The problem is caused by repl/domain/process.domain._errorHandler together and only affects Object.defineProperty(Array.prototype, "-1").

Profile

image

Error Code

node/lib/domain.js

Lines 240 to 241 in e57d8af

while (exports.active === this) {
this.exit();

node/lib/domain.js

Lines 314 to 317 in e57d8af

Domain.prototype.exit = function() {
// Don't do anything if this domain is not on the stack.
const index = ArrayPrototypeLastIndexOf(stack, this);
if (index === -1) return;

Details

  1. Start Node.js Repl and type Object.defineProperty(Array.prototype, "-1", { get() { throw new Error(-1); }, enumerable: false }). The self.eval(cmd) function will be executed

node/lib/repl.js

Lines 833 to 834 in e57d8af

debug('eval %j', evalCmd);
self.eval(evalCmd, self.context, getREPLResourceName(), finish);

  1. self.eval is wrapper of node.js domain.

    self.eval = self._domain.bind(eval_);

    this._domain = options.domain || domain.create();

  2. execute self.eval will run domain.enter and domain.exit

    node/lib/domain.js

    Lines 414 to 420 in e57d8af

    function bound(_this, self, cb, fnargs) {
    self.enter();
    const ret = ReflectApply(cb, _this, fnargs);
    self.exit();
    return ret;
    }

  3. at domain.exit, will set exports.active to stack[stack.length - 1]. But we defined Array. prototype[-1] getter before, so this will trigger an error.

    exports.active = stack[stack.length - 1];

  4. The error is UncaughtException, so error will be caught by Domain.prototype._errorHandler.

    node/lib/readline.js

    Lines 1197 to 1202 in e57d8af

    } catch (err) {
    // If the generator throws (it could happen in the `keypress`
    // event), we need to restart it.
    stream[ESCAPE_DECODER] = emitKeys(stream);
    stream[ESCAPE_DECODER].next();
    throw err;

    Domain.prototype._errorHandler = function(er) {

  5. In Domain.prototype._errorHandler, there is a while loop and domain.exit() will be called. But the stack is empty, so the while will loop indefinitely and cause the process to get stuck

    node/lib/domain.js

    Lines 240 to 241 in e57d8af

    while (exports.active === this) {
    this.exit();

    node/lib/domain.js

    Lines 314 to 317 in e57d8af

    Domain.prototype.exit = function() {
    // Don't do anything if this domain is not on the stack.
    const index = ArrayPrototypeLastIndexOf(stack, this);
    if (index === -1) return;

aduh95 added a commit to aduh95/node that referenced this issue Dec 29, 2020
@jasnell jasnell closed this as completed in 83b428a Jan 9, 2021
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Fixes: #36669

PR-URL: #36676
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue May 1, 2021
Fixes: #36669

PR-URL: #36676
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants