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

child_process: add safety checks on stdio access #3799

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 12, 2015

When a child process is spawned, there is no guarantee that stdout and stderr will be created successfully. This commit adds checks before attempting to access the streams.

This is related to #1321. It prevents Node from synchronously throwing TypeError: Cannot read property 'addListener' of undefined. Instead, an async error, EAGAIN is provided to the callback.

When a child process is spawned, there is no guarantee that stdout
and stderr will be created successfully. This commit adds checks
before attempting to access the streams.
@jasnell
Copy link
Member

jasnell commented Nov 12, 2015

LGTM

@jasnell jasnell added child_process Issues and PRs related to the child_process subsystem. lts-watch-v4.x labels Nov 12, 2015
@mscdex
Copy link
Contributor

mscdex commented Nov 12, 2015

LGTM

});
if (child.stdout) {
if (encoding)
child.stdout.setEncoding(encoding);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a behavior change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I just moved the code up a few lines to avoid another if (child.stdout) check.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh sorry, missed that.

@evanlucas
Copy link
Contributor

LGTM

cjihrig added a commit that referenced this pull request Nov 13, 2015
When a child process is spawned, there is no guarantee that stdout
and stderr will be created successfully. This commit adds checks
before attempting to access the streams.

PR-URL: #3799
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Nov 13, 2015

Landed in 7b355c5

@jasnell jasnell closed this Nov 13, 2015
@cjihrig cjihrig deleted the stdio-checks branch November 13, 2015 04:38
cjihrig added a commit that referenced this pull request Nov 13, 2015
When a child process is spawned, there is no guarantee that stdout
and stderr will be created successfully. This commit adds checks
before attempting to access the streams.

PR-URL: #3799
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit that referenced this pull request Nov 30, 2015
When a child process is spawned, there is no guarantee that stdout
and stderr will be created successfully. This commit adds checks
before attempting to access the streams.

PR-URL: #3799
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit that referenced this pull request Dec 4, 2015
When a child process is spawned, there is no guarantee that stdout
and stderr will be created successfully. This commit adds checks
before attempting to access the streams.

PR-URL: #3799
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Dec 17, 2015
cjihrig added a commit that referenced this pull request Dec 17, 2015
When a child process is spawned, there is no guarantee that stdout
and stderr will be created successfully. This commit adds checks
before attempting to access the streams.

PR-URL: #3799
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit that referenced this pull request Dec 23, 2015
When a child process is spawned, there is no guarantee that stdout
and stderr will be created successfully. This commit adds checks
before attempting to access the streams.

PR-URL: #3799
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Evan Lucas <evanlucas@me.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
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants