tty,doc: add type-check to isatty by bengl · Pull Request #15567 · nodejs/node

bengl · 2017-09-23T08:49:26Z

Previously, various inputs other than non-negative integers would
produce incorrect results.

Added type-checking on input, returning false for anything other than
non-negative integers.

Also clarified in docs.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tty, doc

bengl · 2017-09-23T08:51:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10216/

lpinca · 2017-09-24T07:08:06Z

Where does this fall on the semver scale? Bugfix?

bengl · 2017-09-24T19:04:07Z

@lpinca Yes, I'd call it a patch/bugfix, since it fixes a previously incorrect API.

BridgeAR

LGTM. It would be nice if the messages could be improved though, but it is not a must.

BridgeAR · 2017-09-24T19:41:34Z

test/pseudo-tty/test-tty-isatty.js

The messages here are somewhat confusing out of my perspective. Maybe write something like e.g.
-1 reported to be a tty but should not?

Yeah I'll clarify these.

BridgeAR · 2017-09-24T19:43:29Z

This is definitely semver-patch

> isatty(-2)
/home/ruben/.nvm/versions/node/v8.5.0/bin/node[2433]: ../src/tty_wrap.cc:109:static void node::TTYWrap::IsTTY(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `(fd) >= (0)' failed.
 1: node::Abort() [node]
 2: node::Assert(char const* const (*) [4]) [node]
 3: 0x13e0f97 [node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
 5: 0xc1365a [node]
 6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 7: 0x3d4bf3a846fd
Aborted (core dumped)

bengl · 2017-09-24T20:13:26Z

@BridgeAR I think the latest change clarifies it?

BridgeAR · 2017-09-24T20:15:58Z

Definitely! Thanks a lot

jasnell · 2017-09-25T16:04:54Z

Defensively marked as semver-major but I'd be ok with semver-minor or patch.

BridgeAR · 2017-09-26T04:19:27Z

@nodejs/tsc please weight in about semver-major / semver-patch.

BridgeAR · 2017-09-28T21:30:21Z

Ping @nodejs/tsc

Trott · 2017-09-29T16:53:52Z

doc/api/tty.md

Seems like there ought to be clearer wording than not a non-negative integer. How about just a negative integer?

I'd also be fine with simply removing the additional text here, as a negative number is not a valid fd. But since a negative integer can be returned from a sys call to retrieve a file descriptor if there is an error, I'm OK with leaving it too for explicitness.

Trott · 2017-09-29T16:56:51Z

Seems patch to me. I'm in favor of "when in doubt, mark it semver-major" but I'm not seeing any real doubt on this one.

BridgeAR · 2017-10-01T23:05:18Z

CI https://ci.nodejs.org/job/node-test-pull-request/10366/

BridgeAR · 2017-10-02T02:27:11Z

Seems like there are some related failures

https://ci.nodejs.org/job/node-test-commit-osx/12738/nodes=osx1010/console

ok 1753 pseudo-tty/test-stderr-stdout-handle-sigwinch
  ---
  duration_ms: 0.89
  ...
length differs.
expect=0
actual=13
patterns:
outlines:
outline = assert.js:44
outline = throw new errors.AssertionError({
outline = ^
outline = AssertionError [ERR_ASSERTION]: 5 reported to be a tty, but it is not
outline = at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/pseudo-tty/test-tty-isatty.js:12:1)
outline = at Module._compile (module.js:600:30)
outline = at Object.Module._extensions..js (module.js:611:10)
outline = at Module.load (module.js:521:32)
outline = at tryModuleLoad (module.js:484:12)
outline = at Function.Module._load (module.js:476:3)
outline = at Function.Module.runMain (module.js:641:10)
outline = at startup (bootstrap_node.js:187:16)
outline = at bootstrap_node.js:605:3
not ok 1754 pseudo-tty/test-tty-isatty
  ---
  duration_ms: 0.79
  severity: fail
  stack: |-
    assert.js:44
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 5 reported to be a tty, but it is not
        at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/pseudo-tty/test-tty-isatty.js:12:1)
        at Module._compile (module.js:600:30)
        at Object.Module._extensions..js (module.js:611:10)
        at Module.load (module.js:521:32)
        at tryModuleLoad (module.js:484:12)
        at Function.Module._load (module.js:476:3)
        at Function.Module.runMain (module.js:641:10)
        at startup (bootstrap_node.js:187:16)
        at bootstrap_node.js:605:3

addaleax · 2017-10-04T20:55:42Z

+1 to semver-patch

Fishrock123 · 2017-10-19T18:05:43Z

Does process.binding('tty_wrap').isTTY not type-check? How does a non-fd get seen as an fd today by this?

Fishrock123

lgtm I guess

BridgeAR · 2017-10-19T18:30:15Z

Ping @bengl

bengl · 2017-10-19T18:59:05Z

Cool I'll land this later today.

@Fishrock123 process.binding('tty_wrap').isTTY currently does some type-checking: https://github.com/nodejs/node/blob/master/src/tty_wrap.cc#L108-L109

Unfortunately, giving it a negative number results in an abort (#15567 (comment)), which can't be handled in JS. Also, giving it a non-number always returns true, due to Int32Value().

BridgeAR · 2017-10-19T19:02:22Z

@bengl the CI reported a error that looked related. See my former comment #15567 (comment).

bengl · 2017-10-19T19:04:39Z

@BridgeAR Oh weird! I'll take a look at that first.

Previously, various inputs other than non-negative integers would produce incorrect results. Added type-checking on input, returning false for anything other than non-negative integers. Also clarified in docs.

bengl · 2017-10-19T23:28:57Z

Looks like 5 is potentially a pseudo tty in those tests. Switched it to a pretty huge number that shouldn't ever be a pseudo tty in those tests.

New CI: https://ci.nodejs.org/job/node-test-pull-request/10864/

bengl · 2017-10-20T01:14:47Z

CI failures seem unrelated. @BridgeAR LGTY?

Previously, various inputs other than non-negative integers would produce incorrect results. Added type-checking on input, returning false for anything other than non-negative integers. Also clarified in docs. PR-URL: #15567 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

bengl · 2017-10-22T23:53:22Z

Landed in 27e12e7

Previously, various inputs other than non-negative integers would produce incorrect results. Added type-checking on input, returning false for anything other than non-negative integers. Also clarified in docs. PR-URL: #15567 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Previously, various inputs other than non-negative integers would produce incorrect results. Added type-checking on input, returning false for anything other than non-negative integers. Also clarified in docs. PR-URL: nodejs/node#15567 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins · 2017-11-16T20:58:51Z

Since this fixes a core dump I've gone ahead and backported to v6.x

please lmk if it should be backed out

Previously, various inputs other than non-negative integers would produce incorrect results. Added type-checking on input, returning false for anything other than non-negative integers. Also clarified in docs. PR-URL: #15567 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Previously, various inputs other than non-negative integers would produce incorrect results. Added type-checking on input, returning false for anything other than non-negative integers. Also clarified in docs. PR-URL: nodejs/node#15567 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label Sep 23, 2017

lpinca approved these changes Sep 24, 2017

View reviewed changes

BridgeAR approved these changes Sep 24, 2017

View reviewed changes

bengl force-pushed the bengl/isatty branch from cc90cfb to 534b323 Compare September 24, 2017 20:11

BridgeAR approved these changes Sep 24, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 25, 2017

jasnell approved these changes Sep 25, 2017

View reviewed changes

BridgeAR requested a review from a team September 26, 2017 04:19

Trott reviewed Sep 29, 2017

View reviewed changes

BridgeAR removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 30, 2017

cjihrig approved these changes Oct 7, 2017

View reviewed changes

Fishrock123 approved these changes Oct 19, 2017

View reviewed changes

tty,doc: add type-check to isatty

3310cc0

Previously, various inputs other than non-negative integers would produce incorrect results. Added type-checking on input, returning false for anything other than non-negative integers. Also clarified in docs.

bengl force-pushed the bengl/isatty branch from 534b323 to 3310cc0 Compare October 19, 2017 23:26

BridgeAR approved these changes Oct 22, 2017

View reviewed changes

bengl closed this Oct 22, 2017

MylesBorins added the lts-watch-v6.x label Nov 16, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

Uh oh!

Conversation

bengl commented Sep 23, 2017

Checklist

Affected core subsystem(s)

Uh oh!

bengl commented Sep 23, 2017

Uh oh!

lpinca commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bengl commented Sep 24, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Sep 24, 2017

Choose a reason for hiding this comment

Uh oh!

bengl Sep 24, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 24, 2017

Uh oh!

bengl commented Sep 24, 2017

Uh oh!

BridgeAR commented Sep 24, 2017

Uh oh!

jasnell commented Sep 25, 2017

Uh oh!

BridgeAR commented Sep 26, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

Trott Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Oct 1, 2017

Uh oh!

BridgeAR commented Oct 2, 2017

Uh oh!

addaleax commented Oct 4, 2017

Uh oh!

Fishrock123 commented Oct 19, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Oct 19, 2017

Uh oh!

bengl commented Oct 19, 2017

Uh oh!

BridgeAR commented Oct 19, 2017

Uh oh!

bengl commented Oct 19, 2017

Uh oh!

bengl commented Oct 19, 2017

Uh oh!

bengl commented Oct 20, 2017

Uh oh!

bengl commented Oct 22, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lpinca commented Sep 24, 2017 •

edited

Loading

Trott commented Sep 29, 2017 •

edited

Loading