GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix passing escaped double quoted spaces to native executables by douglaswth · Pull Request #2182 · PowerShell/PowerShell

Conversation

@douglaswth

@douglaswth Douglas Thrift (douglaswth) commented Sep 4, 2016

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Sep 4, 2016

Copy link
Copy Markdown

Hi Douglas Thrift (@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;

All reactions

Sorry, something went wrong.

Uh oh!

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

@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-signed label Sep 4, 2016
@TravisEz13 Travis Plunk (TravisEz13) added the Review - Needed label Sep 5, 2016
@vors

sergei (vors) commented Sep 7, 2016

Copy link
Copy Markdown
Collaborator

Thank you, Douglas Thrift (@douglaswth) !
Can you, please, add tests?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vors sergei (vors) self-assigned this Sep 7, 2016
@vors sergei (vors) added Review - Waiting on Author and removed Review - Needed labels Sep 8, 2016
@vors

sergei (vors) commented Sep 9, 2016

Copy link
Copy Markdown
Collaborator

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@rkeithhill

Keith Hill (rkeithhill) commented Sep 9, 2016 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@TravisEz13 Travis Plunk (TravisEz13) added Review - Needed and removed Review - Waiting on Author labels Sep 10, 2016
@douglaswth

Douglas Thrift (douglaswth) commented Sep 12, 2016

Copy link
Copy Markdown
Contributor Author

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

Keith Hill (@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.

All reactions

Sorry, something went wrong.

Uh oh!

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

vors
vors reviewed Sep 12, 2016
View reviewed changes
Comment thread .gitignore Outdated

@vors sergei (vors) Sep 12, 2016

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.

Is it needed? What does create this file?

Sorry, something went wrong.

Uh oh!

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

All reactions

@douglaswth Douglas Thrift (douglaswth) Sep 12, 2016

Copy link
Copy Markdown
Contributor 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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

@vors sergei (vors) Sep 12, 2016

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
@vors

sergei (vors) commented Sep 12, 2016

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@vors sergei (vors) added Review - Waiting on Author and removed Review - Needed labels Sep 12, 2016
@TravisEz13 Travis Plunk (TravisEz13) added Review - Needed and removed Review - Waiting on Author labels Sep 12, 2016
vors
vors reviewed Sep 12, 2016
View reviewed changes
Comment thread .gitignore Outdated

@vors sergei (vors) Sep 12, 2016

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
@vors

sergei (vors) commented Sep 12, 2016

Copy link
Copy Markdown
Collaborator

🕝

All reactions

Sorry, something went wrong.

Uh oh!

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

@vors sergei (vors) added Review - Waiting on Author and removed Review - Needed labels Sep 12, 2016
@TravisEz13 Travis Plunk (TravisEz13) added Review - Needed and removed Review - Waiting on Author labels Sep 12, 2016
vors
vors reviewed Sep 12, 2016
View reviewed changes
Comment thread test/powershell/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 Outdated

@vors sergei (vors) Sep 12, 2016

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 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>'

Sorry, something went wrong.

Uh oh!

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

All reactions

@lzybkr Jason Shirk (lzybkr) Sep 12, 2016

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

@douglaswth Douglas Thrift (douglaswth) Sep 12, 2016

Copy link
Copy Markdown
Contributor 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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

@vors sergei (vors) Sep 12, 2016

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
vors
vors reviewed Sep 12, 2016
View reviewed changes
Comment thread test/powershell/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 Outdated

@vors sergei (vors) Sep 12, 2016

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@douglaswth Douglas Thrift (douglaswth) Sep 12, 2016

Copy link
Copy Markdown
Contributor 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 because it should be sent to EchoArgs as EchoArgs.exe "a\"b c\"d" with the outer quotes.

Sorry, something went wrong.

Uh oh!

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

All reactions

@vors sergei (vors) Sep 12, 2016

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.

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!

Sorry, something went wrong.

Uh oh!

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

All reactions
@vors sergei (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
@vors

sergei (vors) commented Sep 13, 2016

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

Douglas Thrift (douglaswth) added 6 commits September 13, 2016 13:37
@douglaswth
Fix passing escaped double quoted spaces to native executables
8bca1f5
@douglaswth
Add test/csharp/csharp.nuget.props to .gitignore
77a4e96
@douglaswth
Add tests for native command arguments
7816516
@douglaswth
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
@douglaswth
Rename Publish-EchoArgs to Publish-PSTestTools
4d5d0a0
* 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
@douglaswth
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 Travis Plunk (TravisEz13) added Review - Needed and removed Review - Waiting on Author labels Sep 13, 2016
@vors
sergei (vors) merged commit a266f11 into PowerShell:master Sep 15, 2016
@vors

sergei (vors) commented Sep 15, 2016

Copy link
Copy Markdown
Collaborator

Thank you so much for the great contribution!

douglaswth and lopaka reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

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

@douglaswth
Douglas Thrift (douglaswth) deleted the fix-passing-escaped-double-quoted-spaces-to-native.exe branch September 15, 2016 03:21
@vors

sergei (vors) commented Sep 15, 2016

Copy link
Copy Markdown
Collaborator

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

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

cc Andy Jordan (@andschwa)

All reactions

Sorry, something went wrong.

Uh oh!

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

@andyleejordan

Andy Jordan (andyleejordan) commented Sep 15, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@andyleejordan Andy Jordan (andyleejordan) mentioned this pull request Sep 19, 2016
Arguments for external executables aren't correctly escaped #1995
Closed
@be5invis

Belleve (be5invis) commented Sep 20, 2016

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@coderabbitai coderabbitai Bot mentioned this pull request Aug 3, 2026
fix(sdd): emit PowerShell-safe UserPromptSubmit hook on Windows Gentleman-Programming/gentle-ai#2342
Closed
9 of 10 tasks
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@vors
Merge pull request PowerShell#2182 from douglaswth/fix-passing-escape…
1baafec
…d-double-quoted-spaces-to-native.exe

Fix passing escaped double quoted spaces to native executables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

@vors sergei (vors)

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.

8 participants

@douglaswth @msftclas @vors @rkeithhill @andyleejordan @be5invis @lzybkr @TravisEz13
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.