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

Readline 'cursorTo' function incorrectly treats 'NaN' as a number #36301

Closed
unitario opened this issue Nov 28, 2020 · 3 comments · Fixed by #36379
Closed

Readline 'cursorTo' function incorrectly treats 'NaN' as a number #36301

unitario opened this issue Nov 28, 2020 · 3 comments · Fixed by #36379
Labels
readline Issues and PRs related to the built-in readline module.

Comments

@unitario
Copy link

unitario commented Nov 28, 2020

  • Version: v14.9.0
  • Platform: Darwin computer 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64
  • Subsystem: x86_64

What steps will reproduce the bug?

> node --print 'process.stdout.cursorTo(2, NaN)'

or

> node --print 'process.stdout.cursorTo(NaN, 2)'

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

Anytime NaN is passed as either x or y coordinates to the cursorTo function.

What is the expected behavior?

When y parameter is NaN, it should throw ERR_INVALID_CURSOR_POS, and log true in the terminal.

> node --print 'process.stdout.cursorTo(2, NaN)'
> true

When x parameter is NaN, the if statement for the data variable should evaluate to CSI${x + 1}G, and log true in the terminal.

> node --print 'process.stdout.cursorTo(NaN, 2)'
> true

What do you see instead?

> node --print 'process.stdout.cursorTo(2, NaN)'
> aN;1Htrue

or

> node --print 'process.stdout.cursorTo(NaN, 2)'
> aNHtrue

Additional information

Change:

/**
 * moves the cursor to the x and y coordinate on the given stream
 */

function cursorTo(stream, x, y, callback) {
  if (callback !== undefined && typeof callback !== 'function')
    throw new ERR_INVALID_CALLBACK(callback);

  if (typeof y === 'function') {
    callback = y;
    y = undefined;
  }

  if (stream == null || (typeof x !== 'number' && typeof y !== 'number')) {
    if (typeof callback === 'function')
      process.nextTick(callback, null);
    return true;
  }

  if (typeof x !== 'number')
    throw new ERR_INVALID_CURSOR_POS();

  const data = typeof y !== 'number' ? CSI`${x + 1}G` : CSI`${y + 1};${x + 1}H`;
  return stream.write(data, callback);
}

Into:

/**
 * moves the cursor to the x and y coordinate on the given stream
 */

function cursorTo(stream, x, y, callback) {
  if (callback !== undefined && typeof callback !== 'function')
    throw new ERR_INVALID_CALLBACK(callback);

  if (typeof y === 'function') {
    callback = y;
    y = undefined;
  }

  if (stream == null || (isNaN(x) && isNaN(y))) {
    if (typeof callback === 'function')
      process.nextTick(callback, null);
    return true;
  }

  if (isNaN(x))
    throw new ERR_INVALID_CURSOR_POS();

  const data = isNaN(y) ? CSI`${x + 1}G` : CSI`${y + 1};${x + 1}H`;
  return stream.write(data, callback);
}

Source: https://github.com/nodejs/node/blob/master/lib/readline.js

@unitario unitario changed the title Readline 'cursorTo' function incorrectly treats NaN as number Readline 'cursorTo' function incorrectly treats 'NaN' as a number Nov 28, 2020
@aduh95
Copy link
Contributor

aduh95 commented Nov 28, 2020

Do you want to send a Pull Request?

@unitario
Copy link
Author

Do you want to send a Pull Request?

Alright, I'll have a look at it. Weren't able to find the test-files for cursorTo but managed to do that now.

@Lxxyx
Copy link
Member

Lxxyx commented Nov 29, 2020

@targos targos added the readline Issues and PRs related to the built-in readline module. label Dec 6, 2020
@aduh95 aduh95 closed this as completed in 45dbcbe Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants