child_process.exec() fails with spaces in absolute or relative path to binary file

[jorangreef]

• Version: v5.10.1
• Platform: Darwin Joran.local 15.4.0 Darwin Kernel Version 15.4.0: Fri Feb 26 22:08:05 PST 2016; root:xnu-3248.40.184~3/RELEASE_X86_64 x86_64
• Subsystem: child_process

Using exec() to execute an absolute path to a binary, with spaces in the absolute path, e.g. /Users/Joran/test script.sh fails:

{ [Error: Command failed: /Users/Joran/test script.sh
/bin/sh: /Users/Joran/test: No such file or directory
]
  killed: false,
  code: 127,
  signal: null,
  cmd: '/Users/Joran/test script.sh' } '' '/bin/sh: /Users/Joran/test: No such file or directory\n'

[jorangreef]

If the same binary is passed as a relative path, e.g. just test script.sh then exec() succeeds.

[jasnell]

Yep, quick test locally confirms this. As a workaround, if you wrap the command in quotes it should work, e.g.:

child_process.exec('"/path/to/test script.sh"', (err, stdout, stderr) => { });

Looks like we need to make sure we're wrapping it in quotes internally if is isn't wrapped already.

[jasnell]

Care to make a PR? :-)

[jorangreef]

Sure.

I would like to understand a bit more about why test script.sh as a relative path works when the same file as an absolute path does not?

I tried tracing the exec() call through but didn't see where it is being escaped as a relative path or passed to /bin/sh?

[MylesBorins]

@jorangreef I did some digging for you
exec eventually calls spawn and just passes the file argument

spawn sanitizes the input and then callsinternal/child_process.prototype.spawn

If the shell option is passed to child.spawn then file is modified This does not appear to be the case in this instance... but this is likely a good place to modify file

In internal/child_process.prototype.spawn the file is untouched and passed to this._handle.spawn as this.spawnfile

this._handle is an instance of Process which is a binding to process_wrap.Process

The process_wrap.Process Class has it's name set here. Process.prototype.spawn is set here and references this method. The filepath passed as an option to this method is handled here. It is assigned to options.file and passed to libuv

It would appear that the path handling at that point is happening at the libuv layer. We could likely do some prints from the c++ layer to verify, but I'm 99% that we are just passing the filepath as given as long as the shell option is not given.

[jorangreef]

Thanks @thealphanerd!

I see that the shell option is in fact passed as true because of normalizeExecArgs which does:

options.shell = typeof options.shell === 'string' ? options.shell : true;

This means that the options.shell block is in fact executed so that file is modified:

file = '/bin/sh';
args = ['-c', command];

This is eventually executed on Linux and OS X by libuv using execvp without further modification of the arguments (On Windows, libuv calls quote_cmd_arg on each argument if windowsVerbatimArguments is true). If one were to execute this in a Unix terminal it would look like:

/bin/sh -c "command"

So exec('echo hello world') would be:

/bin/sh -c "echo hello world"

And exec('/Users/Joran/test script.sh') would be:

/bin/sh -c "/Users/Joran/test script.sh"

Therein lies the problem, /bin/sh is interpreting the command string as command plus arguments, so that the command is seen as /Users/Joran/test and the arguments are seen as script.sh.

[jorangreef]

@jasnell and @thealphanerd

It looks like Windows is also affected. I have also found the reason why my earlier test script.sh using a relative path succeeded, /bin/test is a valid binary. So relative paths are also affected, it is not just absolute paths.

Should exec() escape special characters in the binary file argument? I think so. Other interfaces such as fs.stat() already handle special characters in the path.

I am sure there are users who are already passing a fully quoted or partially quoted/escaped file to exec(). It is also more complicated then just wrapping with quotes if quotes are not yet already used, as quotes may be literal, and users may have escaped characters rather than quoting them.

I would like to make this change as safe and backward compatible as possible. I think what would work best is if we escape any special character which is not yet already quoted or escaped, as follows:

echo becomes echo
/Users/Joran Greef/"foo bar.sh" becomes /Users/Joran" "Greef/"foo bar.sh"
/Users/Joran Greef/'foo bar.sh' becomes /Users/Joran" "Greef/'foo bar.sh'
/Users/Joran Greef/foo\ bar.sh becomes /Users/Joran" "Greef/foo\ bar.sh
foo bar.sh becomes foo" "bar.sh
foo & bar.sh becomes foo" & "bar.sh
foo\ bar.sh becomes foo\ bar.sh
"foo bar.sh" becomes "foo bar.sh"
"foo\ bar.sh" becomes "foo\ bar.sh"
'foo bar.sh' becomes 'foo bar.sh'
'foo\ bar.sh' becomes 'foo\ bar.sh'

These should all then work when they end up being called. For example:

/bin/sh -c "./foo\" & \"bar.sh"

Should the escaping be done in the options.shell block (this would then apply to exec, execFile, spawn etc.)? This is where file is used to form command:

const command = [file].concat(args).join(' ');

What have I missed? Are there any other edge cases to consider? What should we consider to be quote-worthy special characters on Windows and on Linux and OS X? Anything that's not in [a-zA-Z0-9\.\/]? Would this affect alternate data streams on Windows if : is quoted?

These changes should hopefully not break anything, but if you want me to proceed then I think they should still best be landed only in the next semver major. It would be good also to get some more eyes on this.

[cjihrig]

Could we pass the command to JSON.stringify() to do the escaping? I'm not sure if this takes care of all of the edge cases or not, but I've seen it used in some of our tests, such as test/parallel/test-stdout-close-catch.js.

[bnoordhuis]

Not escaping spaces is the expected behavior. If exec() started escaping, then e.g. exec('echo hello') would stop working.

Not a bug, IMO, although the documentation for exec() can probably be more explicit; "executes the command within that shell" is currently all it says.

[jasnell]

@bnoordhuis ... good point. That would imply that the right solution here is to expand the documentation to address the fact that paths with spaces need to be quoted before passing in.

[jorangreef]

@bnoordhuis Thanks, I came across this via exec() and it make sense we can't escape exec() commands. The intention was just to escape the file argument, not the command itself.

What about execFile? Does the file argument there escape spaces and special characters?