lib: setup IPC channel before console #16562

seishun · 2017-10-27T18:49:01Z

Initializing IOCP on the same fd twice can fail on Windows.
Consequently, if the IPC channel uses fd 1 or 2 and the console is setup
first, writing to the IPC channel will fail.

Fixes: #16141

Will add a test if there are no objections to this approach.

Checklist

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

Affected core subsystem(s)

lib

addaleax

If there is no way to write a test for this, can you add a comment so that nobody accidentally moves this around again?

refack · 2017-10-27T19:26:00Z

AFAICT a regression test is simple:

d:\code\node-master$ set NODE_CHANNEL_FD=0

d:\code\node-master$ node
child_process.js:107
  p.open(fd);
    ^

Error: EINVAL: invalid argument, uv_pipe_open
    at Object.exports._forkChild (child_process.js:107:5)
    at Object.setupChannel (internal/process.js:244:8)
    at startup (bootstrap_node.js:69:16)
    at bootstrap_node.js:608:3

d:\code\node-master$ set NODE_CHANNEL_FD=

d:\code\node-master$

cjihrig

LGTM but a test would be great.

seishun · 2017-10-28T19:11:35Z

Added a test. @cjihrig @addaleax @refack PTAL

refack · 2017-10-28T20:53:08Z

test/parallel/test-child-process-stdout-ipc.js

+}
+
+const proc = spawn(process.execPath, [__filename, 'child'], {
+  stdio: ['inherit', 'ipc', 'inherit']


Suggestion:

// make sure `fd[1]` has not been opened for `console`

refack · 2017-10-28T20:54:07Z

lib/internal/bootstrap_node.js

@@ -74,6 +68,12 @@

    _process.setupRawDebug();

+    const browserGlobals = !process._noBrowserGlobals;
+    if (browserGlobals) {
+      setupGlobalTimeouts();


Suggestion:

// This needs to happen after `_process.setupChannel()`

There's a regression test and git blame, so I don't think it's necessary (or even useful in this form tbh).

bnoordhuis

LGTM FWIW

seishun · 2017-10-29T13:23:02Z

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

cjihrig · 2017-10-29T13:53:38Z

Still LGTM, thanks.

seishun · 2017-10-29T14:28:55Z

The failure seems unrelated.

Initializing IOCP on the same fd twice can fail on Windows. Consequently, if the IPC channel uses fd 1 or 2 and the console is setup first, writing to the IPC channel will fail. PR-URL: #16562 Fixes: #16141 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

seishun · 2017-10-29T19:02:01Z

Landed in 403ccb6.

Initializing IOCP on the same fd twice can fail on Windows. Consequently, if the IPC channel uses fd 1 or 2 and the console is setup first, writing to the IPC channel will fail. PR-URL: #16562 Fixes: #16141 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Initializing IOCP on the same fd twice can fail on Windows. Consequently, if the IPC channel uses fd 1 or 2 and the console is setup first, writing to the IPC channel will fail. PR-URL: nodejs/node#16562 Fixes: nodejs/node#16141 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-11-16T21:32:03Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

seishun · 2017-11-16T21:36:02Z

No need to backport since #16141 isn't an issue on v6.x AFAIK.

Initializing IOCP on the same fd twice can fail on Windows. Consequently, if the IPC channel uses fd 1 or 2 and the console is setup first, writing to the IPC channel will fail. PR-URL: nodejs/node#16562 Fixes: nodejs/node#16141 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 27, 2017

addaleax approved these changes Oct 27, 2017

View reviewed changes

refack approved these changes Oct 27, 2017

View reviewed changes

cjihrig approved these changes Oct 28, 2017

View reviewed changes

refack approved these changes Oct 28, 2017

View reviewed changes

addaleax approved these changes Oct 29, 2017

View reviewed changes

bnoordhuis approved these changes Oct 29, 2017

View reviewed changes

jasnell approved these changes Oct 29, 2017

View reviewed changes

seishun closed this Oct 29, 2017

seishun deleted the ipc-einval branch October 29, 2017 19:29

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

seishun added dont-land-on-v6.x and removed backport-requested-v6.x labels Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

lib: setup IPC channel before console #16562

lib: setup IPC channel before console #16562

Uh oh!

seishun commented Oct 27, 2017 •

edited by refack

Loading

Uh oh!

addaleax left a comment

Uh oh!

refack commented Oct 27, 2017

Uh oh!

cjihrig left a comment

Uh oh!

seishun commented Oct 28, 2017

Uh oh!

refack Oct 28, 2017

Uh oh!

refack Oct 28, 2017

Uh oh!

seishun Oct 29, 2017

Uh oh!

bnoordhuis left a comment

Uh oh!

seishun commented Oct 29, 2017

Uh oh!

cjihrig commented Oct 29, 2017

Uh oh!

seishun commented Oct 29, 2017

Uh oh!

seishun commented Oct 29, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

seishun commented Nov 16, 2017

Uh oh!

Uh oh!

Uh oh!

lib: setup IPC channel before console #16562

lib: setup IPC channel before console #16562

Uh oh!

Conversation

seishun commented Oct 27, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 27, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

seishun commented Oct 28, 2017

Uh oh!

refack Oct 28, 2017

Choose a reason for hiding this comment

Uh oh!

refack Oct 28, 2017

Choose a reason for hiding this comment

Uh oh!

seishun Oct 29, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

seishun commented Oct 29, 2017

Uh oh!

cjihrig commented Oct 29, 2017

Uh oh!

seishun commented Oct 29, 2017

Uh oh!

seishun commented Oct 29, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

seishun commented Nov 16, 2017

Uh oh!

Uh oh!

seishun commented Oct 27, 2017 •

edited by refack

Loading