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

process,tty: allow reading/writing from duplex sockets #23053

Closed
wants to merge 7 commits into from

Conversation

addaleax
Copy link
Member

As of libuv 1.23.1, the direction of OS-level streams is automatically detected, meaning that reading from stdout/stderr and writing to process.stdin are working again. Making this work completely (i.e. no error when the stream ends) requires some additional fixups to our stdio code.

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

Since faking TTY input is not otherwise fake-able, we need
support in the test runner for it.
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
Make sure that `process.stdin.write()`, and in particular
ending the stream, works.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 24, 2018
@addaleax addaleax added process Issues and PRs related to the process subsystem. tty Issues and PRs related to the tty subsystem. labels Sep 24, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 24, 2018

Is it worth to move the obsolete errors to the Legacy Node.js Error Codes section?

Refs: #22100

@@ -2065,6 +2078,13 @@ instance.setEncoding('utf8');
An attempt has been made to create a string larger than the maximum allowed
size.

<a id="ERR_TTY_WRITABLE_NOT_READABLE"></a>
### ERR_TTY_WRITABLE_NOT_READABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ### -> #### for level consistency)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done … this seems like something that our linter could maybe catch as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems, in this case, it will not be clear for a linter if this is a new level or just a typo.

@addaleax
Copy link
Member Author

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2018
@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

@addaleax addaleax added this to the 11.0.0 milestone Oct 4, 2018
@addaleax
Copy link
Member Author

addaleax commented Oct 4, 2018

Landed in 17a0cd2...8bf48bf

I’ve added this to the 11.0.0 milestone because it reverts a semver-major change that would otherwise be released in that version.

@addaleax addaleax closed this Oct 4, 2018
@addaleax addaleax deleted the tty-allow-read branch October 4, 2018 16:25
addaleax added a commit that referenced this pull request Oct 4, 2018
This reverts commit 91eec00.

Refs: #21654
Refs: #21203

PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Oct 4, 2018
Since faking TTY input is not otherwise fake-able, we need
support in the test runner for it.

PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Oct 4, 2018
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

PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Oct 4, 2018
Make sure that `process.stdin.write()`, and in particular
ending the stream, works.

PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
@refack refack added test Issues and PRs related to the tests. python PRs and issues that require attention from people who are familiar with Python. labels Oct 4, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
This reverts commit 91eec00.

Refs: #21654
Refs: #21203

PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Since faking TTY input is not otherwise fake-able, we need
support in the test runner for it.

PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
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

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

PR-URL: #23053
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Jan 22, 2019
addaleax added a commit that referenced this pull request Jan 27, 2019
This has been unused since cbc3ef6.

Refs: #23053

PR-URL: #25641
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax added a commit that referenced this pull request Jan 28, 2019
This has been unused since cbc3ef6.

Refs: #23053

PR-URL: #25641
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
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]>
@MylesBorins MylesBorins mentioned this pull request Mar 26, 2019
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
This has been unused since cbc3ef6.

Refs: #23053

PR-URL: #25641
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request May 10, 2019
This has been unused since cbc3ef6.

Refs: #23053

PR-URL: #25641
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
This has been unused since cbc3ef6.

Refs: #23053

PR-URL: #25641
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@ronag ronag mentioned this pull request Mar 20, 2020
4 tasks
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
Since faking TTY input is not otherwise fake-able, we need
support in the test runner for it.

PR-URL: nodejs/node#23053
Reviewed-By: James M Snell <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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/node#21203

PR-URL: nodejs/node#23053
Reviewed-By: James M Snell <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
Make sure that `process.stdin.write()`, and in particular
ending the stream, works.

PR-URL: nodejs/node#23053
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem. python PRs and issues that require attention from people who are familiar with Python. test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants