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

Cannot spawn shell script if path has spaces #38490

Closed
robertpatrick opened this issue Apr 30, 2021 · 23 comments
Closed

Cannot spawn shell script if path has spaces #38490

robertpatrick opened this issue Apr 30, 2021 · 23 comments
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.

Comments

@robertpatrick
Copy link

  • Version: 14.16.0
  • Platform: MacOS 10.15.7 (Windows and Linux too)
  • Subsystem: child_process

What steps will reproduce the bug?

  1. Create a javascript program that spawns a shell script.
  2. Ensure that the full path to the shell script has spaces in the directory names.
  3. Run the javascript program.
async function executeChildProcess(executable, argList) {
  const child = spawn(executable, argList, {
    stdio: [ 'pipe', 'pipe', 'pipe' ],
    shell: true,
    detached: false,
    windowsHide: true
  });
  await onExit(child)
    .then(() => console.log(`${executable} exited with exit code 0`))
    .catch((err) => console.log(`${executable} failed: ${err.toString()}`));

  return child.exitCode;
}

executeChildProcess('/Applications/My Application.app/Contents/tools/myScript.sh', [ '-v' ]).then();

How often does it reproduce? Is there a required condition?

Every single time. Yes, the child_process module should be properly escaping commands that have spaces in them since we cannot control where the user installs the application.

What is the expected behavior?

The shell script is executed properly

What do you see instead?

/bin/sh: /Applications/My: No such file or directory
Applications/My Application.app/Contents/tools/myScript.sh failed: Error: Exit with error code: 127

Additional information

@pd4d10
Copy link
Contributor

pd4d10 commented May 1, 2021

Cannot reproduce on my local machine.

Could you please provide more information, for example, what's the implementation of onExit function?

@Ayase-252 Ayase-252 added the child_process Issues and PRs related to the child_process subsystem. label May 1, 2021
@Ayase-252
Copy link
Member

I can reproduce on Node.js v16

const { spawn } = require('child_process');
const path = require('path');

const cp = spawn(
  path.resolve(__dirname, 'with space', 'shell.sh'),
  {
    shell: true,
  });

cp.stderr.on('data', (chunk) => {
  console.log(chunk.toString());
});

It exits with 127 and shows

/bin/sh: /Users/qingyudeng/projects/node/with: No such file or directory

@pd4d10
Copy link
Contributor

pd4d10 commented May 1, 2021

I guess it's caused by the shell: true option. Set to false should solve this issue.

Or add quotes if shell is necessarily set to true:

spawn('"path with space.sh"')

@robertpatrick
Copy link
Author

Thanks, I had already figured out the workaround with extra quotes. I still consider this a bug that should be fixed since adding quotes to a Javascript string is pretty non-intuitive...

@RaisinTen
Copy link
Contributor

Another way is to escape the space character:

const { spawn } = require('child_process');
const path = require('path');

const cp = spawn(
  path.resolve(__dirname, 'with\\ space', 'shell.sh'),
  {
    shell: true,
  });

cp.stdout.on('data', (chunk) => {
  console.log(chunk.toString());
});

Also, this behaviour seems to be compliant with the way the C library function system works, which also invokes the shell:

#include <stdlib.h>

int main() {
  system("with\\ space/shell.sh");

  return 0;
}

@robertpatrick
Copy link
Author

robertpatrick commented May 1, 2021

@RaisinTen step back away from the fix and think about the child_process.spawn API. From an API perspective, I am passing the executable argument as a single Javascript string, as specified by the API. I shouldn't have to understand the underlying implementation requires me to "fix up" the argument in certain scenarios. You can make all the arguments you want but from an API user perspective, the implementation should handle this for me since it is a limitation of the implementation.

By the way, I suspect that the argument list elements may suffer from the same problem...

@Trott
Copy link
Member

Trott commented May 2, 2021

I don't know for certain, but I'm going to guess this is the expected behavior with shell: true and will not be considered a bug. But let's check with @nodejs/child_process.

@schamberg97
Copy link
Contributor

Perhaps the compromise could be specifying the workarounds in the docs?

@Trott
Copy link
Member

Trott commented May 3, 2021

Perhaps the compromise could be specifying the workarounds in the docs?

Yeah, definitely, if it's not a bug, that should be included in the docs with an example. It is in there already, but kind of hidden in a section that applies only to Windows.

@robertpatrick
Copy link
Author

Agreed. While spaces and other special characters are more common on Windows, they are also possible on Unix-based platforms. We are building an Electron-based app and packaging some scripts with the app. Because of how MacOS deals with application names mapping to the .app directory, we end up with spaces in the path because we want our application name to appear in the Launcher in human-readable form rather than Pascal- or camel-case...

@RaisinTen RaisinTen added the doc Issues and PRs related to the documentations. label May 4, 2021
@RaisinTen
Copy link
Contributor

@robertpatrick Would you like to send a PR to fix this issue?

@robertpatrick
Copy link
Author

robertpatrick commented May 6, 2021

I am happy to take a whack at it if I can figure out where the relevant code is and how to build/test it.

@RaisinTen
Copy link
Contributor

@koush
Copy link

koush commented Sep 18, 2021

None of these workarounds work for a path with spaces in Windows as far I can tell from testing.

@koush
Copy link

koush commented Sep 21, 2021

Also, this behaviour seems to be compliant with the way the C library function system works, which also invokes the shell:

@RaisinTen

The "system" command in C and the "exec" command in C are different. Exec takes the path to an executable as the first argument. So this is named poorly so as to be misleading, since this behavior happens with child_process.exec/spawn. There is no child_process.system. As far as I can tell, there's no way to call C exec (which would give desirable behavior with spaces with paths).

@RaisinTen
Copy link
Contributor

@koush

None of these workarounds work for a path with spaces in Windows as far I can tell from testing.

Using "s works on CMD for me:

C:\Users\Administrator\Desktop\temp>type "Hello world\test.bat"
echo Hi
C:\Users\Administrator\Desktop\temp>"Hello world\test.bat"

C:\Users\Administrator\Desktop\temp>echo Hi
Hi

C:\Users\Administrator\Desktop\temp>node -p "child_process.execSync('\"Hello world\\test.bat\"').toString()"

C:\Users\Administrator\Desktop\temp>echo Hi
Hi

Also, this behaviour seems to be compliant with the way the C library function system works, which also invokes the shell:

@RaisinTen

The "system" command in C and the "exec" command in C are different. Exec takes the path to an executable as the first argument.

I never said that they are the same.

So this is named poorly so as to be misleading, since this behavior happens with child_process.exec/spawn. There is no child_process.system.

It's unlikely that the name will change anytime soon.

As far as I can tell, there's no way to call C exec (which would give desirable behavior with spaces with paths).

Is this what you're looking for #21664?

@robertpatrick
Copy link
Author

By putting an extra set of double quotes, I was able to get spawn to work properly.

const command = ‘“/path with spaces/to/executable”’;
const args = [
    -f’,
    ‘“arg with spaces”’
];
const cp = spawn(command, args, );

@koush
Copy link

koush commented Sep 26, 2021

I never said that they are the same.

You indicated that this is the expected behavior of "system" which is named after the system C call which is different from the exec C call. Citing that as the reason for their behavior. The confusion is understandable when they're not analogous to behavior of POSIX functions, because they're named after mismatched POSIX counterparts. I expect to have to quote args in C "system", but I do not expect that for "exec".

It's unlikely that the name will change anytime soon.

Yeah, I get that.

Is this what you're looking for #21664?

Not exactly, process replacement isn't the issue. What we need is having a way to launch a process without having to worry about quoting the executable (something that did not work for me, but I'll try again). This is problematic because the execFile/spawn call accepts "args" arguments: why would one need to quote the executable in the "command" parameter to prevent it from being arg split, if there exists an args parameter? I expected that the executable path is the simply the "command" parameter, and the "args" to contain the arguments.

This is a bug farm lurking in the API. Every consumer of the process.exec/spawn API is going to eventually run into this quoting issue when a user of theirs has the program in a path with a space. Then they'll come find this bug, and go back and fix their code.

A non-replacement "exec" exists in the windows APIs for this purpose (as far as I know, exec-replace doesn't exist on windows). https://docs.microsoft.com/en-us/cpp/c-runtime-library/exec-wexec-functions?view=msvc-160

@DesignByOnyx
Copy link

Hoping this helps someone else out - but for me I was using { shell: 'powershell.exe' } and I needed to escape any spaces in the path name. Powershell's escape character is the backtick character (not backslash!!) , so I had to do this:

spawn(pathToExe.replaceAll(' ', '` '), { shell: 'powershell.exe' }); 

@bnoordhuis
Copy link
Member

Since this isn't a bug, I'm going to close the issue. Please send a pull request if there's still room to improve the documentation.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Feb 7, 2023
@karikera
Copy link

karikera commented Apr 12, 2023

@bnoordhuis
it's occurred on my project. and It definitely looks like a bug.
node.js v.18.70
OS: Windows

I tried to use child_process.spawn for showing the console window.

// process.argv0 = "C:\\Program Files\\nodejs\\node.exe" // it contains whitespace.
const child = child_process.spawn(process.argv0, ['.'], {shell:true});
child.spawnargs; //  ['C:\WINDOWS\system32\cmd.exe', '/d', '/s', '/c', '"C:\Program Files\nodejs\node.exe ."']

and cmd fails to execute the node process because it contains whitespace in the path.
and it can be bypassed by wrapping the process name with the quote. (child_process.spawn('"' + process.argv0 + '"', ... ))

@koush
Copy link

koush commented Apr 12, 2023

Would once again like to suggest this is in fact an API deficiency given people keep linking to this issue.

What we need is having a way to launch a process without having to worry about quoting the executable (something that did not work for me, but I'll try again). This is problematic because the execFile/spawn call accepts "args" arguments: why would one need to quote the executable in the "command" parameter to prevent it from being arg split, if there exists an args parameter? I expected that the executable path is the simply the "command" parameter, and the "args" to contain the arguments.

This is a bug farm lurking in the API. Every consumer of the process.exec/spawn API is going to eventually run into this quoting issue when a user of theirs has the program in a path with a space. Then they'll come find this bug, and go back and fix their code.

@bnoordhuis
Copy link
Member

It's not a bug, it's people holding it wrong. The documentation is, IMO, quite clear about it but, again, please send a pull request if you think it can be improved.

taeold pushed a commit to firebase/firebase-tools that referenced this issue May 12, 2023
Proposed fix for #5830 

Issue seems to be caused by [spawn](https://github.com/firebase/firebase-tools/blob/f8f19dc060e6daf060b87d38fd2a38d24bab8210/src/functions/python.ts#L38) raising an error when path provided has spaces. Might be related to nodejs/node#38490. 

### Scenarios Tested

Similar to steps provided in #5830 

1. Run `mkdir 'i like spaces'`
2. Run `cd i\ like\ spaces` or `cd 'i like spaces'`
3. Run `firebase init functions --project <project_id>`
    1. Select `Python`
    2. Do you want to install dependencies now? Yes
4. Run `cd functions`
5. Run `firebase emulators:start`
    1. Emulators start with no issues
7. Uncomment code in `main.py` to deploy
8. Run `firebase deploy --only functions`
    1. Functions deployed with no issues

### Sample Commands

`firebase emulators:start` && `firebase deploy --only functions`
tonyjhuang pushed a commit to firebase/firebase-tools that referenced this issue May 22, 2023
Proposed fix for #5830 

Issue seems to be caused by [spawn](https://github.com/firebase/firebase-tools/blob/f8f19dc060e6daf060b87d38fd2a38d24bab8210/src/functions/python.ts#L38) raising an error when path provided has spaces. Might be related to nodejs/node#38490. 

### Scenarios Tested

Similar to steps provided in #5830 

1. Run `mkdir 'i like spaces'`
2. Run `cd i\ like\ spaces` or `cd 'i like spaces'`
3. Run `firebase init functions --project <project_id>`
    1. Select `Python`
    2. Do you want to install dependencies now? Yes
4. Run `cd functions`
5. Run `firebase emulators:start`
    1. Emulators start with no issues
7. Uncomment code in `main.py` to deploy
8. Run `firebase deploy --only functions`
    1. Functions deployed with no issues

### Sample Commands

`firebase emulators:start` && `firebase deploy --only functions`
SpaceFoon added a commit to SpaceFoon/Ez-Game-Audio-Conversion that referenced this issue Feb 13, 2024
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

No branches or pull requests

10 participants