Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process.spawn fails on Windows given a space in both the command and an argument #7367

Closed
s100 opened this issue Jun 22, 2016 · 20 comments
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.

Comments

@s100
Copy link

s100 commented Jun 22, 2016

  • Version: 4.4.4, 4.4.7, 5.10.1, 6.2.2
  • Platform: Windows 7, 64-bit
  • Subsystem: child_process

Reproduction of the error: https://gist.github.com/smrq/f028b22bc748af9e68a7

On Windows, child_process.spawn handles the command incorrectly when both it and one of its arguments contains a space. So, this works fine:

var spawn = require('child_process').spawn;

spawn('nospaces.cmd', ['arg with spaces']);
spawn('command with spaces.cmd', ['nospaces']);

But this yields 'command' is not recognized as an internal or external command, operable program or batch file.:

spawn('command with spaces.cmd', ['arg with spaces']);

(This is node-v0.x-archive #25895, still extant in higher Node.js versions.)

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels Jun 22, 2016
@mscdex
Copy link
Contributor

mscdex commented Jun 22, 2016

/cc @nodejs/platform-windows

@bzoz
Copy link
Contributor

bzoz commented Jun 27, 2016

I can reproduce, working on a fix.

@mcdonnelldean
Copy link

I've had success using nodejs/node-v0.x-archive#25895 (comment) from the linked issue as a work around.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 29, 2016

Is this an issue with exec() too?

@bzoz
Copy link
Contributor

bzoz commented Jun 29, 2016

They are related, exec internally calls spawn.

The problem is in how shell commands are executed under Windows. I have a fix almost ready, I'll publish it probably tomorrow.

@mcollina
Copy link
Member

Does this not happen on v4?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 29, 2016

exec internally calls spawn.

I get that. I was asking if exec() was affected as well, since it does some manipulation before passing to spawn.

@s100
Copy link
Author

s100 commented Jun 29, 2016

@mcollina Problem exists on v4.4.4 as well.

@MylesBorins
Copy link
Contributor

@s100 can you check on v4.4.7

@mcdonnelldean
Copy link

LTS is affected for sure thats what I'm using.

@mcdonnelldean
Copy link

Im my case we are only using spawn but given its at the point of exec I assume its affected too.

@s100
Copy link
Author

s100 commented Jun 29, 2016

@thealphanerd still present on v4.4.7. Any others?

@bzoz
Copy link
Contributor

bzoz commented Jun 30, 2016

Unfortunately I haven't been able to find a fix that would not also break a lot of other use cases. But there is luckily easy work around for this issue - you need to quote script filename and add shell:true to options:

spawn('"with spaces.cmd"', ['arg with spaces'], { shell: true });

@mcdonnelldean
Copy link

Thats pretty much what my fixed boiled down to, quoting the cmd

Kindest Regards,

Dean

On 30 Jun 2016, at 17:59, Bartosz Sosnowski notifications@github.com wrote:

Unfortunately I haven't been able to find a fix that would not also break a lot of other use cases. But there is luckily easy work around for this issue - you need to quote script filename and add shell:true to options:

spawn('"with spaces.cmd"', ['arg with spaces'], { shell: true });


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@bzoz
Copy link
Contributor

bzoz commented Aug 9, 2016

I don't think we can do anything more with this so I'm closing the issue. You can always reopen it if you feel so.

@bzoz bzoz closed this as completed Aug 9, 2016
@s100
Copy link
Author

s100 commented Aug 9, 2016

@bzoz Do you mean that the issue is fixed now?

@bzoz
Copy link
Contributor

bzoz commented Aug 9, 2016

@s100 sorry, I didn't make myself clear and provide summary. My bad.

The thing is, there is no way to fix this. When calling shell scripts under Windows, you need to add {shell: true} to make sure things work ok (doc link). Shell scripts are not executable files under Windows, so they need special treatment. Also, Windows takes all arguments as single strings, not as array like Linux does.

When the script filename has space in it, it needs to be quoted. We cannot do this automatically - spawn is used by exec, for which users pass command and arguments as a single string. If we would add quotes we would break usages like in test-exec.js:93.

FWIW I've opened a PR to add a example to the documentation: #8035

jasnell pushed a commit that referenced this issue Aug 18, 2016
Adds an example of how to spawn a shell script under Windows with
spaces in its filename.

Ref: #7367
PR-URL: #8035
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue Aug 24, 2016
Adds an example of how to spawn a shell script under Windows with
spaces in its filename.

Ref: #7367
PR-URL: #8035
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Macil
Copy link

Macil commented Oct 13, 2022

I just spent hours trying to figure out why electron-reloader failed to restart electron for me, and it ultimately had to do with the directory containing spaces, it trying to execute the ".cmd" executable created by npm for electron while passing it the directory (containing spaces) as an argument, and this issue popping up.


Rust's standard library has an API very similar to child_process.spawn which correctly handles this case of running .bat/.cmd files containing spaces being passed arguments with spaces, so the problem seems possible to solve:

use std::process::Command;
fn main() {
    let ecode = Command::new("C:\\path with spaces\\a b.cmd")
        .args(["xx yy", "1 2"])
        .spawn()
        .expect("failed to execute process")
        .wait()
        .expect("failed to wait on child");
    assert!(ecode.success());
}

It turns out Rust had this same problem before and solved it in rust-lang/rust#95246, by specifically handling the case where the program's filename ends with ".bat" or ".cmd" and creating a full command string involving "cmd /C ..." with the program and the arguments each escaped as necessary.


It's true that child_process's docs tell users to turn on shell: true and escape the command and arguments themselves when a .bat/.cmd is being executed, but this isn't enough to prevent buggy software:

  • Unlike what the docs imply, child_process.spawn/execFile/etc right now mostly work on .bat/.cmd files without doing shell:true and manual escaping. It only seems to break in this case involving spaces both in the program and arguments. Plenty of people will write their code in the naive way, find it works, and not think about it twice. Even if they notice the warning in the docs, given that their code works they may assume the docs are outdated or not actually relevant to them. (And even if they do notice the doc and decide to change their code to follow its recommendation, it will result in their code getting uglier.)
  • npm on Windows creates .cmd files for the executables in dependencies, so .cmd files are commonly encountered and executed in the Node ecosystem.
  • npm on non-Windows platforms creates "real" executables for the executables in dependencies, so someone coding on a non-Windows platform would not expect that they have to do anything special in order to execute executables from dependencies, and their code will be more fragile on Windows than they would expect.

If Node copied Rust's behavior, it would make a ton of existing programs more dependable and stop the years of people still running into this issue.

This would not conflict with #29532 / #29576, because those are only about making arguments work more consistently in the shell: true case as with the shell: false case, which seems sensible regardless of it being motivated by making it easier to intentionally work around this issue. Adopting Rust's behavior would only change the handling of the program argument in the shell: false (and program ends-with .bat/.cmd) case, and would make it unnecessary for people to have to identify and work around this issue.

fannheyward added a commit to neoclide/coc.nvim that referenced this issue Apr 22, 2024
fannheyward added a commit to neoclide/coc.nvim that referenced this issue Apr 22, 2024
Closes #4987

on windows, command should be quoted if containing spaces. nodejs/node#7367
fannheyward added a commit to neoclide/coc.nvim that referenced this issue Apr 23, 2024
Closes #4987

on windows, command should be quoted if containing spaces. nodejs/node#7367
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

10 participants