cli: accept /dev/stdin as input file on Linux #13130

ebraminio · 2017-05-20T15:02:08Z

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

Affected core subsystem(s)

cli

As mentioned above, tests will be added but for now, test it on Linux (macOS is OK) like this which with current version is broken:

make && echo "console.log(process.argv[2])" | ./node /dev/stdin --option-for-stdin-script

refack

suggestion

refack · 2017-05-20T16:30:56Z

src/node_file.cc

@@ -559,6 +560,9 @@ static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
  if (rc == 0) {
    const uv_stat_t* const s = static_cast<const uv_stat_t*>(req.ptr);
    rc = !!(s->st_mode & S_IFDIR);
+    if (rc == 0) {


Since this is a non hot-spot, I'd rather see an explicit if and rc = 2, similar to

if (rc == 0 && (s->st_mode & (S_IFIFO | S_IFSOCK))) { rc = 2; }

refack · 2017-05-20T16:56:06Z

Added blocked until #13012 land, and the test case is added.

A question just for my information (since I'm not a posix expert): In the "simple" case node /dev/stdin it's a file, and in echo "console.log(process.argv[2])" | ./node /dev/stdin --option-for-stdin-script, /dev/stdin is a pipe?

ebraminio · 2017-05-20T17:08:00Z

When a process is piped, /dev/stdin will be a pipe (FIFO) normally.

echo "" | node -e "console.log(require('fs').statSync('/dev/stdin').isFIFO())"
(returns true)

But it seems in require('child_process').spawnSync(process.execPath, ['-e', code], { stdio: 'pipe' }) case, it can be a socket also that's why I am checking both at the same time.

node -e "console.log(require('child_process').spawnSync(process.execPath, ['-e', 'console.log(require(\'fs\').statSync(\'/dev/stdin\').isFIFO())'], { stdio: 'pipe' }).stdout.toString())"

(checks if /dev/stdin inside script piped with spawnSync is a pipe, which is false)

node -e "console.log(require('child_process').spawnSync(process.execPath, ['-e', 'console.log(require(\'fs\').statSync(\'/dev/stdin\').isSocket())'], { stdio: 'pipe' }).stdout.toString())"

(is a socket, true)

refack · 2017-05-20T17:29:02Z

@ebraminio Thanks!

Follow Up: So what is s->st_mode on macOS (and maybe freeBSD), where you said curl pad.js.org | node /dev/stdin -h works?

ebraminio · 2017-05-20T17:43:38Z

macOS probably has same stat but its difference and advantage is on the fact that its /dev/stdin is just symlinked to /dev/fd/0 and there is nothing like /proc/self/fd/pipe:[289367] that can followed from there which can not be opened on nodejs in order to execute.

Fishrock123 · 2017-05-20T19:11:03Z

Will this make require('/dev/stdin') work? Should it?

Is that useful or potentially risky?

refack · 2017-05-20T19:26:25Z

Will this make require('/dev/stdin') work? Should it?

Is that useful or potentially risky?

It's as bad as eval(userinput), or exec('node' ['-e', userinput]), or writeFile('./tmp.js', userInput); require('./tmp.js');
People who don't sanitize their user input are asking for trouble. Ref: https://xkcd.com/327/

ebraminio · 2017-05-20T19:51:41Z

Will this make require('/dev/stdin') work? Should it?

It is already possible on macOS and possibly [not tested] other BSD derivations anyway.

bnoordhuis · 2017-05-20T19:48:34Z

lib/module.js

@@ -195,6 +195,8 @@ Module._findPath = function(request, paths, isMain) {
        if (exts === undefined)
          exts = Object.keys(Module._extensions);
        filename = tryPackage(basePath, exts, isMain);
+      } else if (rc === 2) { // Pipe or Socket


Nit: two spaces before the comment.

Removed on new version as suitable comment on that version was something like neither file or directory which can be implied from the above.

bnoordhuis · 2017-05-20T19:55:06Z

src/node_file.cc

@@ -559,6 +560,9 @@ static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
  if (rc == 0) {
    const uv_stat_t* const s = static_cast<const uv_stat_t*>(req.ptr);
    rc = !!(s->st_mode & S_IFDIR);
+    if (rc == 0 && (s->st_mode & (S_IFIFO | S_IFSOCK))) {


I am not 100% convinced this is correct. Is there a reason you left out S_IFCHR?

My gut instinct is that the logic should look something like this:

if (S_ISREG(s->st_mode)) rc = 0; else if (S_ISDIR(s->st_mode)) rc = 1; else rc = 2;

(Whether that compiles on Windows is left as an exercise to the reader.)

That macro is not available but S_IFREG can be used it seems, done.

refack · 2017-05-20T20:28:11Z

src/node_file.cc

@@ -546,7 +546,8 @@ static void InternalModuleReadFile(const FunctionCallbackInfo<Value>& args) {
 }

 // Used to speed up module loading.  Returns 0 if the path refers to
-// a file, 1 when it's a directory or < 0 on error (usually -ENOENT.)
+// a file, 1 when it's a directory, 2 when is pipe or socket, or < 0 on


Need to update the comment: "2 when it's anything else"

refack

1 nit

bnoordhuis

LGTM. A regression test would be nice, though.

ebraminio · 2017-05-20T21:01:11Z

As this is unix only unlike the other patch, a separate test is added and this no longer blocked on that.

refack · 2017-05-20T21:14:55Z

Unblocked, but rename the test to start with the subsystem you are testing, something like: test-cli-arg-devstdin-posix.js, and update the first comment.

refack · 2017-05-20T21:17:30Z

CI: ~~https://ci.nodejs.org/job/node-test-commit/10048/~~ https://ci.nodejs.org/job/node-test-commit/10049/

ebraminio · 2017-05-20T21:52:59Z

Hmmm, the logic seems not OK, hopefully temporary closing

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. labels May 20, 2017

ebraminio force-pushed the pipescript branch from a595bdc to 724e675 Compare May 20, 2017 15:05

refack approved these changes May 20, 2017

View reviewed changes

ebraminio force-pushed the pipescript branch from 724e675 to 723b418 Compare May 20, 2017 16:44

refack added the blocked PRs that are blocked by other issues or PRs. label May 20, 2017

ebraminio mentioned this pull request May 20, 2017

fs: fix realpath{Sync} on resolving pipes/sockets #13028

Closed

4 tasks

refack self-assigned this May 20, 2017

bnoordhuis requested changes May 20, 2017

View reviewed changes

ebraminio force-pushed the pipescript branch from 723b418 to e52f707 Compare May 20, 2017 20:24

refack reviewed May 20, 2017

View reviewed changes

refack approved these changes May 20, 2017

View reviewed changes

ebraminio force-pushed the pipescript branch from e52f707 to 75ed439 Compare May 20, 2017 20:39

bnoordhuis approved these changes May 20, 2017

View reviewed changes

ebraminio force-pushed the pipescript branch from 75ed439 to b0db367 Compare May 20, 2017 21:00

refack removed the blocked PRs that are blocked by other issues or PRs. label May 20, 2017

cli: accept /dev/stdin as input file on Linux

e0a4a1e

ebraminio force-pushed the pipescript branch from b0db367 to e0a4a1e Compare May 20, 2017 21:23

ebraminio closed this May 20, 2017

refack removed their assignment Oct 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: accept /dev/stdin as input file on Linux #13130

cli: accept /dev/stdin as input file on Linux #13130

ebraminio commented May 20, 2017 •

edited

Loading

refack left a comment

refack May 20, 2017

ebraminio May 20, 2017

refack commented May 20, 2017

ebraminio commented May 20, 2017 •

edited

Loading

refack commented May 20, 2017

ebraminio commented May 20, 2017 •

edited

Loading

Fishrock123 commented May 20, 2017

refack commented May 20, 2017

ebraminio commented May 20, 2017 •

edited

Loading

bnoordhuis May 20, 2017

ebraminio May 20, 2017

bnoordhuis May 20, 2017

ebraminio May 20, 2017 •

edited

Loading

refack May 20, 2017

ebraminio May 20, 2017

refack left a comment

bnoordhuis left a comment

ebraminio commented May 20, 2017 •

edited

Loading

refack commented May 20, 2017

refack commented May 20, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading

cli: accept /dev/stdin as input file on Linux #13130

cli: accept /dev/stdin as input file on Linux #13130

Conversation

ebraminio commented May 20, 2017 • edited Loading

Affected core subsystem(s)

refack left a comment

Choose a reason for hiding this comment

refack May 20, 2017

Choose a reason for hiding this comment

ebraminio May 20, 2017

Choose a reason for hiding this comment

refack commented May 20, 2017

ebraminio commented May 20, 2017 • edited Loading

refack commented May 20, 2017

ebraminio commented May 20, 2017 • edited Loading

Fishrock123 commented May 20, 2017

refack commented May 20, 2017

ebraminio commented May 20, 2017 • edited Loading

bnoordhuis May 20, 2017

Choose a reason for hiding this comment

ebraminio May 20, 2017

Choose a reason for hiding this comment

bnoordhuis May 20, 2017

Choose a reason for hiding this comment

ebraminio May 20, 2017 • edited Loading

Choose a reason for hiding this comment

refack May 20, 2017

Choose a reason for hiding this comment

ebraminio May 20, 2017

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

ebraminio commented May 20, 2017 • edited Loading

refack commented May 20, 2017

refack commented May 20, 2017 • edited Loading

ebraminio commented May 20, 2017 • edited Loading

ebraminio commented May 20, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading

ebraminio May 20, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading

refack commented May 20, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading