This repository has been archived by the owner. It is now read-only.

child_process.spawn ignores PATHEXT on Windows #2318

Closed
OrangeDog opened this Issue Dec 12, 2011 · 67 comments

Comments

Projects
None yet
@OrangeDog

OrangeDog commented Dec 12, 2011

For example require('child.process').spawn('mycmd') won't find C:\util\mycmd.bat when PATH contains C:\util and PATHEXT contains .BAT.

Ye olde code (https://github.com/joyent/node/blob/v0.4/src/node_child_process_win32.cc) looks like it would have worked, but I have no idea where the v0.6 equivalent is.

@igorzi

This comment has been minimized.

Show comment
Hide comment
@igorzi

igorzi commented Dec 18, 2011

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Dec 20, 2011

Member

At the moment child_process.spawn() can only run exe files. This is a limitation of the CreateProcess API. child_process.exec() can run batch files though. We could make libuv prefix cmd /c if the file is not an exe - but iirc Peter was strongly against that.

@DrPizza @igorzi thoughts?

Member

piscisaureus commented Dec 20, 2011

At the moment child_process.spawn() can only run exe files. This is a limitation of the CreateProcess API. child_process.exec() can run batch files though. We could make libuv prefix cmd /c if the file is not an exe - but iirc Peter was strongly against that.

@DrPizza @igorzi thoughts?

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Dec 20, 2011

Member

@OrangeDog as a workaround, you could use

require('child_process').spawn('cmd', ['/s', '/c', '"C:\\util\\mycmd.bat"'], { 
  windowsVerbatimArguments: true
});

(this option is internal and not guarateed to stick btw)

Member

piscisaureus commented Dec 20, 2011

@OrangeDog as a workaround, you could use

require('child_process').spawn('cmd', ['/s', '/c', '"C:\\util\\mycmd.bat"'], { 
  windowsVerbatimArguments: true
});

(this option is internal and not guarateed to stick btw)

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Dec 20, 2011

Not very helpful for writing portable code though.

OrangeDog commented Dec 20, 2011

Not very helpful for writing portable code though.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Dec 20, 2011

Member

@OrangeDog Well you can't really write a portable batch file anyway.

@DrPizza suggested today that we could add a { shell: true } option to spawn. I kind of like the idea. It allows using spawn for the same purpose as exec without buffering all the output. We also currently have the weird distinction between exec and execFile; we could just make those the same function but with a different default for the shell option. @ry, @bnoordhuis, what do you guys think?

Member

piscisaureus commented Dec 20, 2011

@OrangeDog Well you can't really write a portable batch file anyway.

@DrPizza suggested today that we could add a { shell: true } option to spawn. I kind of like the idea. It allows using spawn for the same purpose as exec without buffering all the output. We also currently have the weird distinction between exec and execFile; we could just make those the same function but with a different default for the shell option. @ry, @bnoordhuis, what do you guys think?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Dec 20, 2011

Member

It'd be superfluous on Unices - the two things a shell script needs is a valid shebang and the executable bit set.

Member

bnoordhuis commented Dec 20, 2011

It'd be superfluous on Unices - the two things a shell script needs is a valid shebang and the executable bit set.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Dec 20, 2011

Member

@bnoordhuis not completely because people may want to do ls -r | grep bla

Member

piscisaureus commented Dec 20, 2011

@bnoordhuis not completely because people may want to do ls -r | grep bla

@dfellis

This comment has been minimized.

Show comment
Hide comment
@dfellis

dfellis Dec 20, 2011

If I can interject into this conversation; the shebang indicates which executable will actually run the file and the executable bit flags that the user (or a user) has granted permission to run the file (probably why auto-executing of shell scripts in the Windows %PATH% was objected to before, since there's no executable bit there).

Perhaps a { shell: "shellname" } option would be better? This would indicate which program you want to pass the file to for execution (being explicit that you do want to execute a script rather than an application), and it could still be useful for Unices.

Basically, the shebang line is invalid syntax in Javascript, but Node.js tolerates it because Unix shells will automatically interpret that line and pass the file to the specified program -- but then that means you can't use that JS file in a browser without modification. A developer using browser-require might instead prefer to spawn the script with { shell: "node" } instead and not include the shebang at all.

EDIT: Of course, in Unix, a shebanged shell script could still be run the normal way.

dfellis commented Dec 20, 2011

If I can interject into this conversation; the shebang indicates which executable will actually run the file and the executable bit flags that the user (or a user) has granted permission to run the file (probably why auto-executing of shell scripts in the Windows %PATH% was objected to before, since there's no executable bit there).

Perhaps a { shell: "shellname" } option would be better? This would indicate which program you want to pass the file to for execution (being explicit that you do want to execute a script rather than an application), and it could still be useful for Unices.

Basically, the shebang line is invalid syntax in Javascript, but Node.js tolerates it because Unix shells will automatically interpret that line and pass the file to the specified program -- but then that means you can't use that JS file in a browser without modification. A developer using browser-require might instead prefer to spawn the script with { shell: "node" } instead and not include the shebang at all.

EDIT: Of course, in Unix, a shebanged shell script could still be run the normal way.

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Dec 20, 2011

@piscisaureus but you can write batch/perl/etc. scripts to allow spawn('scp') or spawn('readlink') to behave in the same way on Windows as on *nix.

I thought the whole point of adding libuv was to give portable cross-platform support.

OrangeDog commented Dec 20, 2011

@piscisaureus but you can write batch/perl/etc. scripts to allow spawn('scp') or spawn('readlink') to behave in the same way on Windows as on *nix.

I thought the whole point of adding libuv was to give portable cross-platform support.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jul 28, 2012

Member

@piscisaureus Is this still an issue?

Member

bnoordhuis commented Jul 28, 2012

@piscisaureus Is this still an issue?

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus
Member

piscisaureus commented Jul 29, 2012

@bnoordhuis It is.

@ghost ghost assigned piscisaureus Jul 29, 2012

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs Aug 2, 2012

I think @piscisaureus's suggestion is on to something.

It's kind of annoying right now that there's a "run in a shell" function that buffers the output (exec), and a "run as-is" option that doesn't (spawn), and a "run as-is" that buffers the output (execFile), but no "run in a shell" that doesn't buffer the output.

However, it can't be spawn(cmd, args, {shell: true}), because that doesn't really work for the ls -laF | grep foo case, right? The cmd is either "sh" or "cmd" depending on platform, and the arg is always either /c $cmd or -c $cmd.

What about this?

child = child_process.spawnShell('util.bat glerp gorp', {options...})

Which would be sugar for:

child = child_process.spawn(isWin ? 'cmd' : 'sh', [isWin?'/c':'-c', arg], options)

isaacs commented Aug 2, 2012

I think @piscisaureus's suggestion is on to something.

It's kind of annoying right now that there's a "run in a shell" function that buffers the output (exec), and a "run as-is" option that doesn't (spawn), and a "run as-is" that buffers the output (execFile), but no "run in a shell" that doesn't buffer the output.

However, it can't be spawn(cmd, args, {shell: true}), because that doesn't really work for the ls -laF | grep foo case, right? The cmd is either "sh" or "cmd" depending on platform, and the arg is always either /c $cmd or -c $cmd.

What about this?

child = child_process.spawnShell('util.bat glerp gorp', {options...})

Which would be sugar for:

child = child_process.spawn(isWin ? 'cmd' : 'sh', [isWin?'/c':'-c', arg], options)
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Aug 2, 2012

Member

So, I don't understand anything about shells on Unix, so take this with a grain of salt. In particular I don't understand how sh -c would affect things. I guess maybe that would let it run chmodded shebanged files?

But for me the problem is that there is lots of published code out there that is not cross-platform because of this. (npm with git and npm-www with couchdb recently, but much more I've seen) A fix that retroactively makes all that code work would be ideal, instead of throwing a new method in the already-confusing medley of child_process and having to evangelize "use this if you want your code to work on Windows."

So I'd say

We could make libuv prefix cmd /c if the file is not an exe - but iirc Peter was strongly against that.

is the most useful thing I've seen so far. Alternately, if there's an alternative to the CreateProcess API that doesn't have this suckiness, that would be nice.

Member

domenic commented Aug 2, 2012

So, I don't understand anything about shells on Unix, so take this with a grain of salt. In particular I don't understand how sh -c would affect things. I guess maybe that would let it run chmodded shebanged files?

But for me the problem is that there is lots of published code out there that is not cross-platform because of this. (npm with git and npm-www with couchdb recently, but much more I've seen) A fix that retroactively makes all that code work would be ideal, instead of throwing a new method in the already-confusing medley of child_process and having to evangelize "use this if you want your code to work on Windows."

So I'd say

We could make libuv prefix cmd /c if the file is not an exe - but iirc Peter was strongly against that.

is the most useful thing I've seen so far. Alternately, if there's an alternative to the CreateProcess API that doesn't have this suckiness, that would be nice.

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs Aug 2, 2012

@domenic sh -c would allow you to do shell syntax stuff, pipes and whatnot. sh -c "ls -laF | grep foo > output.log" would list all the files, search the output lines for "foo" and then write the results to output.log. "ls -laF | grep foo > output.log" isn't an executable name, it's a shell program. (An executable name is also a valid shell program, of course.)

If we start wrapping every command in a shell, then that's not so great.

isaacs commented Aug 2, 2012

@domenic sh -c would allow you to do shell syntax stuff, pipes and whatnot. sh -c "ls -laF | grep foo > output.log" would list all the files, search the output lines for "foo" and then write the results to output.log. "ls -laF | grep foo > output.log" isn't an executable name, it's a shell program. (An executable name is also a valid shell program, of course.)

If we start wrapping every command in a shell, then that's not so great.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Aug 2, 2012

Member

@isaacs I see. Then that seems like a fairly orthogonal concern to the fact that Windows executables come in many flavors (exe, bat, cmd, etc.). People using spawn will not expect shell syntax, but they will expect that spawn("couchdb") works cross-platform without any extra { shell: true } or spawnShell or the like.

It sounds like spawnShell would be independently useful for the

"run in a shell" that doesn't buffer the output

case, but regardless I think spawn should work with .bats, .cmds, etc.

Member

domenic commented Aug 2, 2012

@isaacs I see. Then that seems like a fairly orthogonal concern to the fact that Windows executables come in many flavors (exe, bat, cmd, etc.). People using spawn will not expect shell syntax, but they will expect that spawn("couchdb") works cross-platform without any extra { shell: true } or spawnShell or the like.

It sounds like spawnShell would be independently useful for the

"run in a shell" that doesn't buffer the output

case, but regardless I think spawn should work with .bats, .cmds, etc.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 2, 2012

Member

libuv used to use PATHEXT, but this was reverted in joyent/libuv@8ed2ffb because it didn't work for non-exe files. I would be ok with putting this back in and running all non-exe files with "cmd /c". I take patches. (Note that the escaping rules are quite complicated when running stuff with cmd /c)

Member

piscisaureus commented Aug 2, 2012

libuv used to use PATHEXT, but this was reverted in joyent/libuv@8ed2ffb because it didn't work for non-exe files. I would be ok with putting this back in and running all non-exe files with "cmd /c". I take patches. (Note that the escaping rules are quite complicated when running stuff with cmd /c)

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Aug 6, 2012

Member

@piscisaureus Would using ShellExecuteEx instead of CreateProcess be a good route toward this?

Otherwise, if I were to work on a patch for this, would you suggest it as a libuv patch or Node patch? Probably libuv, right?

Member

domenic commented Aug 6, 2012

@piscisaureus Would using ShellExecuteEx instead of CreateProcess be a good route toward this?

Otherwise, if I were to work on a patch for this, would you suggest it as a libuv patch or Node patch? Probably libuv, right?

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 6, 2012

Member

@domenic Afaik ShellExecuteEx doesn't allow redirection of stdio handles.

Member

piscisaureus commented Aug 6, 2012

@domenic Afaik ShellExecuteEx doesn't allow redirection of stdio handles.

@geekytime

This comment has been minimized.

Show comment
Hide comment
@geekytime

geekytime Sep 19, 2012

The problem seems to be that child_process got a bit muddied up trying to handle some of these cases for shell commands, but we've reached a point where it's getting uncomfortable to continue to add shims in there to support more shell-friendly options. I don't think this is a cross-platform problem. It's a problem of executables vs. shell scripts. This isn't a problem for most Unix devs, because they already know the difference between the two.

I'm primarily a Windows guy myself, but it doesn't seem fair that Windows should get special treatment at the child_process level just because cmd and bat files are treated as shell commands by the OS. Windows devs will just need to learn the difference between an executable and a shell command for their platform. (This isn't just a problem with Node, BTW. I've seen this play out in other platforms, too.) We need to respect Windows's idea of what is executable, and what is not, and be careful not to break that.

I initially liked the idea of adding a {shell:true} sort of option to spawn(), but after looking at the code, I'd expect it to be supported in exec() and execFile(), too, and that would break things. (We could have different defaults for spawn() and exec(), but that's still pretty confusing.)

Rather than try to make the mess of spawn(), exec(), and execFile() do even more black magic, and risk making things even more unclear, I'd prefer that we keep things honest and add @isaacs's suggestion of adding a spawnShell() method. If we clean up the docs a bit, and make it more clear what each of these methods does, we've got a shot at having a fairly complete set of use cases.

In the long term, it might be worth considering the deprecation of exec() and execFile(), in favor of a set of functions with a more deliberate separation between executables and shell commands. The code would get a lot cleaner, and with just slightly better documentation and error messages, it would also be an easier API for new developers to understand.

geekytime commented Sep 19, 2012

The problem seems to be that child_process got a bit muddied up trying to handle some of these cases for shell commands, but we've reached a point where it's getting uncomfortable to continue to add shims in there to support more shell-friendly options. I don't think this is a cross-platform problem. It's a problem of executables vs. shell scripts. This isn't a problem for most Unix devs, because they already know the difference between the two.

I'm primarily a Windows guy myself, but it doesn't seem fair that Windows should get special treatment at the child_process level just because cmd and bat files are treated as shell commands by the OS. Windows devs will just need to learn the difference between an executable and a shell command for their platform. (This isn't just a problem with Node, BTW. I've seen this play out in other platforms, too.) We need to respect Windows's idea of what is executable, and what is not, and be careful not to break that.

I initially liked the idea of adding a {shell:true} sort of option to spawn(), but after looking at the code, I'd expect it to be supported in exec() and execFile(), too, and that would break things. (We could have different defaults for spawn() and exec(), but that's still pretty confusing.)

Rather than try to make the mess of spawn(), exec(), and execFile() do even more black magic, and risk making things even more unclear, I'd prefer that we keep things honest and add @isaacs's suggestion of adding a spawnShell() method. If we clean up the docs a bit, and make it more clear what each of these methods does, we've got a shot at having a fairly complete set of use cases.

In the long term, it might be worth considering the deprecation of exec() and execFile(), in favor of a set of functions with a more deliberate separation between executables and shell commands. The code would get a lot cleaner, and with just slightly better documentation and error messages, it would also be an easier API for new developers to understand.

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate commented Nov 6, 2012

bump

@pghalliday

This comment has been minimized.

Show comment
Hide comment
@pghalliday

pghalliday Nov 20, 2012

I'd like to add my 2 cents with regard to using

require('child_process').spawn('cmd', ['/s', '/c', '"C:\\util\\mycmd.bat"']);

on windows. This is giving me a real headache as doing this with long running processes results in those processes being orphaned when I call child.kill() ie. the cmd process is killed but the process it spawns is not.

See this gist for an example:

https://gist.github.com/4117163

I'm still trying to figure out if this is a bug that should be raised separately (child.kill seems to deliberately not kill grandchildren (which is annoying also) and this might be considered a grandchild)

pghalliday commented Nov 20, 2012

I'd like to add my 2 cents with regard to using

require('child_process').spawn('cmd', ['/s', '/c', '"C:\\util\\mycmd.bat"']);

on windows. This is giving me a real headache as doing this with long running processes results in those processes being orphaned when I call child.kill() ie. the cmd process is killed but the process it spawns is not.

See this gist for an example:

https://gist.github.com/4117163

I'm still trying to figure out if this is a bug that should be raised separately (child.kill seems to deliberately not kill grandchildren (which is annoying also) and this might be considered a grandchild)

Jishaxe added a commit to Jishaxe/forever-monitor that referenced this issue Mar 8, 2017

@Jishaxe Jishaxe referenced this issue Mar 8, 2017

Open

Fix #140 #145

ChristianMurphy added a commit to ChristianMurphy/eslint-git-changes that referenced this issue Mar 27, 2017

@drkmullins drkmullins referenced this issue Jun 6, 2018

Merged

Add Windows support for spawning mvn #5028

0 of 7 tasks complete
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.