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

Terminal doesn't honor quoted command lines. #19078

Closed
dbaeumer opened this issue Jan 23, 2017 · 17 comments · Fixed by #22565
Closed

Terminal doesn't honor quoted command lines. #19078

dbaeumer opened this issue Jan 23, 2017 · 17 comments · Fixed by #22565
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) verified Verification succeeded windows VS Code on Windows issues
Milestone

Comments

@dbaeumer
Copy link
Member

dbaeumer commented Jan 23, 2017

VSCode Version: Windows.

-create a new terminal with the following shell args
capture

Executing this on a folder that contains a folder test folder results in the following error message:

The system cannot find the file specified.

Using the following args (observer the additional quote around the command) results in the same error

capture

Executing this in a command shell (cmd.exe) works:

capture

As does on powershell (without the extra double quotes):

capture

@dbaeumer
Copy link
Member Author

Could it be that the command line somehow get changed / corrupted until it is passed to the cmd.exe process.

@mousetraps mousetraps added the terminal Integrated terminal issues label Jan 23, 2017
@Tyriar Tyriar added this to the February 2017 milestone Jan 23, 2017
@Tyriar
Copy link
Member

Tyriar commented Jan 23, 2017

I'm guessing this is an issue with the argvToCommandLine function in node-pty.

@dbaeumer
Copy link
Member Author

Could be, but please note that I pass one arg for the command line. So instead of ["dir", "test folder"] which would need proper escaping I pass [""dir "test folder"""] which is exactly one arg and already proper escaped.

@dbaeumer
Copy link
Member Author

I inspected the cmd.exe's command line when spawned and it looks like this (removed /c to keep it alive)

capture

Looks like some double quotes got escaped :-)

@dbaeumer
Copy link
Member Author

Actually I didn't quote the command correctly. It should be quoted dir "test folder" so the JS string in debugger looks like "dir \"test folder\"". Passing this to the terminal still results in \ characters on the command line:

capture

Executing cmd.exe /d /k dir "test folder" from a powershell command prompt results in

capture

which looks correct (no \ to escape ")

@Tyriar
Copy link
Member

Tyriar commented Feb 24, 2017

I think I can reproduce this in node-pty by running the following:

var os = require('os');
var pty = require('..');

var ptyProcess = pty.spawn('cmd.exe', ['/c', 'dir "a b"'], {
  name: 'xterm-color',
  cols: 80,
  rows: 30,
  cwd: "C:\\Users\\daimms\\Documents",
  env: process.env
});

ptyProcess.on('data', function(data) {
  console.log(data);
});

setTimeout(ptyProcess.kill.bind(ptyProcess), 5000);

Output:

C:\Users\daimms\Documents\dev\Tyriar\node-pty>node .\demo\quotes_commands_demo.js
The system cannot find the file specified.

C:\Users\daimms\Documents>cmd /c "dir \"a b\""
The system cannot find the file specified.

C:\Users\daimms\Documents>dir "a b"
 Volume in drive C is OSDisk
 Volume Serial Number is 34DE-8099

 Directory of C:\Users\daimms\Documents\a b

02/24/2017  12:20 PM    <DIR>          .
02/24/2017  12:20 PM    <DIR>          ..
02/24/2017  12:20 PM    <DIR>          test
               0 File(s)              0 bytes
               3 Dir(s)  72,050,741,248 bytes free

C:\Users\daimms\Documents>cmd /c "dir "a b""
 Volume in drive C is OSDisk
 Volume Serial Number is 34DE-8099

 Directory of C:\Users\daimms\Documents\a b

02/24/2017  12:20 PM    <DIR>          .
02/24/2017  12:20 PM    <DIR>          ..
02/24/2017  12:20 PM    <DIR>          test
               0 File(s)              0 bytes
               3 Dir(s)  72,204,726,272 bytes free

A similar example however works fine on Linux using pty.spawn('bash', ['-c', 'ls "a b"'], ...

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug upstream Issue identified as 'upstream' component related (exists outside of VS Code) windows VS Code on Windows issues labels Feb 24, 2017
@Tyriar
Copy link
Member

Tyriar commented Feb 24, 2017

Fixed upstream microsoft/node-pty#41, still want to verify it within VS Code before pushing it.

@Tyriar
Copy link
Member

Tyriar commented Mar 14, 2017

@dbaeumer this should be good to go now, you'll want to update node_modules/node-pty and then set IShellLaunchConfig.args as a string that will be used as is. Example:

const config: IShellLaunchConfig = {
  executable: 'cmd.exe',
  args: '/d /c "dir "test folder""'
};

Let me know if you have any issues.

@dbaeumer
Copy link
Member Author

@Tyriar why is the string option only supported under Windows. You need to escape under Mac and Linux as well. Under these OSes there are even three different kind of space escapings so the code can't simply take one (see http://wiki.bash-hackers.org/syntax/quoting). IMO we need to let users control this.

@dbaeumer dbaeumer reopened this Mar 23, 2017
@Tyriar
Copy link
Member

Tyriar commented Mar 23, 2017

@dbaeumer I was under the impression this only really applies to cmd/Windows.

As node-pty presents an Unix-like interface to conlole applications, it should also provide a unix-like argv[] calling mechanism instead of windows-like escape-Commandline-yourself way. Therefore the Commandline should be build in a way, that matches those rules.

From microsoft/node-pty#41 (comment)

Do you have a particular use case that fails in sh on macOS or Linux?

@dbaeumer
Copy link
Member Author

dbaeumer commented Mar 24, 2017

@Tyriar may be I am not understanding it correctly but here is what I like to allow users do to for an example to run a command in a shell that takes args with spaces in it:

command dirk\ baeumer: escpae using \ for a single space
command "dirk baeumer": escape using weak quoting
command 'dirk baeumer' : escape using strong quoting

How would I pass such a command line to a shell I start under Mac / Linux?

@Tyriar
Copy link
Member

Tyriar commented Mar 24, 2017

Aren't they all basically the same thing? Is there a reason for using anything other than ["dirk baeumer"]? That's all the terminal shell/shellArgs support and I haven't seen any complaints so far.

@dbaeumer
Copy link
Member Author

According to http://wiki.bash-hackers.org/syntax/quoting " and ' have a very different semantic.

So is there a way for me to pass args and tell the shell to not do any quoting. So if I pass ls -ls 'dirk baeumer' will it respect the quoting and leave the string untouched?

@Tyriar
Copy link
Member

Tyriar commented Mar 27, 2017

Is this what you mean?

image

Force waitOnExit in dev build.

Add this to settings.json:

    "terminal.integrated.shell.linux": "/bin/bash",
    "terminal.integrated.shellArgs.linux": [
    "-c",
    "ls -ls 'a b'"
    ]

image

@Tyriar Tyriar modified the milestones: April 2017, March 2017 Mar 28, 2017
@dbaeumer
Copy link
Member Author

Yes. So how would I pass this to a shellLaunchConfig in code in terms of args? Like this: ["-c", "ls -ls 'a b'"] and not one string args like under Windows ?

@Tyriar
Copy link
Member

Tyriar commented Mar 29, 2017

Yes, terminal.integrated.shell.linux and terminal.integrated.shellArgs.linux map 1:1 to the IShellLaunchConfig.executable and .args

@dbaeumer
Copy link
Member Author

@Tyriar thanks!

@dbaeumer dbaeumer added the verified Verification succeeded label Apr 28, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) verified Verification succeeded windows VS Code on Windows issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants