Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

properly escape trailing backslash so that it doesn't end up escaping the quotes#4965

Merged
iSazonov merged 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
SteveL-MSFT:trailing-slashSteveL-MSFT/PowerShell:trailing-slashCopy head branch name to clipboard
Oct 11, 2017
Merged

properly escape trailing backslash so that it doesn't end up escaping the quotes#4965
iSazonov merged 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
SteveL-MSFT:trailing-slashSteveL-MSFT/PowerShell:trailing-slashCopy head branch name to clipboard

Conversation

@SteveL-MSFT

Copy link
Copy Markdown
Member

When we create the arg string, an arg of .\test 1\ ends up being ".\test 1\" which is stored in StartInfo.Arguments which then corefx passes as-is to SHELLEXECUTEINFO.

The native command receives the arg as .\test 1" as the last \" is treated as escaping the quotes. Fix is to add an extra backslash to escape the last enclosing quote.

Fix #4358

escape last backslash before adding quote
@PetSerAl

PetSerAl commented Oct 1, 2017

Copy link
Copy Markdown
Contributor

Since not every application use \ as escapement it will also affect behavior of that programs. For example:

cmd /c 'echo \'

will print \\ instead of \.

Also you should not only double one \ at the end, but whole sequence of \ in the end. For example:

cmd /c 'echo \\\'

will print \\\\, while it should be \\\\\\ to be properly recognized as three literal \.

}

It "Should correctly quote paths with spaces" {
$lines = testexe -echoargs '.\test 1\' '.\test 2\'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check double quotes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add test

@SteveL-MSFT

Copy link
Copy Markdown
Member Author

@PetSerAl for cmd echo, perhaps the only thing we can do is suggest using cmd --% /c echo \

modify test to also use double quotes
@PetSerAl

PetSerAl commented Oct 1, 2017

Copy link
Copy Markdown
Contributor

How about multiple trailing \?:

testexe -echoargs '.\test 1\\' ".\test 2\\"

@SteveL-MSFT

Copy link
Copy Markdown
Member Author

@PetSerAl I'll work on that and add a test

support multiple trailing backslashes
@SteveL-MSFT

Copy link
Copy Markdown
Member Author

Apparently these strange trailing backslash and double quotes rules are documented

@@ -196,16 +196,28 @@ private void appendOneNativeArgument(ExecutionContext context, object obj, char
if (NeedQuotes(arg))
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

_arguments.Append('"'); 
_arguments.Append(arg); 
for (int i = arg.Length-1; i >= 0 && arg[i] == '\\'; i--)
{
    _arguments.Append('\\');
}
_arguments.Append('"'); 

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's much better than my attempt. Will change.

address PR feedback
for (int i = arg.Length-1; i >= 0 && arg[i] == '\\'; i--)
{
_arguments.Append(arg);
_arguments.Append('\\');

@iSazonov iSazonov Oct 3, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be "\\\\"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, unless you want to triplicate trailing \. arg already have one copy of \. This just adding second copy on top of it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarify!

@iSazonov

iSazonov commented Oct 6, 2017

Copy link
Copy Markdown
Collaborator

@daxian-dbw @lzybkr Could you please review?

1 similar comment
@iSazonov

Copy link
Copy Markdown
Collaborator

@daxian-dbw @lzybkr Could you please review?

@SteveL-MSFT SteveL-MSFT requested a review from anmenaga October 10, 2017 16:09
@SteveL-MSFT

Copy link
Copy Markdown
Member Author

@daxian-dbw is in training today, @anmenaga can you review?

@daxian-dbw

Copy link
Copy Markdown
Member

Sorry that I missed the the first "@mention" notification. I will take a look later today.
I wish @mklement0 can review this if possible, as he has a better understanding of the native argument parsing user experience in PowerShell.

@anmenaga anmenaga left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iSazonov iSazonov merged commit 59311d0 into PowerShell:master Oct 11, 2017
@SteveL-MSFT SteveL-MSFT deleted the trailing-slash branch October 26, 2018 21:34
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 this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.