Fix passing escaped double quoted spaces to native executables #2182

douglaswth · 2016-09-04T04:35:21Z

This fixes a regression that seems to have been introduced in PowerShell 5 where spaces in segments of a string with an even number of quotation marks even though some may be escaped are ignored for quoting when executing native programs.

I discovered this issue when trying to run a utility that needs to have double quotes to delimit strings in its query language:

rsc json --x1 "object:has(.name:val(\`"rs low space on C: drive\`"))"

With PowerShell versions before 5, this worked correctly and the query argument was accepted, but with PowerShell 5 the argument passed was getting through to the utility broken up: object:has(.name:val("rs, low, space, on, C:, drive")).

Out of curiosity, I tried it out with the latest PowerShell 6 alpha and the same issue still exists, even on Linux! So I have come up with this fix which is to not count backslash escaped quotes in the quote count for the appendOneNativeArgument in NativeCommandParameterBinder.

msftclas · 2016-09-04T04:35:25Z

Hi @douglaswth, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

vors · 2016-09-07T18:43:47Z

Thank you, @douglaswth !
Can you, please, add tests?

vors · 2016-09-09T20:14:50Z

To properly test it, we would need a cross-platform equivalent of echoargs. It's a simple program that just returns the arguments that been passed.

We can implement it as dotnet application, similar to ResGen that we use in the build process.

rkeithhill · 2016-09-09T23:01:45Z

It's a very simple command. Feel free to use this - if you want: https://github.com/Pscx/Pscx/blob/master/Src/EchoArgs/EchoArgs.cs

Should compile on .NET Core.

douglaswth · 2016-09-12T05:24:35Z

@vors I have added tests that use EchoArgs via dotnet.

@rkeithhill EchoArgs didn't quite compile on .NET Core due to the use of Environment.CommandLine, but that part was not necessary for my test so I have just left it commented out in the import.

vors · 2016-09-12T05:33:37Z

.gitignore

Is it needed? What does create this file?

I'm not sure, but it was showing up at least on my Linux box.

Perhaps, we should do a wild-card, like *.nuget.props

vors · 2016-09-12T05:39:20Z

Wow, that was fast! Great job! Let's make it vigorous.
🕐

vors · 2016-09-12T17:20:43Z

.gitignore

Sorry for hustle, I know that I suggested this name run yesterday. I didn't find a good one at the moment, but now I think we should use bin instead of run, because it would match dotnet output.

vors · 2016-09-12T17:24:31Z

🕝

vors · 2016-09-12T18:09:15Z

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

Should Contain usage here makes it's harder to troubleshoot the error. When it accured.
Here, I'm trying to execute the test on the inbox powershell v5.1

Describing Native Command Arguments [+] Should handle quoted spaces correctly 619ms [-] Should handle spaces between escaped quotes 127ms Expected: file TestDrive:\test.txt to contain {Arg 0 is <a"b c"d>} 19: $testPath | Should Contain 'Arg 0 is <a"b c"d>' at <ScriptBlock>, F:\dev\PowerShell\test\powershell\Scripting\NativeExecution\NativeCommandArguments.Tests.ps1: line 19

As you see, I don't have information about the produced outpuit immediately in my disposal.
Just to find what we are currently produce I would need to run commands by hands.

I suggest to replace $testPath | Should Contain 'Arg 0 is <ab cd>' by

$lines = Get-Contetxt $testPath ($lines | measure).Count | Should Be 3 # this line will help get a better error message in case there is only one line in the output $lines[0] | Should Be 'Arg 0 is <ab cd>'

Why bother writing the file? You can just capture the output in a variable with:

$lines = & $echoArgs $a 'a"b c"d' a"b c"d

I was trying to do what I thought would be reasonable, but it didn't seem to work with Pester:

& $echoArgs $a 'a"b c"d' a"b c"d | Should Match @" Arg 0 is <ab cd> Arg 1 is <ab cd> Arg 2 is <ab cd> "@

It seems like Pester only tries to match the first line of output when using Should Match. 😕

It's because powershell take output line by line.
You can add | Out-String | to get a single string.
Mulit-line probably indeed appropriate here, but I'd like to bring up couple things:

please, use Be, not Match, because Match takes regex.

Multi-line output will likely cause problems on cross-platforms, because of line-endings.

I suggest use what I described above, with array of 3 strings. It avoids problems with line-ends and produce a reasonable good error message.

vors · 2016-09-12T18:19:47Z

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

I'd like to understand this better.

"a\`"b c\`"d"

There is a mix of two things:
backticks as powershell string escape mechanics and \ as command line args escape mechanics

This is an equivalent of 'a\"b c\"d'
When I use ehcoargs from cmd:

EchoArgs.exe a\"b c\"d

I'm getting

Arg 0 is <a"b>
Arg 1 is <c"d>

So the behavior that is currently described with Arg 1 doesn't seem right.

That's because it should be sent to EchoArgs as EchoArgs.exe "a\"b c\"d" with the outer quotes.

I see. I'd like to dig this a little bit more on all platforms, just to make sure we are doing the right thing.
At the meantime, can I ask you to add comments in the tests, so it's easier to maintain them?
Particularly the line

"a\`"b c\`"d"

already requires knowledge about both PS and native args passing escaping.

Will come back to this today after some experiments.
Thank you, again, for such thoughtful approach to the PR!

vors · 2016-09-13T01:04:59Z

I verified this behavior is aligned with bash and cmd.
We will merge it when you address comments about the tests.

* Move EchoArgs from test/EchoArgs to test/tools/EchoArgs * Use "dotnet publish" for building EchoArgs in build.psm1 so the test can call it directly

* Rename Publish-EchoArgs to Publish-PSTestTools so it can be used for other tools as well in the future * Publish EchoArgs to the bin directory instead of run to match convention * Add source URL to EchoArgs header comment * Use wildcard of "*.nuget.props" to match "test/csharp/csharp.nuget.props" in .gitignore

* Use a variable (which can be indexed by line) instead of a file to test the output of arguments passed to echoargs * Add comments explaining the tests

vors · 2016-09-15T02:08:18Z

Thank you so much for the great contribution!

vors · 2016-09-15T16:45:19Z

I just realised where this *.nuget.props file came from:

log  : Generating MSBuild file .../test.nuget.props.

cc @andschwa

andyleejordan · 2016-09-15T18:49:20Z

Ah, looks like the CLI team is making some serious progress!

be5invis · 2016-09-20T02:35:21Z

We still need to add a backslash before a backtick-escaped double quote? This does not solve the double-escaping problem. (That is, we have to escape a double quote for both PowerShell and CommandLineToArgvW.)

msftclas added the cla-signed label Sep 4, 2016

TravisEz13 added the Review - Needed label Sep 5, 2016

vors self-assigned this Sep 7, 2016

vors added Review - Waiting on Author and removed Review - Needed labels Sep 8, 2016

TravisEz13 added Review - Needed and removed Review - Waiting on Author labels Sep 10, 2016

vors reviewed Sep 12, 2016
View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed labels Sep 12, 2016

TravisEz13 added Review - Needed and removed Review - Waiting on Author labels Sep 12, 2016

vors reviewed Sep 12, 2016
View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed labels Sep 12, 2016

TravisEz13 added Review - Needed and removed Review - Waiting on Author labels Sep 12, 2016

vors reviewed Sep 12, 2016
View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed labels Sep 12, 2016

This was referenced Sep 12, 2016

Added tests for hashtabletoPSCustomObjectConversion, OutErrorVairable… #2160

Merged

Make ConvertTo-Html work on CoreCLR #2241

Merged

douglaswth added 6 commits September 13, 2016 13:37

Fix passing escaped double quoted spaces to native executables

8bca1f5

Add test/csharp/csharp.nuget.props to .gitignore

77a4e96

Add tests for native command arguments

7816516

Move and "publish" EchoArgs for the tests

7e80702

* Move EchoArgs from test/EchoArgs to test/tools/EchoArgs * Use "dotnet publish" for building EchoArgs in build.psm1 so the test can call it directly

Use a variable instead of a file for testing args

ddf30c4

* Use a variable (which can be indexed by line) instead of a file to test the output of arguments passed to echoargs * Add comments explaining the tests

TravisEz13 added Review - Needed and removed Review - Waiting on Author labels Sep 13, 2016

vors merged commit a266f11 into PowerShell:master Sep 15, 2016

douglaswth deleted the fix-passing-escaped-double-quoted-spaces-to-native.exe branch September 15, 2016 03:21

andyleejordan mentioned this pull request Sep 19, 2016

Arguments for external executables aren't correctly escaped #1995

Closed

Fix passing escaped double quoted spaces to native executables #2182

Fix passing escaped double quoted spaces to native executables #2182

Uh oh!

Conversation

douglaswth commented Sep 4, 2016

Uh oh!

msftclas commented Sep 4, 2016

Uh oh!

vors commented Sep 7, 2016

Uh oh!

vors commented Sep 9, 2016

Uh oh!

rkeithhill commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglaswth commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors commented Sep 13, 2016

Uh oh!

vors commented Sep 15, 2016

Uh oh!

vors commented Sep 15, 2016

Uh oh!

andyleejordan commented Sep 15, 2016

Uh oh!

be5invis commented Sep 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rkeithhill commented Sep 9, 2016 •

edited

Loading