GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — properly escape trailing backslash so that it doesn't end up escaping the quotes by SteveL-MSFT · Pull Request #4965 · PowerShell/PowerShell

Conversation

Steve Lee (SteveL-MSFT) commented Oct 1, 2017

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [feature]

4cad7fa

escape last backslash before adding quote

Steve Lee (SteveL-MSFT) assigned Ilya (iSazonov)

Oct 1, 2017

Steve Lee (SteveL-MSFT) requested a review from Jason Shirk (lzybkr)

October 1, 2017 05:50

Steve Lee (SteveL-MSFT) requested review from Bruce Payette (BrucePay), Dongbo Wang (daxian-dbw) and sergei (vors) as code owners

October 1, 2017 05:50

PetSerAl commented Oct 1, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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 \.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) reviewed

Oct 1, 2017

View reviewed changes

Comment thread

test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 Outdated

		}

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

Ilya (iSazonov) Oct 1, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Oct 1, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will add test

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) commented Oct 1, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          [feature]

d309521

modify test to also use double quotes

PetSerAl commented Oct 1, 2017

Copy link

Copy Markdown

Contributor

How about multiple trailing \?:

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Michael Klement (mklement0) mentioned this pull request

Oct 1, 2017

Arguments for external executables aren't correctly escaped #1995

Closed

Steve Lee (SteveL-MSFT) commented Oct 1, 2017

Copy link

Copy Markdown

Member Author

PetSerAl I'll work on that and add a test

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          [feature]

bc01df6

support multiple trailing backslashes

Steve Lee (SteveL-MSFT) commented Oct 2, 2017

Copy link

Copy Markdown

Member Author

Apparently these strange trailing backslash and double quotes rules are documented

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

PetSerAl reviewed

Oct 2, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/engine/NativeCommandParameterBinder.cs

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

PetSerAl Oct 2, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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('"');

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Oct 2, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [feature]

0deab6a

address PR feedback

Ilya (iSazonov) reviewed

Oct 3, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/engine/NativeCommandParameterBinder.cs

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

Ilya (iSazonov) approved these changes

Oct 3, 2017

View reviewed changes

Ilya (iSazonov) commented Oct 6, 2017

Copy link

Copy Markdown

Collaborator

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) approved these changes

Oct 10, 2017

View reviewed changes

Andrew (anmenaga) approved these changes

Oct 10, 2017

View reviewed changes

Andrew (anmenaga) left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) merged commit 59311d0 into PowerShell:master

Oct 11, 2017

Steve Lee (SteveL-MSFT) deleted the trailing-slash branch

October 26, 2018 21:34

Roy Ivy III (rivy) mentioned this pull request

Oct 5, 2021

Use backslash instead of forward slash u-tools/msls#10

Open

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Properly escape trailing backslash so that it doesn't end up escaping…

94888e5

… the quotes (PowerShell#4965)

The native command receives the arg ".\test 1" 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.

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw approved these changes

Ilya (iSazonov)

iSazonov approved these changes

Jason Shirk (lzybkr)

Awaiting requested review from lzybkr

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

sergei (vors)

Awaiting requested review from vors

+2 more reviewers

PetSerAl

PetSerAl left review comments

Andrew (anmenaga)

anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Ilya (iSazonov)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Steve Lee (SteveL-MSFT) commented Oct 1, 2017

Uh oh!

PetSerAl commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 1, 2017

Uh oh!

PetSerAl commented Oct 1, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 1, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 2, 2017

Uh oh!

PetSerAl Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetSerAl Oct 3, 2017

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 3, 2017

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Oct 6, 2017

Uh oh!

Ilya (iSazonov) commented Oct 10, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 10, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Oct 10, 2017

Uh oh!

Andrew (anmenaga) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PetSerAl commented Oct 1, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) Oct 3, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.