shelljs auto generate some files?

[cage1618]

Node version (or tell us if you're using electron or some other framework):6.9.1

ShellJS version (the most recent version/Github branch you see the bug on):0.7.0

Operating system:centos x86_64

Description of the bug:auto generate three file like shelljs_*

Example ShellJS command to reproduce the error: no

const shell = requiire('shelljs');
shell.exec('node app.js');

shelljs auto generate 3 fiels:

How to disable this? Thanks

[nfischer]

This is an unfortunate aspect of how shell.exec works. Here are some workarounds:

• Use child_process.execSync instead (this won't do console output for you)
• Use shell.exec(..., { async: true }) (won't create temp files, but it's async)
• Set os.tmpdir (or process.env.TMPDIR) to point to a directory where you can put these files

exec actually should be cleaning up those files for you after it finishes, so something may be wrong in our code if it's missing those files.

[nfischer]

Nothing we can do to fix this, so I'm closing this

[BurtHarris]

@nfischer I had the same question about the temp files, found this bug.

Could you explain what you mean when you say that child_process.execSync "won't do console output for you."

[nfischer]

shell.exec() will run your external command. If that command has any stdout or stderr output, then that will be forwarded to the console. child_process.execSync will not print that output to the console for you.

> shell.exec('echo hello world');
hello world
> child_process.execSync('echo hello world'); // silent

[BurtHarris]

But this works to get the same effect:

> const child_process = require('child_process')
> child_process.execSync('echo hello world', {stdio:"inherit"})
hello world

You can also control the three standard file descriptors individually. Does this allow a simpler implementation that won't need to generate temp files?

[nfischer]

But this works to get the same effect:

Yup

You can also control the three standard file descriptors individually. Does this allow a simpler implementation that won't need to generate temp files?

We tried this in the past. I think you'll find that you see output like this:

> var result = child_process.execSync('echo hello world', { stdio: 'inherit' }); // prints to console
hello world
> console.log(result); // result is null, not a Buffer
null
> console.log(result === null);
true

The only way I know to print output to the console and to also return it from a sync function is the way we've implemented. Please correct me if I'm wrong--we've recently dropped support for anything below v4, so we might have more options available now.

[BurtHarris]

So the goal is to get it both on console/screen and capture it in a Node.js Buffer. Is that right?

[nfischer]

That's the current behavior of shell.exec(), so backwards compatibility would be the motivator. If child_process.execSync() works well enough for you, by all means feel free to use it 😄

[BurtHarris]

Thanks Nate. I've been in the dev game for over 40 years now, but I'm still rather new to Node.js. I'm sort of feeling my way through issues like how to best exec right now. I'll let you know if I find a solution that matches your requirements, but I'm all set for now myself.

[freitagbr]

@nfischer I left a comment on another issue (#724 (comment)) regarding this very problem.

In short, we don't need to create temp files and fork node in shell.exec() in order to output to stdout and capture the output. What we can do instead is use child_process.execSync(..., { stdio: 'inherit' }) to output to the console. The stdout and stderr from the spawned process will be inherited by the current process, so shell output works correctly. We can also intercept stdout and stderr, similar to how it is done in the tests for echo, to return the actual value of stdout and stderr from the call.

I think this method would be much more reliable and secure than writing temp files with executable code.

[nfischer]

In short, we don't need to create temp files and fork node in shell.exec() in order to output to stdout and capture the output. What we can do instead is use child_process.execSync(..., { stdio: 'inherit' }) to output to the console. The stdout and stderr from the spawned process will be inherited by the current process, so shell output works correctly. We can also intercept stdout and stderr, similar to how it is done in the tests for echo, to return the actual value of stdout and stderr from the call.

Can you provide a minimal proof of concept?

I think this method would be much more reliable and secure than writing temp files with executable code.

This was one of the design considerations of #524

[BurtHarris]

How very interesting @freitagbr. I'd love to see that proof of concept too. You said:

We can also intercept stdout and stderr, similar to how it is done in the tests for echo, to return the actual value of stdout and stderr from the call.

I'm not familiar with these tests, can you summarize briefly how that's done in those tests?

[freitagbr]

The reason the temp-file-and-fork method was used was because we needed to print the stdout/stderr of the spawned process in real-time, but also return all of the data printed to stdout from the shell.exec function call.

This method does the same, but without forking node or using temp files. The simplified procedure goes something like this:

// set up the intercepts for process.stdout.write, process.stderr.write
var stdio = intercept.init();

// use stdio: 'inherit' here, so the stdio from the spawned command is piped to
// the stdio of the node process itself
child_process.execSync(cmd, { stdio: 'inherit' });

// retrieve the string values of the data output to stdout and stderr
var stdout = stdio.stdout();
var stderr = stdio.stderr();

// restore the original process.stdout.write and process.stderr.write
stdio.restore();

Here is a module that does this stdout/stderr intercepting that I demonstrate above: sfarthin/intercept-stdout. We may want to base our implementation on this.

And here is the way stdout/stderr are mocked in the echo tests (note that with these mocks, nothing is printed to stdout or stderr):
test/utils/mocks.js.

Another consideration is that child_process.execSync is unsafe, because it treats the string passed in as the actual shell command. In short, do not pass arbitrary user input to child_process.execSync. A much better choice here would be child_process.execFileSync, which does not have the same security issues as execSync. Perhaps this discussion deserves its own issue.

[nfischer]

Here is a module that does this stdout/stderr intercepting that I demonstrate above: sfarthin/intercept-stdout. We may want to base our implementation on this.

I don't think this works. Try this example that's been adapted from their README:

var intercept = require('intercept-stdout');
var child_process = require('child_process');
var captured_text = '';

var unhook_intercept = intercept(function(text) {
  captured_text += text;
});

child_process.execSync('echo this text is being captured', { stdio: 'inherit' });

// Stop capturing stdout.
unhook_intercept();

console.log('This text is not being captured.');

// This is the text that was captured.
console.log('CAPTURED:', captured_text); // @freitagbr you'll find this to be empty

Another consideration is that child_process.execSync is unsafe, because it treats the string passed in as the actual shell command. In short, do not pass arbitrary user input to child_process.execSync.

I agree with the suggestion, but shell.exec() is actually following its API ("Executes the given command synchronously", and git add .; git commit is a valid command). shell.exec() will always remain this way for backwards compatibility. I'm building an alternative in #524.

Don't pass arbitrary user input to any of our functions. #345 has an example.

Perhaps this discussion deserves its own issue.

This is the main motivation for #524. Discussed to length in #143, #495, #103. Please don't open another issue.