src: ensure that fd 0-2 are valid on windows by bzoz · Pull Request #11863 · nodejs/node

bzoz · 2017-03-15T12:22:01Z

Check that stdin, stdout and stderr are valid file descriptors on Windows. If not, reopen them with nul file.

This is a port of #875 for Windows.

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)

src

Check that stdin, stdout and stderr are valid file descriptors on Windows. If not, reopen them with 'nul' file. Refs: nodejs#875 Fixes: nodejs#11656

bzoz · 2017-03-15T16:51:16Z

cc @nodejs/platform-windows

bzoz · 2017-03-15T17:06:13Z

CI failed on win2008 and 2012, I'll investigate.

jasnell · 2017-03-15T23:33:02Z

ping @bnoordhuis

addaleax

Looks good from a distance ;) It’s been too long since I worked with Windows APIs to give this a proper LGTM, even if it’s only a few lines of code.

addaleax · 2017-03-15T23:41:57Z

test/fixtures/spawn_closed_stdio.py

+os.close(0)
+os.close(1)
+os.close(2)
+cmd = sys.argv[1] + ' -e "process.stdin; process.stdout; process.stderr;"'


I think I would prefer it if the arguments came from the test file, i.e. you just spawn sys.argv[1] here and move the -e … bits to test-stdio-closed.js

If you do use -e, write this as cmd = sys.argv[1:2] + ['-e', 'process.stdin; ...'] so it gets escaped properly.

addaleax · 2017-03-15T23:44:32Z

src/node.cc

 #endif  // __POSIX__
+#ifdef _WIN32
+  for (int fd = 0; fd <= 2; ++fd) {
+    auto handle = (HANDLE) _get_osfhandle(fd);


Can you use the appropriate C++-style cast here, whichever one that is?

bnoordhuis · 2017-03-16T07:30:34Z

test/fixtures/spawn_closed_stdio.py

+os.close(0)
+os.close(1)
+os.close(2)
+cmd = sys.argv[1] + ' -e "process.stdin; process.stdout; process.stderr;"'


If you do use -e, write this as cmd = sys.argv[1:2] + ['-e', 'process.stdin; ...'] so it gets escaped properly.

bnoordhuis · 2017-03-16T07:36:16Z

test/parallel/test-stdio-closed.js

 if (common.isWindows) {
-  common.skip('platform not supported.');
+  const python = process.env.PYTHON || 'python';
+  const script = path.join(__dirname, '..', 'fixtures',


path.join(common.fixturesDir, 'spawn_closed_stdio.py')?

bzoz · 2017-03-16T16:42:27Z

Updated, CI is green, PTAL

bnoordhuis

LGTM with a question.

bnoordhuis · 2017-03-16T17:09:09Z

src/node.cc

+    auto handle = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+    if (handle == INVALID_HANDLE_VALUE ||
+        GetFileType(handle) == FILE_TYPE_UNKNOWN) {
+      _close(fd);


_close() can't fail?

It fails on Windows 2008 and 2012 but succeeds on Windows 10. IMHO the cleanest way is to just assert that _open returned proper fd.

Is it known why it can fail on 2008/2012 and can that be worked around at all? I'm good with this as is but I'm curious :-)

Win2008 and 2012 fail with errno EBADF. Win 10 happily closes the handle. But regardless of that, _open works as expected on all Win versions.

@bzoz actually, having this explanation as a comment in the source seems like a good idea to me?

Fishrock123

Uh not really sure about my review on this, CI passes and stuff I guess? ¯\_(ツ)_/¯

bzoz · 2017-03-20T11:22:12Z

@addaleax I've added a comment about _close

addaleax · 2017-03-20T19:52:29Z

Landed in bd496e0

Check that stdin, stdout and stderr are valid file descriptors on Windows. If not, reopen them with 'nul' file. Refs: #875 Fixes: #11656 PR-URL: #11863 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2017-04-18T19:51:40Z

landed in v4.x and v6x staging
lmk if this shouldn't have landed

Check that stdin, stdout and stderr are valid file descriptors on Windows. If not, reopen them with 'nul' file. Refs: #875 Fixes: #11656 PR-URL: #11863 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs#11863 adds _O_RDWR to node.cc which is defined in fcntl.h. This adds this include directly to node.cc.

#11863 adds _O_RDWR to node.cc which is defined in fcntl.h. This adds this include directly to node.cc. PR-URL: #12540 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Check that stdin, stdout and stderr are valid file descriptors on Windows. If not, reopen them with 'nul' file. Refs: nodejs/node#875 Fixes: nodejs/node#11656 PR-URL: nodejs/node#11863 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs/node#11863 adds _O_RDWR to node.cc which is defined in fcntl.h. This adds this include directly to node.cc. PR-URL: nodejs/node#12540 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com>

src: ensure that fd 0-2 are valid on windows

15d4f8c

Check that stdin, stdout and stderr are valid file descriptors on Windows. If not, reopen them with 'nul' file. Refs: nodejs#875 Fixes: nodejs#11656

bzoz added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Mar 15, 2017

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 15, 2017

mscdex added the process Issues and PRs related to the process subsystem. label Mar 15, 2017

bzoz added the wip Issues and PRs that are still a work in progress. label Mar 15, 2017

jasnell requested review from Fishrock123, addaleax and bnoordhuis March 15, 2017 23:34

addaleax reviewed Mar 15, 2017

View reviewed changes

bnoordhuis requested changes Mar 16, 2017

View reviewed changes

bzoz added 2 commits March 16, 2017 15:45

fixup: cast in cpp, works on win2008, python things

a73e803

fixup: more changes

db01a43

bzoz removed the wip Issues and PRs that are still a work in progress. label Mar 16, 2017

bnoordhuis approved these changes Mar 16, 2017

View reviewed changes

jasnell approved these changes Mar 17, 2017

View reviewed changes

Fishrock123 approved these changes Mar 17, 2017

View reviewed changes

addaleax approved these changes Mar 19, 2017

View reviewed changes

addaleax added lts-watch-v4.x lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 19, 2017

fixup: add comment about _close

5d563cf

cjihrig approved these changes Mar 20, 2017

View reviewed changes

addaleax closed this Mar 20, 2017

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Apr 18, 2017

This was referenced Apr 19, 2017

v6.10.3 proposal #12498

Merged

v4.8.3 proposal #12499

Merged

bzoz added a commit to JaneaSystems/node that referenced this pull request Apr 20, 2017

src: add fcntl.h include to node.cc

8c34036

nodejs#11863 adds _O_RDWR to node.cc which is defined in fcntl.h. This adds this include directly to node.cc.

bzoz mentioned this pull request Apr 20, 2017

src: add fcntl.h include to node.cc #12540

Closed

2 tasks

joaocgreis mentioned this pull request Sep 6, 2017

WARNING: Jenkins update to 2.60.1 requires Java 8 nodejs/build#775

Closed

4 tasks

ehsanen mentioned this pull request Feb 1, 2018

Getting Nodejs to work on standalone C++ application on Windows nodejs/help#1087

Closed

Uh oh!

Conversation

bzoz commented Mar 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

bzoz commented Mar 15, 2017

Uh oh!

bzoz commented Mar 15, 2017

Uh oh!

jasnell commented Mar 15, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Mar 16, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Mar 20, 2017

Uh oh!

addaleax commented Mar 20, 2017

Uh oh!

MylesBorins commented Apr 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Fishrock123 left a comment •

edited

Loading