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 on Windows uses cmd /C, this corrupts passed arguments #145265

Closed
rioj7 opened this issue Mar 15, 2022 · 25 comments
Closed

Terminal on Windows uses cmd /C, this corrupts passed arguments #145265

rioj7 opened this issue Mar 15, 2022 · 25 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone

Comments

@rioj7
Copy link

rioj7 commented Mar 15, 2022

Environment data

  • VS Code version: 1.62.2
  • Extension version: v2021.12.1559732655
  • OS and version: Windows 8.1
  • Python version: Python 3.6

Steps to reproduce:

Using the following Python script (try-argparse.py)

import sys
print(f'sys.argv: {sys.argv}')

with this launch.json

{
  "version": "0.2.0",
  "configurations": [
    {
      "name": "Python: Current File with arguments",
      "type": "python",
      "request": "launch",
      "program": "${file}",
      "args": [
        "-f",
        "\"AMOUNT>=0,06\""
      ]
    }
}

Because I have a > in the argument I have to put the argument in double quotes.

Executing the config results in the following command in the terminal:

c: && cd c:\test && cmd /C "c:\Python36\python.exe c:\Users\XXXX\.vscode\extensions\ms-python.python-2021.12.1559732655\pythonFiles\lib\python\debugpy\launcher 60722 -- c:\test\try-argparse.py -f """AMOUNT>=0,06""" "

The output in the terminal:

sys.argv: ['c:\\test\\try-argparse.py', '-f', '"AMOUNT,06"']

The > is gone (cmd removes it), and I get extra " characters.


If I start the debugger with the following command (no cmd /C "..."):

c: && cd c:\test && c:\Python36\python.exe c:\Users\XXXX\.vscode\extensions\ms-python.python-2021.12.1559732655\pythonFiles\lib\python\debugpy --listen 60722 c:\test\try-argparse.py -f "AMOUNT>=0,06"

I can add the argument --wait-for-client if I want to attach to the debugger with VSC.

I get in the terminal

sys.argv: ['c:\\test\\try-argparse.py', '-f', 'AMOUNT>=0,06']

The correct argument as set in the launch config.

What is the reason to use cmd /C "debug launch command"?
And in this process the " in the argument gets transformed to """

@karthiknadig karthiknadig transferred this issue from microsoft/vscode-python Mar 15, 2022
@int19h
Copy link

int19h commented Mar 15, 2022

cmd /c comes from VSCode itself when we request to run a given command line using your currently selected shell.

That said, why do you need to additionally quote your argument that contains >? All quoting and escaping that's necessary to keep arguments properly separated on the command line should be done by VSCode (since it varies depending on the shell being used). If things don't work for you when you don't quote >, then that is the real bug here.

@rioj7
Copy link
Author

rioj7 commented Mar 16, 2022

@int19h

Then why is this question transferred to microsoft/debugpy?

I asked it in microsoft/vscode-python because I suspected it not done by debugpy. So it should be asked in microsoft/vscode?

No matter how you write the argument in launch.json, cmd /C "...." removes the > and a few characters following and creates a file out redirection.

If you type the same command as put between " on the command line (prompt c:\somepath >) you have to put the argument between " and it works.

c: && cd c:\test && c:\Python36\python.exe c:\Users\XXXX\.vscode\extensions\ms-python.python-2021.12.1559732655\pythonFiles\lib\python\debugpy --listen 60722 c:\test\try-argparse.py -f "AMOUNT>=0,06"

I just found out (somebody posted an image with Powershell as terminal) that the Powershell logic puts each argument between ', no additional shell (like cmd /C) This prevents you from setting up an input/output redirect on the launch command but that does not happen often, you can use a task for that case.

Can't the logic for Command Prompt be the same as for Powershell, but now put each argument between ", maybe ' will also work. The logic to transform " to """ does not work.

@int19h
Copy link

int19h commented Mar 16, 2022

It should be filed in https://github.com/microsoft/vscode/ (with a reference to this one); I can't transfer it there automatically, unfortunately.

And yes, escape rules differ from shell to shell, and VSCode has ad-hoc escape implementations for each of the supported ones, so a bug in one does not necessarily apply to any other. I'm not surprised that cmd in particular has these issues, as it has extremely convoluted escaping/quoting rules in general.

@karthiknadig
Copy link
Member

I can transfer it over to vscode.

@karthiknadig karthiknadig transferred this issue from microsoft/debugpy Mar 16, 2022
@karthiknadig karthiknadig changed the title Python debug on Windows uses cmd /C, this corrupts passed arguments Terminal on Windows uses cmd /C, this corrupts passed arguments Mar 16, 2022
@Tyriar
Copy link
Member

Tyriar commented Mar 17, 2022

@karthiknadig be careful transferring issues as labels get auto created in the target repo

@Tyriar Tyriar removed the classify label Mar 17, 2022
@Tyriar Tyriar assigned connor4312 and unassigned Tyriar Mar 17, 2022
@connor4312 connor4312 assigned roblourens and weinand and unassigned connor4312 Mar 17, 2022
@roblourens roblourens added bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues labels Apr 7, 2022
@roblourens roblourens added this to the April 2022 milestone Apr 7, 2022
@roblourens
Copy link
Member

Getting this perfectly correct is a losing game, but it should be closer to correct now. Please try it out in insiders

@weinand weinand removed their assignment Apr 12, 2022
@rioj7
Copy link
Author

rioj7 commented Apr 14, 2022

@roblourens

Tried with

Version: 1.67.0-insider (user setup)
Commit: a7a7f209ade96c516ce3cd9b330aa8c3db816223
Date: 2022-04-14T05:15:18.304Z

with this launch config

{
  "version": "0.2.0",
  "configurations": [
    {
      "name": "Python: Current File with arguments",
      "type": "python",
      "request": "launch",
      "program": "${file}",
      "args": [
        "-f",
        "AMOUNT>=0,06"
      ]
    }
}

Running the try-argparse.py script results in the following terminal output

D:\somedir> d: && cd d:\somedir && cmd /C "C:\Python38\python.exe c:\Users\somebody\.vscode-insiders\extensions\ms-python.python-2022.4.1\pythonFiles\lib\python\debugpy\launcher 49775 -- d:\somedir\try-argparse.py -f "AMOUNT>=0,06" "

D:\somedir>

The quoting of the argument is better.

But it still creates a file 0.

In the question I made a mistake saying that there was terminal output, it should have been a file 0 is created with the content sys.argv: ['c:\\test\\try-argparse.py', '-f', '"AMOUNT,06"']

The content of the file0 is:

sys.argv: ['d:\\somedir\\try-argparse.py', '-f', 'AMOUNT,06']

The text >=0 is removed by cmd /C and used as a file redirection. cmd /C always removes the > no matter how you format/escape the command arguments.

But looking at the code workbench/contrib/debug/node/terminals.ts I see the reason for cmd /C. It is used to manipulate the environment strings and keep the environment of the current terminal the same.

But this is not done for Powershell, it just set and clear the variables.

And if the environment is to messed up you just kill the current terminal and start a new one.

Why not do the same for ShellType.cmd?

Remove the following lines:

135:    command += 'cmd /C "';

149:   if (env) {
150:   	command += '"';
151:   }

@roblourens
Copy link
Member

roblourens commented Apr 14, 2022

I wonder why we don't use powershell by default? This is using the default "automation profile" from the terminal service, do you know @isidorn? I assume that changing it would break somebody's launch config though.

@roblourens
Copy link
Member

I realized it does use powershell by default for me, do you have any terminal settings set @rioj7?

@rioj7
Copy link
Author

rioj7 commented Apr 14, 2022

@roblourens It doesn't matter what the default terminal config is.

The problem is that using cmd /C for cmd.exe corrupts the arguments and there is NO reason to use it.

The solution is to remove the 4 lines.

@roblourens
Copy link
Member

@weinand looks like you added this originally, do you know what the idea is behind wrapping the command with cmd /C?

It's fine either way, I can escape the > with ^ and it works in this context, that's the right way to do it.

And @rioj7 I know it should be fixed for cmd, but was just curious because we should use powershell by default whenever possible. It seems like it should be using powershell unless you have set the automationProfile/automationShell setting, or if vscode can't find powershell.

@rioj7
Copy link
Author

rioj7 commented Apr 15, 2022

@roblourens I had to change the default terminal profile to cmd to get a cmd prompt.

I looked at the blame and the commit for these lines and weinand moved this function from another file (4 years ago), and the cmd /C was already in this code. I did not look at the history and blame of that other file to see who the original author is.

I don't see a reason to hold on to these 4 lines by trying to escape some characters with ^, because cmd /C has no purpose. If the purpose was environment variables then there should also be such a construct in PowerShell, but it is not.

@roblourens roblourens reopened this Apr 15, 2022
@isidorn
Copy link
Contributor

isidorn commented Apr 19, 2022

@roblourens I do not know. @weinand might know better on what terminal profile we should use by default.

@weinand
Copy link
Contributor

weinand commented Apr 19, 2022

IIRC, I'm using cmd /C ... to introduce a local scope for the env vars (as already speculated by @rioj7).
And not doing the same for PowerShell is most likely an omission on my side (and a bug).

Please note: the ultimate goal of prepareCommand() in terminal.ts is to make all characters used in arguments survive the underlying shell (that is prevent them from being interpreted by the shell). So redirecting output via < and > syntax is not supported. The reason for this approach is to have "portable" launch configs that are independent from the command line interpreters used on different OSes.

But may be that's a "losing game" (as already pointed out by @roblourens).

@roblourens
Copy link
Member

Right, I will keep it then and escape characters inside that cmd /C

I opened #147709 for doing the same with powershell, either I can spawn a subshell in the same way or just clear environment variables manually.

@rioj7
Copy link
Author

rioj7 commented Apr 19, 2022

@weinand There is no reason to use cmd /C because I can remove/set variables by using the correct options in the env argument of the launch config, like I have to do for Powershell.

If we just delete the cmd /C then all arguments are passed to the application correct by enclosing the arguments with " like rob has done now.

Now rob is going to implement some workaround to Escape some characters because we have no reason to keep these 4 holy lines, but we keep them in.

By the way the cmd /C should not be part of the command because there is no change in the environment, I don't see set commands in the terminal. Thus we enclose a nice command with some holy garbage that has no function in this launch config.

If I make a mess of the environment by using the env argument of launch config wrong, I just delete the terminal and start a new one. Just like I have to do if I use Powershell.

If this local environment would be important then by now somebody using Powershell would have complained or he would have adjusted the launch config to set/reset all the environment variables he uses.

Just deleting these 4 lines would make cmd the same as Powershell no need for some workarounds that could lead to other strange behavior if people use these escape characters in there arguments (regex expressions as arguments).

@roblourens
Copy link
Member

By the way the cmd /C should not be part of the command because there is no change in the environment, I don't see set commands in the terminal.

The debug adapter can specify environment variables, python apparently doesn't support this with the terminal but others do and we have code to handle that.

If you run one launch config with some environment variables, then another launch config, the vars from the first run shouldn't leak into the second. We could do that by killing the terminal too, but it's faster to keep it around. Either way, this is getting fixed, don't worry.

@int19h
Copy link

int19h commented Apr 20, 2022

debugpy does support specifying env vars, but the way we propagate them to the debuggee is different from most other adapter. We used to propagate env vars via the command line when doing runInTerminal; but the resulting command lines were very long and hard to parse visually - which is especially annoying when they end up in history.

So now, instead of that, we tell VSCode to spawn a separate launcher process with the default env, and then pass it the entire "launch" request to it as is via a socket backchannel; from there it reads env vars and sets them up before running the user code in a child process.

I dunno if this makes any difference, though. Well, VSCode could drop the cmd /c part if there's no runInTerminal.env - but do we really want different behavior here depending on the adapter?

@rioj7
Copy link
Author

rioj7 commented Apr 20, 2022

@roblourens

Another possibility is to let the user decide if it wants the sub-shell by adding a new argument to the launch config:

   "subShell": true

and the default value is

   "subShell": false
			if (subShell) {
				command += 'cmd /C "';
			}
			if (env) {
				for (const key in env) {
					let value = env[key];
					if (value === null) {
						command += `set "${key}=" && `;
					} else {
						value = value.replace(/[\^\&\|\<\>]/g, s => `^${s}`);
						command += `set "${key}=${value}" && `;
					}
				}
			}
			for (const a of args) {
				command += `${quote(a)} `;
			}
			if (subShell) {
				command += ' "';
			}
			break;

This way the 4 lines can be kept but the user decides if he wants it.

Very few users have environment variables in the launch config and a few of them may need to create a sub-shell.

Or

  "commandWrap": ["cmd /C ", "\""]

To let the user set the wrapping text.

@rioj7
Copy link
Author

rioj7 commented Apr 20, 2022

@roblourens

It is for bash but the first user with a problem passing regex arguments.

Why is the | character not part of the to-be-quoted characters for bash.

btw. Only \ and ] need to be escaped inside a [] construct in a regex. Technically ] not if it is the first character or the first after ^ if it is a [^] construct.

@roblourens
Copy link
Member

Good find, I'll fix that too.

@rioj7
Copy link
Author

rioj7 commented Apr 21, 2022

@roblourens

line 142 changed from

value = value.replace(/[\^\&\|\<\>]/g, s => `^${s}`);

to

value = value.replace(/[^&|<>]/g, s => `^${s}`);

This results in a completely different regex:

[\^\&\|\<\>] means match one of the following characters ^&|<>

[^&|<>] means match a character that is not any of &|<> ( [^xyz] is a negated character class)

On line 124 it is done correct, ^ is not the first character in the character class definition

line 142 should be:

value = value.replace(/[&^|<>]/g, s => `^${s}`);

@roblourens
Copy link
Member

Damnit... thanks for checking

@boardkeystown
Copy link

I'm so confused what are you talking about?

The official documentation say

"If the debugger extension you are using can run the debug target in VS Code's Integrated Terminal (or an external terminal), you can try to pass the shell redirect syntax (for example, "<" or ">") as arguments."

https://code.visualstudio.com/docs/editor/debugging#_redirect-inputoutput-tofrom-the-debug-target

meaning that

{
    // Use IntelliSense to learn about possible attributes.
    // Hover to view descriptions of existing attributes.
    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
    "version": "0.2.0",
    "configurations": [
        {
            "name": "Python: Current File",
            "type": "python",
            "request": "launch",
            "program": "${file}",
            "console": "integratedTerminal",
            "justMyCode": true,
            "args": ["<","out.txt"]
        }
    ]
}

should do

D:\Python\Foo> d: && cd d:\Python\Foo && cmd /C "C:\Python310\python.exe c:\Users\name.vscode\extensions\ms-python.python-2022.6.0\pythonFiles\lib\python\debugpy\launcher 54849 -- d:\Python\Foo\main.py < out.txt "

Like it had been before this update

Not

D:\Python\Foo> d: && cd d:\Python\Foo && cmd /C "C:\Python310\python.exe c:\Users\name.vscode\extensions\ms-python.python-2022.6.0\pythonFiles\lib\python\debugpy\launcher 54849 -- d:\Python\Foo\main.py ^< out.txt "

Notice the main.py ^>out.txt and it's not main.py > out.txt

If I type python main.py > out.txt in cmd or powershell it works as you would expect. But only now after the 1.67.0 update today 5/6/2022 debug args no long works because it adds that weird escape character.

So how can I get back to being able to redirect standard input from args in the debugger?

@boardkeystown
Copy link

I realized it does use powershell by default for me, do you have any terminal settings set @rioj7?

How do you change the debug terminal? It always defaults to cmd over powershell despite setting me setting the default terminal as power shell

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 debug Debug viewlet, configurations, breakpoints, adapter issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

10 participants