-
Notifications
You must be signed in to change notification settings - Fork 8.1k
WIP: Try to manage quotes in strings when calling native executables #13482
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
WIP: Try to manage quotes in strings when calling native executables #13482
Conversation
|
It would be nice if the nullOrEmpty check on the arg is removed too. Almost every platform has a concept of an empty string appearing in argv. The whole only-quote-when-unquoted still feels wonky to reason about, but ehhhh the test cases are there. (FWIW, I would really prefer forgoing the wonky code for "need quote" altogether.) |
|
First of all, thanks for working on this long overdue issue. And thanks for showing the (incomplete?) code in a work-in-progress pull request.
I think this is a very dangerous half-hearted measure. I'd honestly prefer to not change the current behavior, than to further modify the "guess what the user wanted and try to do something maybe that could be right" method. Implementing it this way would still not allow to pass arbitrary sting contents to executables. But instead of knowing that I have to correctly escape This would mean changing from one inconsistent behavior to another more complicated inconsistent behavior... Another disadvantage: The current behavior is at least relatively obvious. It is at least somewhat likely that a user would test if a string containing a quote is passed correctly. The proposed behavior would would mean, that people would only notice the problem if they explicitly test if a backslash followed by a quote is passed correctly... Note: The correct way to escape N backslashes followed by |
|
Looks like this is triggering the same failures as in #13099. The Pester stuff really is a goldmine for strange hacks relating to this. |
Yes, I would again strongly recommend, to not do these strange tests - that always means, that one can not pass arbitrary content as an argument to an executable. Please always treat any quotes within an argument as literal quotes that need to be escaped. |
|
@TSlivede the intent of not modifying an already escaped quote |
|
Well, since this is already a breaking change, I think ensuring this sort of thing is unnecessary. |
Yes, I already guessed that - but doing it this way does not solve the problem in general. It just makes the problem harder to notice. This is not a solution - this is hiding the problem. I am fully aware that many people would be happy, that quotes would then "just work". So there would be less complaints to powershell. But it does not change the fact, that argument passing to external executables would be still fundamentally flawed. |
|
I wholeheartedly agree, @TSlivede and @Artoria2e5. The only worthwhile fix is one that fixes the problem once and for all, even though that means breaking existing workarounds. To recap, that means:
|
I agree. Especially as I assume, that there are more cases that would brake anyway and only a few cases that would additionally break if this would be done correctly. I assume, that there are more calls of the form |
I have to admit, that that I'm really not a fan of checking which executable is called and then changing the quoting. That is absolutely horrible to debug if someone has a problem. If there must be accommodations for msiexec style CLIs, then arguments should simply always be quoted as And I'm not even sure, that we should implement such accommodations: It would be much much better if starting with the next update, Microsoft would make their executables accept arguments wich are quoted from beginning to end (e.g. If powershell implements special handling for these parameters, then these programs will probably never be fixed and any other cross platform tools will forever be unable to properly call these programs... |
Neither am I, but we would do it as a courtesy to shield users from the underlying command-line anarchy, which seems well worth doing given the high profile of And, of course, in order to accommodate batch files - which as CLI entry points are probably often used without the user realizing that a batch file is involved - there's no other option than to check whether the (resolved) executable path ends in Also note that the
(a) The accommodations are easy to conceptualize and document - for which the proposed and sorely needed (b) We should ship something akin to
That would mean engaging with and indulging the command-line anarchy, whereas we want to abstract it away - for the remaining cases, if any, where that doesn't work (to be diagnosed with PowerShell is a shell, which means that however I pass an argument that I want the target executable to see as verbatim E.g.,
Absolutely - these CLIs should have been fixed a long time ago, incidentally not just with respect to By contrast, the accommodations for batch files can never go away (without massively breaking backward compatibility). But even fixing the CLIs will not help older systems that may, however, install a fixed PowerShell version.
Again - I don't see how any of these accommodations could break anything - do tell us if you know of any cases. (Hypothetically, breakage is possible in the following scenario, but I do not think it is a real-world concern: A |
DetailsMaybe I didn't express clear enough, what I wanted to say.
Ok, for batch files I can accept that - its easily documentable and its not some magic list of executables.
Yes, that was my main point - just always quote a string of the form
I agree, that such a help topic is absolutely necessary. But adding an additional section to it, that explains that some executables are treated differently doesn't exactly make the topic easier to understand...
Agreed. But such a
No, of course
No, we should absolutely not do that! That's not what I meant. What I meant was: Always construct the lpCommandLine with arguments quoted in a way that is understood by msiexec, etc. but also (and this is of course the most important aspect) always parsed correctly by executables that follow the common rules. If I write in powershell it should result in the lpCommandLine
No, that can't break anything. My point is: If powershell doesn't implement any accommodations, microsoft would have more pressure to finally fix msiexec. |
This comment has been minimized.
This comment has been minimized.
|
Thanks, @TSlivede - I don't think there's any disagreement in substance here. I am not advocating for a hard-coded list of "rogue" CLIs (I had considered that earlier, but decided against it), I am indeed recommending that any verbatim argument seen by PowerShell on Windows that looks like Of course, this is an accommodation, but a highly beneficial one. As for the other, closely related accommodation I proposed, namely to switch to That
Given the above, the only scenarios in which Therefore, we either need a separate |
This can be trivially done by just escaping the two pieces separately. My PR has a TODO on that while I decide on exactly which separators I am going to need to accommodate: anything more than |
This comment has been minimized.
This comment has been minimized.
|
In my implementation I also use |
|
Good point about paths, @TSlivede (as an aside: very nice, concise implementation; The question is: does it cover all high-profile cases? We should strive for a pattern that is easy to conceptualize and document (yours qualifies), so I suggest we avoid exceptions for single-letter keys, if possible. |
|
Below is a proof-of-concept written in PowerShell that implements all accommodations: Note:
Running the code, which includes Pester tests, yields the following, which shows how the verbatim input arguments must be transformed to form part of the command-line string that is passed to Code: # WINDOWS ONLY
# Sample arguments, as seen verbatim by PowerShell before constructing
# the command line.
# Note: The first, empty line represents an empty argument ('' / "").
$sampleArgs = @'
foo=bar none
/foo:bar none
-foo:bar none
foo=bar "quote" none
foo:not me
foo=notme
notmeeither
c:\program files\
a "b c" d
a \"b c\" d
3" of snow
ab\
a b\\
a&b
a"b
'@ -split '\r?\n'
# Non-batch files: expected quoted forms:
# * \"-escaping
# * double-quoting only for space-less arguments.
# * msiexec-style partial argument double-quoting
$expectedQuoted = @'
""
foo="bar none"
/foo:"bar none"
-foo:"bar none"
foo="bar \"quote\" none"
"foo:not me"
foo=notme
notmeeither
"c:\program files\\"
"a \"b c\" d"
"a \\\"b c\\\" d"
"3\" of snow"
ab\
"a b\\\\"
a&b
a\"b
'@ -split '\r?\n'
# Batch files: expected quoted forms:
# * ""-escaping instead of \"-escaping
# * double-qouting also for space-less args with cmd.exe metachars.
# * msiexec-style partial argument double-quoting
$expectedQuotedForBatchFile = @'
""
foo="bar none"
/foo:"bar none"
-foo:"bar none"
foo="bar ""quote"" none"
"foo:not me"
foo=notme
notmeeither
"c:\program files\\"
"a ""b c"" d"
"a \\""b c\\"" d"
"3"" of snow"
ab\
"a b\\\\"
"a&b"
"a""b"
'@ -split '\r?\n'
$false, $true | ForEach-Object {
$callingABatchFile = $_
"--- $($callingABatchFile ? 'C' : 'NOT c')alling a batch file: " | Write-Host -ForegroundColor Green
# Quote the arguments for use as part of the raw command line.
$quotedArgs = $sampleArgs | ForEach-Object {
if ('' -eq $_) { return '""' } # Empty argument, always represented as ""
# Analyze the argument.
$mustDQuote = $_.Contains(' ') -or ($callingABatchFile -and $_ -match '["&|<>^,;]')
$mustPartiallyDQuote = $mustDQuote -and $_ -match '^([/-]\w+[=:]|\w+=)(.+)$'
$escapedDQuote = $callingABatchFile ? '""' : '\"'
if ($mustPartiallyDQuote) {
$prefix, $quotedArg = $Matches[1], $Matches[2]
}
else {
$prefix = ''
$quotedArg = $_
}
# Escape any embedded " first, including doubling \ instances before them.
$quotedArg = $quotedArg -replace '\\+(?=")', '$&$&' -replace '"', $escapedDQuote
# If double-quoting must be used, trailing '\'s must be doubled.
if ($mustDQuote) { $quotedArg = $quotedArg -replace '\\+$', '$&$&' }
# Apply double-quoting, if necessary, and output:
$prefix + ($mustDQuote ? ('"{0}"' -f $quotedArg) : $quotedArg)
}
$expected = ($callingABatchFile ? $expectedQuotedForBatchFile : $expectedQuoted)
# Output to the host to see the transformations.
0..($sampleArgs.Count-1) | ForEach-Object {
[pscustomobject] @{ Verbatim = $sampleArgs[$_]; Quoted = $quotedArgs[$_]; 'ExpectedIfDifferent' = if ($quotedArgs[$_] -ne $expected[$_]) { $expected[$_]} }
} | Out-Host
# Run the Pester test.
try {
$quotedArgs | Should -BeExactly $expected
} catch {
$_ | Write-Error # Write error and continue.
}
} |
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.
Empty (but non-null, of course) arguments are totally legitimate for command lines, so IMO there's no need of filtering them out.
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
35c18dd to
723f53f
Compare
|
i'm going to close this and take a different tack. Some things have changed underneath this which need to be addressed. |
PR Summary
This PR attempts to manage the scenario where multiple quotes and strings with spaces are sent to the Native Command Processor. The current behavior of our native executable has the following results:
which is somewhat surprising. This PR changes the behavior to:
somewhat more pathological is the following:
with my change:
This PR represents a breaking change
PR Context
see issue 1995
The implementation is somewhat straightforward, it inspects the string argument looking for
"and prepends the native escape\character to the". If there is already a\in front of the"no change is made.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.