Copying fifos hangs. #727

bruce-one · 2017-05-31T10:38:00Z

shelljs' cp will hang if trying to copy a fifo.

This PR skips copying more non-file-like entities to avoid this issue.

nfischer · 2017-05-31T13:08:04Z

Unix cp also hangs though, so this isn't the bug.

I do see a difference in behavior:

$ cp my_fifo some_file &
$ echo 'hi' > my_fifo # now write to the fifo
$ cat some_file # expected output
hi

# Using ShellJS
$ shx cp my_fifo some_file &
$ echo 'hi' > my_fifo
ShellJS: internal error
Error: ESPIPE: invalid seek, read
    at Error (native)
    at Object.fs.readSync (fs.js:731:19)
    at copyFileSync (/home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/cp.js:67:22)
    at /home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/cp.js:269:7
    at Array.forEach (native)
    at _cp (/home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/cp.js:231:11)
    at /home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/common.js:338:25
    at shx (/home/nate/.npm-global/lib/node_modules/shx/lib/shx.js:116:37)
    at run (/home/nate/.npm-global/lib/node_modules/shx/lib/cli.js:20:23)
    at Object.<anonymous> (/home/nate/.npm-global/lib/node_modules/shx/lib/cli.js:50:3)
$ cat some_file # empty file

Maybe we should just fix the crash during copy? @freitagbr

bruce-one · 2017-05-31T21:05:43Z

Hmm, intriguing.

cp -r also seems to act differently to a straight copy? (I was copying a directory, and it was what lead me to creating this PR)

eg:

mkdir a
mkfifo a/fifo
cp -r a b # no hang
ls b # fifo

So this PR isn't properly replicating cp, no... (Although, afaik, node doesn't support creating fifo's; except I guess if this shells out? :-s (Or uses some native bindings))

execve("/usr/bin/cp", ["cp", "-r", "a", "b"], 0x7ffd9a4e0168 /* 76 vars */) = 0
...
lstat("a/fifo", {st_mode=S_IFIFO|0644, st_size=0, ...}) = 0
mknod("b/fifo", S_IFIFO|0644)           = 0

bruce-one · 2017-05-31T21:26:15Z

When copying the file directly (eg cp fifo newFifo); cp does:

stat("newFifo", 0x7fffdfb9e570)         = -1 ENOENT (No such file or directory)
stat("fifo", {st_mode=S_IFIFO|0644, st_size=0, ...}) = 0
stat("newFifo", 0x7fffdfb9e300)         = -1 ENOENT (No such file or directory)
open("fifo", O_RDONLY)                  = 3
fstat(3, {st_mode=S_IFIFO|0644, st_size=0, ...}) = 0
open("newFifo", O_WRONLY|O_CREAT|O_EXCL, 0644) = 4
fstat(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = -1 ESPIPE (Illegal seek)
mmap(NULL, 139264, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f0cb3a6f000
read(3, "q\n", 131072)                  = 2
write(4, "q\n", 2)                      = 2

vs the following for shelljs:

open("fifo", O_RDONLY|O_CLOEXEC)        = 9
open("newFifo", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 10
pread64(9, 0x26697d0, 65536, 0)         = -1 ESPIPE (Illegal seek)

(The point being read vs pread; wherein pread says "The file referenced by fd must be capable of seeking." 1 which is likely the issue here? :-s (This comes from uv by the look of it? 2)

(I'm just braindumping as I follow some of the rabbit hole :-) )

bruce-one · 2017-05-31T22:49:09Z

Okay, so I think I've fixed the scenario you've referenced :-)

(Setting that offset to -1 (was zero) means that libuv uses read rather than pread :-) )

freitagbr · 2017-06-01T03:17:52Z

@bruce-one is there documentation on setting the offset to -1? I'm not seeing any references to that in the Node.js documentation or source code.

bruce-one · 2017-06-01T03:31:01Z

Ah, no, I just found it in https://github.com/libuv/libuv/blob/v1.x/src/unix/fs.c#L306 (and when looking through the libuv tests -1 is used for the offset quite a bit -- was trying to replicate it in straight uv and stumbled across the difference)

bruce-one · 2017-06-01T03:42:04Z

That switch has been there for a very long time; and it kinda makes sense (I think?) because it's the way to say "no need to worry about offset, so just read not pread":

libuv/libuv@74999f8#diff-6a16903c26af4b4035eda9922a73ecc9R160 (via a few steps in the middle)

bruce-one · 2017-06-02T01:20:50Z

(Sorry for the noise; trying to fix the tests on Windows. It looks like Appvoyer's environment (cygwin?) supports mkfifo, but fs.existsSync fails to recognise that fifo's exist, I'm assuming it's something to do with how cygwin(?) fakes fifo's, whilst node is trying to go direct?)

codecov-io · 2017-06-02T01:56:25Z

Codecov Report

Merging #727 into master will increase coverage by 2.58%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #727      +/-   ##
=========================================
+ Coverage   94.81%   97.4%   +2.58%     
=========================================
  Files          33      33              
  Lines        1254    1231      -23     
=========================================
+ Hits         1189    1199      +10     
+ Misses         65      32      -33

Impacted Files	Coverage Δ
src/cp.js	`91.72% <100%> (+0.6%)`	⬆️
src/common.js	`98.29% <0%> (-0.64%)`	⬇️
src/sort.js	`96.96% <0%> (-0.09%)`	⬇️
src/cd.js	`100% <0%> (ø)`	⬆️
src/grep.js	`100% <0%> (ø)`	⬆️
src/tempdir.js	`100% <0%> (ø)`	⬆️
src/head.js	`100% <0%> (+5.26%)`	⬆️
src/exec.js	`100% <0%> (+24.76%)`	⬆️
src/echo.js	`100% <0%> (+62.5%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15558cf...0d70322. Read the comment docs.

nfischer · 2017-06-02T07:17:08Z

test/cp.js

@@ -797,7 +797,7 @@ test('should not attempt to copy fifos via symlinks in directories', t => {
 test('should copy fifos directly', t => {
  try {
    shell.exec(`mkfifo ${t.context.tmp}/fifo`);
-    shell.exec(`echo -n test1 > ${t.context.tmp}/fifo`, { async: true });
+    shell.exec(`printf test1 > ${t.context.tmp}/fifo`, { async: true });


I'd prefer to see shx echo -n test1 here. Appveyor may have the command, but it may not be consistent with unix printf, and I don't think it's available by default on Windows systems.

Iirc, osx's echo doesn't support the -n... (Which was why I changed it initially)

Also worth noting named pipes aren't supported directly by Windows (only via cygwin etc, which didn't appear reliable)

(Could match the \n though if echo is preferred)

Iirc, osx's echo doesn't support the -n... (Which was why I changed it initially)

shx echo is platform agnostic, so it runs on OS X, Win, and Linux. The behavior is guaranteed to be the same across all machines, which is what you want out of an external dependency.

Oh yeah :-) good point :-)

Is it possible to easily shell out to shelljs's echo implementation? (I've shelled out here because the event loop is blocked by the cp, so need a way to get data into the fifo in the background; and atm this is shelling out to the /bin/sh impl (iirc?) and using it's echo/printf (although maybe there's a better way?)).

nfischer · 2017-06-03T01:17:50Z

@bruce-one are we confident that isFIFO() always returns false for Windows, or is this something we need to investigate? I'm hesitant to add features that go untested on Windows--if there's any chance we hit those code paths in production, then we have unpredictable behavior. If we can guarantee that we don't hit that code for that platform, I feel a bit more comfortable.

bruce-one · 2017-06-03T12:58:04Z

From what I know/can ascertain (some but not extensive research), Windows can't put named pipes on a filesystem? (a la mkfifo x; ls # x) Hence I don't know of a situation where isFIFO could return true on Windows.

Hence support for this in native windows isn't possible? (Cygwin emulates pipes, but unless node is built for Cygwin, I don't think node will report them - from my testing.)

I'm no Windows expert though... :-s

bruce-one · 2017-06-03T13:00:57Z

Another outstanding question with this is in the recursive type scenario: should we call mkfifo or something to create the fifo to better replicate cp? (afaict, node can't do that natively)

To illustrate:

mkdir a
mkfifo a/fifo
cp -r a b
ls b # fifo ; with shx this fifo wouldn't exist as it stands in this pr

nfischer · 2017-06-18T00:16:05Z

Another outstanding question with this is in the recursive type scenario: should we call mkfifo or something to create the fifo to better replicate cp? (afaict, node can't do that natively)

Seems like the answer would be yes. Our goal here is compatibility with POSIX behavior.

Shells out to `mknod`, so less platform independent than normal, although these devices are not platform independent.

bruce-one · 2017-06-19T21:51:45Z

Have added support for acting like -R has been specified (when recursive is true).

From some reading (eg https://unix.stackexchange.com/questions/18712/difference-between-cp-r-and-cp-r-copy-command) -r (by contrast to -R) is not well defined in this context, however based on the existing code and the GNU implementation I've just made -r act like -R. I don't know whether adding a --copy-contents flag becomes useful now? (It's not posix... From skimming the posix definition there's no equivalent of that GNU flag?)

Unfortunately creating character and block devices requires root permissions... I didn't know how to handle that. (Running ava with sudo and a grep for root (ish) after running it as non-root seemed like a tempting solution? But I wasn't sure what was preferred.)

So for now those two tests just don't run on CI :-s (And I haven't even manually tested them on OSX...)

nfischer · 2017-06-20T06:48:47Z

@bruce-one thanks for following up on this

Unfortunately creating character and block devices requires root permissions...

I would like to steer clear of root-only features in ShellJS core.

After thinking a bit, your initial suggestion may have been right after all: skip these file types by default. We can indicate an error occurred via common.error() and copy all the other files.

Since this is a great PR though, I think this would be a good candidate for a plugin. We're adding a plugin API for v0.8, allowing users to extend ShellJS. We could create a plugin to add back support for fifo and other specialized file types, overriding the default behavior.

shell.cp('blockDeviceName', 'dest'); // default behavior
// cp: block device is not supported: 'blockDeviceName'

require('shelljs-plugin-cp-special-files');
shell.cp('blockDeviceName', 'dest');
// Now it succeeds! (assuming we're on Unix and have root permission)

ShellJS proper won't need to have any more platform-dependent code, and we can still provide support for the rare users who need it. This sounds like a win. What do you think?

nfischer · 2017-06-20T06:56:06Z

src/cp.js

+    args.push(major(stat.rdev), minor(stat.rdev));
+  }
+  try {
+    var cmd = exec(args.join(' '), { silent: true });


For security, I recommend child_process.execFileSync() here. Just wrap it in a try-catch, since it throws an exception if the command fails.

try { child_process.execFileSync('mknod', argsArray); } catch (e) { failureMessage = e.stderr; }

nfischer · 2017-06-20T06:59:50Z

src/cp.js

+  var created;
+  var failureMessage;
+  var args = ['mknod'];
+  args.push('"' + destFile + '"');


At first glance this looks wrong. What if destFile is hello world"; rm -rf *; echo "? Looks like you'll be running:

$ mknod "hello world"; rm -rf *; echo "" p

JSON.stringify() is the way we usually perform quote escaping, but you can avoid this entirely if you take my suggestion below.

nfischer · 2017-06-20T07:01:22Z

src/cp.js

@@ -153,6 +171,42 @@ function cpdirSyncRecursive(sourceDir, destDir, currentDepth, opts) {
  } // for files
 } // cpdirSyncRecursive

+function major(rdev) {


An explanatory comment might be good, since this is pretty niche and not very obvious

bruce-one · 2017-06-20T23:30:03Z

I've done that cleanup based on the feedback from your review (thanks! :-) ).

I agree that making this a plugin does seem reasonable :-)

Hence, should this PR be changed to just log the error message and then independently create a plugin package?

Although, it might be nice to mention the plugin in the error/warning message? Hence maybe make the plugin, then change PR, then merge? Or are you not that keen on "centralising" plugins quite that much? (e.g. if it were called out explicitly (a la cp: block device is not supported: 'blockDeviceName' and at the end of the cp run Special files were encountered during this operation. Add the cp-special-files plugin if support for these is required.) then it might be nice to have it as shelljs/plugin-cp-special-files? Absolutely no issue if that's not how you want to handle it though :-)

One of the reasons I ask that is because a "shelljs/plugin-posix" could be useful (from past experience) because I know I personally use shelljs solely on posix-like OSs, and find it's consistency excellent (and still very useful), but being able to do "all the posix things" in one plugin could be nice?

nfischer · 2017-06-21T06:16:28Z

One of the reasons I ask that is because a "shelljs/plugin-posix" could be useful (from past experience) because I know I personally use shelljs solely on posix-like OSs, and find it's consistency excellent (and still very useful), but being able to do "all the posix things" in one plugin could be nice?

Sounds like a good idea. Let's discuss the plugin over at #748. We'll leave this issue for only the cp-specific changes.

This is flushed at the end of the copy command, or on exit if that doesn't happen first.

nfischer · 2017-06-22T01:56:31Z

src/cp.js

+    if (srcFileStat.isCharacterDevice()) type = 'block device';
+    if (srcFileStat.isBlockDevice()) type = 'character device';
+    common.error('copyFileSync: ' + type + ' is not supported (' + srcFile + ')', { continue: true });
+    common.logLater('copyFileSync: Special files were encountered during this operation. Please investigate shelljs plugins if you would like to add support for these.');


I think the common.error() log message is sufficient. Let's get rid of logLater and move something like this into the docs. You can edit the //@ section in this file, and then run npm run gendocs.

nfischer · 2017-06-22T01:59:22Z

test/cp.js

@@ -757,7 +757,7 @@ test('should not overwrite recently created files (not give error no-force mode)
  t.is(shell.cat(`${t.context.tmp}/file1`).toString(), 'test1');
 });

-test('should copy fifos contents', t => {
+test('should copy fifo contents', t => {


I think this case should also produce a warning. We'll completely drop fifo support and add it back in the plugin.

This fits in the "act like cp" sense.

Being, without the recursive flag cp will treat these files as just "dumb files" rather than special files.

From what I'd read, that's the context of where this came from in posix's cp; i.e. originally it would just treat everything as a file and do a dumb copy (creating a normal-file rather than a special-file), but then later on support was added for being clever with special-files. Hence, my brain goes "native shelljs can treat them like normal-files" and "plugin'd shelljs gains the knowledge to treat them as special-files" parallels "normal cp is dumb" and "modern cp -R is smart". If that helps frame why I'd left this in?

There's a conflict between "act like cp" and "pull all the unix out" :-s
I was inclined to "act like cp" (and treat them like normal files, and hence hang while copying) because it's simple and native? Although that could just be me :-)

There's a conflict between "act like cp" and "pull all the unix out" :-s

I agree these goals are in conflict. I think in this case we should try to stay internally consistent. We should output a warning when copying any fifos and delegate to the plugin to provide actual posix behavior.

If the plugin aims for posix compatibility, what arguments does it need passed? It sounds like it needs options.recursive, srcfile, & destFile, is that correct?

Yep, sounds right :-) (Assuming the srcFile symlink has already been resolved if options.followsymlink === true.) Perhaps it's worth passing the whole options object though? (Thinking in terms of flexibility, eg if more flags are added (eg -p))

(And I agree with your perspective on this: the whole reason I opened this PR in the first place was because it tripped me up, and staying internally consistent would have saved me whereas "acting like cp" wouldn't. Just hadn't quite come to that realisation yet :-p )

nfischer · 2017-06-22T02:03:10Z

test/cp.js

  }
 });

-test('should copy fifos contents via symlinks', t => {
+test('should copy fifo contents via symlinks', t => {


For consistency, should this also be a warning?

This reverts commit 6a4a6a0.

bruce-one · 2017-06-22T02:27:05Z

(btw, more than happy to just log and not copy when in non-recursive mode, was just explaining why I'd originally left that aspect alone because it feels like sound logic (hence why I'd left it :-p ), but I'm more than happy make the change :-) )

bruce-one added 2 commits May 31, 2017 19:57

Don't attempt to copy fifos.

e17d624

Avoid fifos via symlinks.

52c412f

nfischer assigned freitagbr May 31, 2017

Better replicate cp during direct fifo copies.

e4a911d

Bugfix: don't break offset during copies.

eaaa40a

bruce-one added 4 commits June 2, 2017 07:29

Test fix, use printf for osx not echo.

476d4b4

Check exec exit code to fix tests on Windows.

c71edc2

Skip fifo tests on Windows.

d469212

Typo.

5689dc0

nfischer reviewed Jun 2, 2017

View reviewed changes

bruce-one added 7 commits June 19, 2017 16:06

Shell out to create fifos during recursive copy.

777fe04

Handle "recursive" flag for special non-files.

8fa217b

Shells out to `mknod`, so less platform independent than normal, although these devices are not platform independent.

Cleanup.

18c513e

Error messages.

be93375

mknod fails without root.

599a2c5

Linting.

2d3af53

Permissions on special files.

d942f3f

bruce-one added 3 commits June 20, 2017 07:40

Logging.

fad43aa

Lint.

e6760bb

chown and chmod special files.

045c290

nfischer reviewed Jun 20, 2017

View reviewed changes

bruce-one added 2 commits June 21, 2017 09:00

Use execFileSync for safety and simplicity.

40518f7

Use mkfifo for fifo creation due for OSX compat.

c69ddc3

nfischer mentioned this pull request Jun 21, 2017

New plugin for unix-only features and extensions #748

Open

bruce-one added 2 commits June 22, 2017 07:48

Replace unix specific code with warning.

b2932cf

Log that special files were skipped.

6a4a6a0

This is flushed at the end of the copy command, or on exit if that doesn't happen first.

nfischer reviewed Jun 22, 2017

View reviewed changes

bruce-one added 2 commits June 22, 2017 12:05

Revert "Log that special files were skipped."

f7957bf

This reverts commit 6a4a6a0.

Doc.

a4e5411

bruce-one added 3 commits June 22, 2017 13:04

Treat special files the same whether recursive or not.

117894c

Doc.

48a0198

Build doc.

0d70322

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copying fifos hangs. #727

Copying fifos hangs. #727

bruce-one commented May 31, 2017 •

edited

nfischer commented May 31, 2017

bruce-one commented May 31, 2017

bruce-one commented May 31, 2017

bruce-one commented May 31, 2017

freitagbr commented Jun 1, 2017 •

edited

bruce-one commented Jun 1, 2017

bruce-one commented Jun 1, 2017

bruce-one commented Jun 2, 2017

codecov-io commented Jun 2, 2017 •

edited

nfischer Jun 2, 2017

bruce-one Jun 2, 2017 •

edited

nfischer Jun 3, 2017

bruce-one Jun 3, 2017

nfischer commented Jun 3, 2017

bruce-one commented Jun 3, 2017 •

edited

bruce-one commented Jun 3, 2017

nfischer commented Jun 18, 2017

bruce-one commented Jun 19, 2017

nfischer commented Jun 20, 2017

nfischer Jun 20, 2017

nfischer Jun 20, 2017

nfischer Jun 20, 2017

bruce-one commented Jun 20, 2017

nfischer commented Jun 21, 2017

nfischer Jun 22, 2017

nfischer Jun 22, 2017

bruce-one Jun 22, 2017

nfischer Jun 22, 2017

bruce-one Jun 22, 2017

nfischer Jun 22, 2017

bruce-one commented Jun 22, 2017 •

edited

Copying fifos hangs. #727

Are you sure you want to change the base?

Copying fifos hangs. #727

Conversation

bruce-one commented May 31, 2017 • edited

nfischer commented May 31, 2017

bruce-one commented May 31, 2017

bruce-one commented May 31, 2017

bruce-one commented May 31, 2017

freitagbr commented Jun 1, 2017 • edited

bruce-one commented Jun 1, 2017

bruce-one commented Jun 1, 2017

bruce-one commented Jun 2, 2017

codecov-io commented Jun 2, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

bruce-one Jun 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfischer commented Jun 3, 2017

bruce-one commented Jun 3, 2017 • edited

bruce-one commented Jun 3, 2017

nfischer commented Jun 18, 2017

bruce-one commented Jun 19, 2017

nfischer commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruce-one commented Jun 20, 2017

nfischer commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruce-one commented Jun 22, 2017 • edited

bruce-one commented May 31, 2017 •

edited

freitagbr commented Jun 1, 2017 •

edited

codecov-io commented Jun 2, 2017 •

edited

bruce-one Jun 2, 2017 •

edited

bruce-one commented Jun 3, 2017 •

edited

bruce-one commented Jun 22, 2017 •

edited