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

ebraminio · 2017-05-14T23:21:48Z

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

Affected core subsystem(s)

fs

Currently

echo "" | node -e "require('fs').realpath('/dev/stdin', console.log)"

and

echo "" | node -e "console.log(require('fs').realpathSync('/dev/stdin'))"

are broken due to our logic of realpath and this patch changes node in a way to match python:

echo "" | python -c "import os; print(os.path.realpath('/dev/stdin'))"

mscdex · 2017-05-15T03:55:39Z

test/parallel/test-fs-realpath-pipe.js

+'use strict';
+
+// Skip Windows as /dev/stdin is not available
+if (process.platform === 'win32')


Use common.isWindows and common.skip() here instead. See here for an example.

mscdex · 2017-05-15T03:59:11Z

test/parallel/test-fs-realpath-pipe.js

+  });`,
+  `console.log(require('fs').realpathSync('/dev/stdin'));`
+]) {
+  const child = spawn(process.execPath, ['-e', code], { stdio: 'pipe' });


Consider using spawnSync() instead to simplify this a bit and to make future debugging a bit easier.

ebraminio · 2017-05-15T06:40:38Z

Both done.

mscdex · 2017-05-15T07:35:54Z

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

ebraminio · 2017-05-16T12:50:05Z

This probably needs more study, both of just not following (this patch) and following to the end and raising [current] or not raising error on following symlinks to pipes/sockets should be discussed, lets close it for now.

ebraminio · 2017-05-20T16:30:28Z

Updated to follow python result.

ebraminio · 2017-05-20T16:53:56Z

@refack: Hi. Thanks for reviewing my patches. :) Does this need a documentation change also?

ebraminio · 2017-05-20T17:22:15Z

Have a look at #13130 (comment) also

refack

Need documentation.

refack · 2017-05-20T19:02:32Z

test/parallel/test-fs-realpath-pipe.js

+    process.exit(1);
+  };`
+]) {
+  assert.strictEqual(spawnSync(process.execPath, ['-e', code], {


In adition to sanity, I would consider asserting the value of resolvedPath and the returned value from require('fs').realpathSync('/dev/stdin');
Either in the script literal or console.loging it and adding another assert for stdout.

Hmm, that resolves to different things on different runs and platforms, that is hard to check I think

Than check it has a truthy value if (!resolvedPath) process.exit(2)

refack · 2017-05-20T19:15:31Z

@ebraminio Yes, I would add a note to fs.md#fsrealpathpath-options-callback and realPathSync since this is a deviation from realpath(3).

Something like

*Note*: If `path` resolves to a Socket or a Pipe, the function will return the value of `require('path').resolve(path)`.

P.S. this might be a breaking change (a.k.a semver-major), so I've run CITGM and we'll need to compare
Baseline: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/806/testReport/
~~With patch: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/802/testReport/~~
With patch after rebase: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/805/testReport/
With patch after rebase #2: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/808/

refack · 2017-05-20T19:18:38Z

since this is a deviation from realpath(3).

Or is it converted by the notes?

ebraminio · 2017-05-20T20:48:13Z

doc is done now, have a look

refack · 2017-05-20T21:37:34Z

doc/api/fs.md

+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/13028
+    description: Pipe/Socket resolve support was added.
+  - version: REPLACEME


Rebase on master this comment was reverted.

refack · 2017-05-20T21:42:13Z

doc/api/fs.md

@@ -1872,10 +1879,16 @@ object with an `encoding` property specifying the character encoding to use for
 the path passed to the callback. If the `encoding` is set to `'buffer'`,
 the path returned will be passed as a `Buffer` object.

+*Note*: If `path` resolves to a Socket or a Pipe, the function will return the


BTW is my note actually correct? In that case it's easy to check in the test.

No I've copy-pasted your comment just in rush, removed the both as I couldn't reach to anything useful for here.

Show me some input -> output results. I'll phrase something better.

I am not sure what it should be called, before my patch echo "" | node -e "require('fs').realpath('/dev/stdin', console.log)" was raising error but with my patch now it has a similar result with python, say /proc/12938/fd/pipe:[224961], just on Linux

*Note*: If `path` resolves to a socket or a pipe, the function will return a system dependent name for that object.

refack · 2017-05-20T22:18:07Z

CI: https://ci.nodejs.org/job/node-test-commit/10050/

refack · 2017-05-21T02:57:11Z

So there are a linting error:

not ok 26 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-fs-realpath-pipe.js
message: '''expected'' is assigned a value but never used.'
  severity: error
  data:
    line: 12
    column: 7
    ruleId: no-unused-vars

and a fail on AIX:

assert.js:92
  throw new AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 1 === 2
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-fs-realpath-pipe.js:33:10)

If you don't have a clear idea why this fails on AIX, and since this is not a regression, we could add it to /known_issues/

ebraminio · 2017-05-21T05:40:31Z

Done. I don't have access to AIX but perhaps it doesn't have the concept of /dev/stdin or that somehow is different there.

richardlau · 2017-05-21T06:10:10Z

cc @nodejs/platform-aix

mhdawson · 2017-05-23T15:45:51Z

AIX does not have /dev/stdin. See the second comment in https://unix.stackexchange.com/questions/338667/unix-systems-without-dev-stdin-dev-stdout-and-dev-stderr. I also logged into the community AIX machines and there is no /dev/stdin:

# ls /dev/stdin
ls: 0653-341 The file /dev/stdin does not exist.

So although for redirection behaviour seems to be similar, the files do not actually exist in /dev.

Given that the test depends on /dev/stdin, I'd suggest it just be skipped as it is for windows.

refack · 2017-05-23T15:47:26Z

AIX does not have /dev/stdin.

@mhdawson Thanks!

Given that the test depends on /dev/stdin, I'd suggest it just be skipped as it is for windows.

I'm +1

ebraminio · 2017-05-23T20:36:41Z

Given that the test depends on /dev/stdin, I'd suggest it just be skipped as it is for windows.

Done.

refack · 2017-05-25T17:26:31Z

Pre land CI: https://ci.nodejs.org/job/node-test-commit/10154/

refack · 2017-05-25T22:15:47Z

arm-fanned fail is infrastructure. Landing.

PR-URL: nodejs#13028 Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-05-25T22:21:42Z

Landed in b3d1e3d

PR-URL: #13028 Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins · 2017-07-17T16:47:51Z

The docs here claim that it is adding a new api... should this have been semver minor?
Backport to v6.x?

MylesBorins · 2017-08-14T18:19:05Z

ping

MylesBorins · 2017-09-19T15:53:16Z

ping @refack

refack · 2017-09-19T16:05:59Z

AFAICT this is semver-patch. It fixes a case that would throw

$ echo "" | node -e "console.log(require('fs').realpathSync('/dev/stdin'))"

Error: ENOENT: no such file or directory, lstat '/proc/180/fd/pipe:[140]'
    at Object.fs.lstatSync (fs.js:961:11)
    at Object.realpathSync (fs.js:1629:21)

so now it returns a sane value:

$ echo "" | node -e "console.log(require('fs').realpathSync('/dev/stdin'))"
/proc/577/fd/pipe:[565]

Documentation comment could have been "Pipe/Socket resolve support ~~was added~~ fixed."

MylesBorins · 2017-10-16T21:15:30Z

@refack this doesn't appear broken on v6.x right now

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label May 14, 2017

ebraminio force-pushed the unnamedpiperealpath branch from 4104b3d to 84cbd7a Compare May 14, 2017 23:22

mscdex reviewed May 15, 2017

View reviewed changes

ebraminio force-pushed the unnamedpiperealpath branch from 84cbd7a to 1fa2c30 Compare May 15, 2017 06:39

ebraminio force-pushed the unnamedpiperealpath branch from 1fa2c30 to dbac492 Compare May 15, 2017 06:58

ebraminio closed this May 16, 2017

ebraminio reopened this May 20, 2017

ebraminio force-pushed the unnamedpiperealpath branch from dbac492 to 00f1de7 Compare May 20, 2017 16:25

ebraminio force-pushed the unnamedpiperealpath branch 2 times, most recently from c630b2b to f39045a Compare May 20, 2017 16:50

refack suggested changes May 20, 2017

View reviewed changes

refack self-assigned this May 20, 2017

ebraminio force-pushed the unnamedpiperealpath branch 3 times, most recently from e65f679 to 035b935 Compare May 20, 2017 20:43

refack reviewed May 20, 2017

View reviewed changes

ebraminio force-pushed the unnamedpiperealpath branch from 035b935 to f0146f4 Compare May 20, 2017 21:41

refack reviewed May 20, 2017

View reviewed changes

ebraminio force-pushed the unnamedpiperealpath branch 2 times, most recently from e8ed5ba to eef07fc Compare May 20, 2017 21:56

refack approved these changes May 20, 2017

View reviewed changes

ebraminio force-pushed the unnamedpiperealpath branch from e1ae7c5 to 9746ad1 Compare May 21, 2017 05:39

ebraminio mentioned this pull request May 22, 2017

cmd: support dash as stdin alias #13012

Closed

4 tasks

ebraminio force-pushed the unnamedpiperealpath branch from 9746ad1 to d4d9120 Compare May 23, 2017 20:35

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

363020c

ebraminio force-pushed the unnamedpiperealpath branch from d4d9120 to 363020c Compare May 24, 2017 14:43

refack pushed a commit to refack/node that referenced this pull request May 25, 2017

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

b3d1e3d

PR-URL: nodejs#13028 Reviewed-By: Refael Ackermann <refack@gmail.com>

refack closed this May 25, 2017

jasnell pushed a commit that referenced this pull request May 28, 2017

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

d99b7bc

PR-URL: #13028 Reviewed-By: Refael Ackermann <refack@gmail.com>

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

refack removed their assignment Oct 12, 2018

mgabeler-lee-6rs mentioned this pull request Jul 22, 2024

require() no longer follows symlinks after calling statSync() on a FIFO or socket file #51142

Open

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

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

Conversation

ebraminio commented May 14, 2017 • edited Loading

Affected core subsystem(s)

mscdex May 15, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex May 15, 2017 • edited Loading

Choose a reason for hiding this comment

ebraminio commented May 15, 2017

mscdex commented May 15, 2017

ebraminio commented May 16, 2017

ebraminio commented May 20, 2017 • edited Loading

ebraminio commented May 20, 2017 • edited Loading

ebraminio commented May 20, 2017

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 • 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 commented May 20, 2017 • edited Loading

refack commented May 20, 2017 • edited Loading

ebraminio commented May 20, 2017

refack May 20, 2017

Choose a reason for hiding this comment

refack 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 • edited Loading

Choose a reason for hiding this comment

refack May 20, 2017 • edited Loading

Choose a reason for hiding this comment

ebraminio May 20, 2017

Choose a reason for hiding this comment

refack commented May 20, 2017

refack commented May 21, 2017

ebraminio commented May 21, 2017 • edited Loading

richardlau commented May 21, 2017

mhdawson commented May 23, 2017

refack commented May 23, 2017

ebraminio commented May 23, 2017

refack commented May 25, 2017

refack commented May 25, 2017

refack commented May 25, 2017

MylesBorins commented Jul 17, 2017

MylesBorins commented Aug 14, 2017

MylesBorins commented Sep 19, 2017

refack commented Sep 19, 2017

MylesBorins commented Oct 16, 2017

ebraminio commented May 14, 2017 •

edited

Loading

mscdex May 15, 2017 •

edited

Loading

mscdex May 15, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading

ebraminio commented May 20, 2017 •

edited

Loading

ebraminio May 20, 2017 •

edited

Loading

refack commented May 20, 2017 •

edited

Loading

refack commented May 20, 2017 •

edited

Loading

ebraminio May 20, 2017 •

edited

Loading

ebraminio May 20, 2017 •

edited

Loading

refack May 20, 2017 •

edited

Loading

ebraminio commented May 21, 2017 •

edited

Loading