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

[v8.x] Backport tty fixes #25351

Closed
wants to merge 4 commits into from
Closed

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jan 5, 2019

Fixes: #25204

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

addaleax and others added 4 commits January 5, 2019 12:44
Since faking TTY input is not otherwise fake-able, we need
support in the test runner for it.

PR-URL: nodejs#23053
Reviewed-By: James M Snell <[email protected]>
Allow reading from stdio streams that are conventionally
associated with process output, since this is only convention.

This involves disabling the oddness around closing stdio
streams. Its purpose is to prevent the file descriptors
0 through 2 from being closed, since doing so can lead
to information leaks when new file descriptors are being
opened; instead, not doing anything seems like a more
reasonable choice.

Fixes: nodejs#21203

PR-URL: nodejs#23053
Reviewed-By: James M Snell <[email protected]>
Make sure that `process.stdin.write()`, and in particular
ending the stream, works.

PR-URL: nodejs#23053
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#23051
Fixes: nodejs#22814
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: George Adams <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mcollina mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 5, 2019
@mcollina mcollina requested a review from addaleax January 5, 2019 12:22
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. v8.x labels Jan 5, 2019
@mcollina
Copy link
Member Author

cc @nodejs/lts is this something that is of interest in a future 8.x release? Or should this be closed?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@BethGriggs
Copy link
Member

BethGriggs pushed a commit that referenced this pull request Mar 19, 2019
Since faking TTY input is not otherwise fake-able, we need
support in the test runner for it.

Backport-PR-URL: #25351
PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Mar 19, 2019
Allow reading from stdio streams that are conventionally
associated with process output, since this is only convention.

This involves disabling the oddness around closing stdio
streams. Its purpose is to prevent the file descriptors
0 through 2 from being closed, since doing so can lead
to information leaks when new file descriptors are being
opened; instead, not doing anything seems like a more
reasonable choice.

Fixes: #21203

Backport-PR-URL: #25351
PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Mar 19, 2019
Make sure that `process.stdin.write()`, and in particular
ending the stream, works.

Backport-PR-URL: #25351
PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Mar 19, 2019
Backport-PR-URL: #25351
PR-URL: #23051
Fixes: #22814
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: George Adams <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs
Copy link
Member

Landed on v8.x-staging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants