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

Accessing stdin with file descriptor 0 on Windows causes exception if there's no input / if stdin is closed #19831

Open
mklement0 opened this issue Apr 5, 2018 · 24 comments

Comments

@mklement0
Copy link

@mklement0 mklement0 commented Apr 5, 2018

  • Version: v9.11.1
  • Platform: Windows 10 Pro (64-bit; v10.0.15063)
  • Subsystem: fs module

On Windows, run the following from a cmd.exe console ("Command Prompt"):

# Echo stdin input

# OK: Nonempty stdin input.
echo hi | node -pe "require('fs').readFileSync(0).toString()"

# BREAKS ON WINDOWS: no stdin input, which should *prompt* for it when run in a terminal.
node -pe "require('fs').readFileSync(0).toString()"

# BREAKS ON WINDOWS: stdin input closed; should return an empty line.
node -pe "require('fs').readFileSync(0).toString()" < NUL

The last 2 commands break as follows:

fs.js:531
    binding.fstat(fd);
            ^

Error: EISDIR: illegal operation on a directory, fstat
    at tryStatSync (fs.js:531:13)
    at Object.fs.readFileSync (fs.js:567:3)
...

Using .readFile() and the readline module is equally affected.

On Unix platforms, the behavior is as expected in all cases.

@jens1o

This comment has been minimized.

Copy link

@jens1o jens1o commented Apr 5, 2018

Can reproduce this on Win.

@ryzokuken

This comment has been minimized.

Copy link
Member

@ryzokuken ryzokuken commented Apr 5, 2018

Just confirmed, works on both Linux and MacOS. @mklement0 @jens1o could you try a few other versions of Node? Does this also happen on LTS?

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 5, 2018

Window 7 x64.
Throws with Node.js 06.14.0, 08.11.0, 09.11.1.
OK with the master (Node.js 10.0.0. 2018-04-03 nightly).

@ryzokuken

This comment has been minimized.

Copy link
Member

@ryzokuken ryzokuken commented Apr 5, 2018

@vsemozhetbyt if it's okay with nightly, can't we just find out which commit "accidentally" fixed it and backport it?

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 5, 2018

I can try to brute-force-bisect)

@mklement0 mklement0 changed the title Accessing stdin with file descriptor 0 on windows causes exception if there's no input / if stdin is closed Accessing stdin with file descriptor 0 on Windows causes exception if there's no input / if stdin is closed Apr 5, 2018
@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 5, 2018

Unbreak point:

> node.v10.0.0-nightly20180116f75bc2c1a5.exe -pe "require('fs').readFileSync(0).toString()"
fs.js:634
    binding.fstat(fd);
            ^

Error: EBADF: bad file descriptor, fstat
    at tryStatSync (fs.js:634:13)
    at Object.fs.readFileSync (fs.js:670:3)
    at [eval]:1:15
    at Script.runInThisContext (vm.js:64:33)
    at Object.runInThisContext (vm.js:185:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:660:30)
    at evalScript (bootstrap_node.js:484:27)
    at startup (bootstrap_node.js:170:9)
    at bootstrap_node.js:640:3

> node.v10.0.0-nightly20180116f75bc2c1a5.exe -pe "require('fs').readFileSync(0).toString()" < NUL
fs.js:634
    binding.fstat(fd);
            ^

Error: EINVAL: invalid argument, fstat
    at tryStatSync (fs.js:634:13)
    at Object.fs.readFileSync (fs.js:670:3)
    at [eval]:1:15
    at Script.runInThisContext (vm.js:64:33)
    at Object.runInThisContext (vm.js:185:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:660:30)
    at evalScript (bootstrap_node.js:484:27)
    at startup (bootstrap_node.js:170:9)
    at bootstrap_node.js:640:3

> node.v10.0.0-nightly201801171b0d9795b6.exe -pe "require('fs').readFileSync(0).toString()"
123
^Z
123

> node.v10.0.0-nightly201801171b0d9795b6.exe -pe "require('fs').readFileSync(0).toString()" < NUL

>
@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 5, 2018

So it seems some of these commits may have the fix:

f75bc2c...1b0d979

@ryzokuken

This comment has been minimized.

Copy link
Member

@ryzokuken ryzokuken commented Apr 5, 2018

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 5, 2018

Let's see what @nodejs/fs think)

@bzoz

This comment has been minimized.

Copy link
Contributor

@bzoz bzoz commented May 16, 2018

This was fixed in 8c00a80 but then it was broken again in #20167. Now we get:

>node -pe "require('fs').readFileSync(0).toString()"
fs.js:163
  return (stats[1/* mode */] & S_IFMT) === fileType;
               ^

TypeError: Cannot read property '1' of undefined
    at isFileType (fs.js:163:16)
    at Object.fs.readFileSync (fs.js:455:7)
    at [eval]:1:15
    at Script.runInThisContext (vm.js:91:20)
    at Object.runInThisContext (vm.js:298:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:678:30)
    at evalScript (internal/bootstrap/node.js:542:27)
    at startup (internal/bootstrap/node.js:204:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)

/cc @joyeecheung

EDIT: it only reproduces on Windows, on Linux it works fine

@bzoz bzoz mentioned this issue May 16, 2018
2 of 4 tasks complete
@joyeecheung

This comment has been minimized.

Copy link
Member

@joyeecheung joyeecheung commented May 16, 2018

@bzoz Thanks for the ping. Looks like it's a corner case of tryStatSync, I can think of a few ways to fix this:

  1. Special case for fd 0, 1, 2 and do not check S_IFREG with fstat
  2. Fix fstat for fd 0, it currently returns EISDIR (looks like a libuv bug? Or is it a wont-fix? cc @bnoordhuis @cjihrig), the difference relies in how we handle the error. 8c00a80 accidentally fixed this when it ignored the error if the fd was supplied by users. At that commit the S_IFREG was applied on the global stats array that contained the result of last (possibly irrelevant) successful operation.
@joyeecheung

This comment has been minimized.

Copy link
Member

@joyeecheung joyeecheung commented May 16, 2018

Also this is where the root cause is:

> fs.fstatSync(0)
Error: EISDIR: illegal operation on a directory, fstat
    at Object.fs.fstatSync (fs.js:894:3)
> fs.fstatSync(1)
Error: EISDIR: illegal operation on a directory, fstat
    at Object.fs.fstatSync (fs.js:894:3)
> fs.fstatSync(2)
Error: EISDIR: illegal operation on a directory, fstat
    at Object.fs.fstatSync (fs.js:894:3)
@ryzokuken

This comment has been minimized.

Copy link
Member

@ryzokuken ryzokuken commented Oct 27, 2018

@joyeecheung is this fixed? Can this be closed?

@addaleax

This comment has been minimized.

Copy link
Member

@addaleax addaleax commented Oct 27, 2018

This might be related to #20640 (which currently can’t really move forward without some macOS help)?

@ryzokuken

This comment has been minimized.

Copy link
Member

@ryzokuken ryzokuken commented Oct 27, 2018

@addaleax I'll try to help out.

@GiGurra

This comment has been minimized.

Copy link

@GiGurra GiGurra commented Nov 2, 2019

Still an issue with latest nodejs on windows. Unable to read stdin :/. Works fine on linux and windows using git bash, but powershell fails (e.g. cat somefile | myscript).

@addaleax addaleax added the help wanted label Nov 3, 2019
@addaleax

This comment has been minimized.

Copy link
Member

@addaleax addaleax commented Nov 3, 2019

@GiGurra Is cat somefile | myscript what you’re running? What kind of error are you seeing in that case, and how are you reading stdin in your app?

@GiGurra

This comment has been minimized.

Copy link

@GiGurra GiGurra commented Nov 3, 2019

@addaleax I have been looking into this now for a couple of hours and have found what I believe could be the source of my issue.

  • my case works in linux/bash/fish (cat somefile.txt | myscript)
  • my case works in windows cmd (TYPE somefile.txt | myscript)
  • my case does NOT work in powershell (cat somefile.txt | myscript), gives the error listed in this issue

My test script is as simple as it gets :)

#!/usr/bin/env node
console.log("printer running")
console.log(require('fs').readFileSync(process.stdin.fd).toString());

npm link/install on the above and then doing cat somefile.txt | myscript ==> fails with the error mentioned in this github issue.

I believe it has to do with how powershell works together with the startup script that npm install creates. (npm install creates a powershell startup script, cmd startup script and cygwin startup script)

I dont know much/anything about powershell tbh, but it seems like it does some funky stuff to stdin -not just forwarding it to any child processes spawned by the powershell script, and in the process the generated powershell script for any cli application installed with npm install will lose access to stdin.

Conclusion: The powershell startup script that npm install/link creates does not forward stdin properly/at all.

Update: Confirmed. If explicitly tell powershell to use the generated cmd script, it works fine :)

  • powershell cat src.txt | myscript.cmd works
  • powershell cat src.txt | myscript gives the error
  • powershell cat src.txt | myscript.ps1 gives the error
@mklement0

This comment has been minimized.

Copy link
Author

@mklement0 mklement0 commented Nov 3, 2019

Note that the commands in the OP still fail, irrespective of whether you run them from cmd.exe or PowerShell, as of v13.01.

However, the error message for closed stdin / empty input has changed and is now:

fs.js:168
  return (stats[1/* mode */] & S_IFMT) === fileType;
               ^

TypeError: Cannot read property '1' of undefined
...

@GiGurra: I suspect the PowerShell issue you're describing is a separate issue: it may be that the .ps1 file you're running is not designed to accept stdin input. To do so, it would have to check the automatic $Input variable explicitly.

I'm not familiar with the mechanism that auto-generates .ps1 files in the context of npm install (can you provide a link?), but it sounds like that script-generation process should be revisited in order to provide stdin support.

Here's an example that shows that nonempty stdin input is received in a PowerShell Script:

Create test file t.ps1 with the following content:

$Input | node -pe "require('fs').readFileSync(0).toString()"

Then invoke it from PowerShell as follows, and you'll see that the stdin input is correctly received and echoed:

# Provide stdin input and echo it.
PS> 'echo me' | .\t.ps1
echo me

Again, not providing stdin input surfaces the error, albeit with a different error message - whether that points to yet another problem is unclear to me:

# Provide NO stdin input
PS> .\t.ps1
internal/fs/utils.js:220
    throw err;
    ^

Error: EOF: end of file, read
...
@saghul

This comment has been minimized.

Copy link
Member

@saghul saghul commented Nov 5, 2019

Do things work if you use process.stdin.read instead of the fs APIs?

@mklement0

This comment has been minimized.

Copy link
Author

@mklement0 mklement0 commented Nov 7, 2019

@saghul

The following work correctly:

node -e "process.stdin.pipe(process.stdout)"
node -e "process.stdin.on('readable', function() { try { process.stdout.write(process.stdin.read()) } catch {} })"

Without the try / catch, an exception about writing a null to the stream is thrown. Is that expected?

@addaleax

This comment has been minimized.

Copy link
Member

@addaleax addaleax commented Nov 7, 2019

Without the try / catch, an exception about writing a null to the stream is thrown. Is that expected?

It’s intentional that stdin.read() returns null when no more data is available or end-of-stream is reached, and it’s intentional that passing data that is not string or buffer to stdout.write() leads to an exception.

I’m actually personally a bit torn on whether this is the right behaviour, i.e. whether .write(null) shouldn’t maybe work like .end() for consistency, but that goes a bit off-topic here… :)

@saghul

This comment has been minimized.

Copy link
Member

@saghul saghul commented Nov 8, 2019

This looks a bit like a programing error to me. On Windows you can't just write to fd 0 on all shells. You may need to do it to CONOUT$.

@mklement0

This comment has been minimized.

Copy link
Author

@mklement0 mklement0 commented Nov 8, 2019

@saghul: Given that we're talking about standard input, you probably meant CONIN$, but note that:

  • process.stdin.fd is 0 on Windows too (from the docs: "The process.stdin property returns a stream connected to stdin (fd 0)"; try node -pe process.stdin.fd on Windows).
  • non-empty stdin input works just, fine as noted in the OP (echo hi | node -pe "require('fs').readFileSync(0).toString()")

on all shells.

The standard streams are not shell features, they're system features. Shells may choose to surface them differently, though, as PowerShell does via $Input, for instance.

Whatever the calling shell, readFileSync(0) shouldn't fail if there's either no or empty stdin input - it works properly on Unix-like platforms, but it fails on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.