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

Backslash Escaping Problems @next #316

Closed
shellscape opened this issue Jun 25, 2019 · 12 comments · Fixed by #317
Closed

Backslash Escaping Problems @next #316

shellscape opened this issue Jun 25, 2019 · 12 comments · Fixed by #317

Comments

@shellscape
Copy link

Hey folks. Using 2.0.0-alpha.0. I've exhausted options for figuring this out (sans diving into the code). I'm trying to run a command using execa.command which is throwing an error with the awk portion of the command. Here's the raw command as used in the terminal:

$ df -H /dev/disk1 | awk '/\// {printf("%s\n", $2)}'

And here's how I'm executing it in code:

const diskPath = '/dev/disk1';
execa.command(`df -H ${diskPath} | awk '/\// {printf("%s\n", $2)}'`);

I believe the error messages hold some clue. With this variation the error message reads:

`Command failed with exit code 1 (EPERM): df -H /Volumes/wpr | awk /// {printf("%s␊
    ", $2)}`

If I double escape, as such:

execa.command(`df -H ${diskPath} | awk '/\\// {printf("%s\n", $2)}'`);

The error message reads:

`Command failed with exit code 1 (EPERM): df -H /Volumes/wpr | awk /\\// {printf("%s␊
    ", $2)}`

It's important that I get that regex for awk correct, or the command will fail to return the correct data. Is this a bug in execa and/or is there a way around this to define the command in another way that I can pass the right regex to awk?

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 25, 2019

Thanks for the question @shellscape!

execa.command() is just a thin wrapper around execa() which allows passing arguments as a single string instead of an array. The | operator is something that is interpreted by Bash (or other Unix shell), so you need to specify the shell option. Please note that using a shell is not cross-platform (your code won't work on Windows), slower and (although not in your case) potentially unsafe.

I will submit a PR to improve the documentation and make the above points clearer.

Also please checkout 2.0.0, we just released it :)

@ehmicky ehmicky closed this as completed Jun 25, 2019
@shellscape
Copy link
Author

Perfect, thank you.

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 25, 2019

By the way, you might get better performance by doing what awk is doing but inside Node.js. Doing this would also allow you not to use the shell option. But that's up to you :)

@shellscape
Copy link
Author

shellscape commented Jun 25, 2019

Tacked the shell: true option on and the same issue persists. Specifying /\\// in the command string to pass /\// onto the command still seems right to me, but something is getting munged along the way. Some context:

    stderr: `awk: non-terminated string %s... at source line 2␊
     context is␊
    	/\\// {printf("%s >>> ␊
     <<< `,

@shellscape
Copy link
Author

@ehmicky when you have the time (precious as that can be) I'd like to work through resolving this one, as there doesn't seem to be any other references for this particular problem, and I'm positive it would be a good resource for someone facing the same thing in the future.

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 25, 2019

This is an escaping problem, that's not specific to execa. Also it's not specific to execa.command(), you get the same result if you break down your arguments as an array and pass them to execa('df', [...], {shell: true}).

The argument passed to awk is escaped with single quotes in the shell, but the underlying Node.js child_process is doing escaping differently. Some special characters might need double or even quadruple backslashes. Others might need fewer.

If you can get this working with child_process.exec(), you should be able to do this with execa too.

I can try to investigate what the specific escaping of your command should be, but it does look like everything is working as intended.

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 25, 2019

Would the following work?

const diskPath = '/dev/disk1';
const {stdout} = await execa.command(`df -H ${diskPath}`);
const [line] = stdout.split('\n')
const [, size] = line.split(/ +/)

This is also much faster than using awk and works on any OS.

@shellscape
Copy link
Author

I am doing exactly that now 😄 Still working on the escaping though, there has to be a solution to that. I'm surprised that it's not been encountered before.

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 25, 2019

Again the escaping is not a problem, it's just about figuring out the right amount of backslashes. Also it's not specific to execa, you get the same thing with child_process.spawn().

@shellscape
Copy link
Author

We're agreed on those points.

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 25, 2019

Please note than you get several layers of escaping:

  • execa(), execa.command() or child_process.spawn() do not do any escaping (they just forward the raw characters) unless you use the shell option. If you do use it, some escaping might take place in Node.js and/or inside execa (more specifically npm-cross-spawn one of our dependencies, and only on Windows).
  • the shell interpreter (e.g. Bash) handles escaping
  • awk has its own escaping mechanisms (if I recall correctly)

To make things more complex, escaping is completely different on Windows (cmd.exe) so whichever way you go, it won't work on any OS (if you use the shell option).

The complexity of escaping is just one of the many reasons to avoid shell interpreters unless necessary.

@shellscape
Copy link
Author

I think that bit of information will be extremely helpful for folks in the future. Since it sounds like a rabbit hole, I say we leave it there. Well done, and thank you for humoring this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants