Skip to content
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

Parameter invalid when it contains a colon. Works prior to 7.3.0. #18554

Closed
5 tasks done
rmegal opened this issue Nov 14, 2022 · 28 comments · Fixed by #18559
Closed
5 tasks done

Parameter invalid when it contains a colon. Works prior to 7.3.0. #18554

rmegal opened this issue Nov 14, 2022 · 28 comments · Fixed by #18559
Assignees
Labels
Resolution-Fixed The issue is fixed.

Comments

@rmegal
Copy link

rmegal commented Nov 14, 2022

Prerequisites

Steps to reproduce

Enter:

$test = "&sqlcmd -v MDF_FILEPATH=```"C:\Databases\junk.mdf```""
invoke-expression $test

Expected behavior

sqlcmd is launched and you can enter:

select '$(MDF_FILEPATH)'
go

And be presented with:

----------------------------
C:\Databases\junk.mdf

Actual behavior

sqlcmd is no longer able to parse parameter value and you get the following after the invoke-expression:

Sqlcmd: 'MDF_FILEPATH=\"C:\Databases\junk.mdf\""': Invalid argument. Enter '-?' for help.

Error details

n/a

Environment data

Name                           Value
----                           -----
PSVersion                      7.3.0
PSEdition                      Core
GitCommitId                    7.3.0
OS                             Microsoft Windows 10.0.22621
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Visuals

PowerShell_7 3

PowerShell_prior_to_7 3

@rmegal rmegal added the Needs-Triage The issue is new and needs to be triaged by a work group. label Nov 14, 2022
@SteveL-MSFT
Copy link
Member

Use:

 $PSNativeCommandArgumentPassing = 'Legacy'

for now as a workaround. We probably need to add sqlcmd to the legacy list. cc @JamesWTruher

@SteveL-MSFT
Copy link
Member

@rmegal if you can confirm using Legacy mode works for you, I'll update the list to include sqlcmd

@rmegal
Copy link
Author

rmegal commented Nov 14, 2022

Using $PSNativeCommandArgumentPassing = 'Legacy' works for sqlcmd.

I will use Legacy for the scripts I have that use sqlcmd directly. Will I need to keep doing this once sqlcmd is in the legacy list? Not sure how the "legacy list" works.

@SteveL-MSFT
Copy link
Member

Once we have the list updated and backported to 7.3.x, the idea is that you can keep $PSNativeCommandArgumentPassing = 'Windows' (which is the default) and commands in the list automatically use legacy when needed for compat. So you shouldn't have to make any changes. However, if you uset legacy at the top of your script, then the changes we're making should still not break you and you can keep it like that as well.

@ghost ghost added the In-PR Indicates that a PR is out for the issue label Nov 14, 2022
@SteveL-MSFT SteveL-MSFT self-assigned this Nov 14, 2022
@JamesWTruher
Copy link
Member

it is also likely that the triple back-tick is no longer needed (a single might do the trick with this new behavior)

@mklement0
Copy link
Contributor

mklement0 commented Nov 14, 2022

@rmegal, note that there's generally no reason to use Invoke-Expression for invoking external programs.

The problem boils down to the following call, which is a workaround that builds on the fundamentally broken handling of embedded " chars. in external-program calls up to v7.2 - that's why it stopped working in v7.3:

& sqlcmd -v MDF_FILEPATH=`"C:\Databases\junk.mdf`"

In your particular case, given that value C:\Databases\junk.mdf contains no spaces (or other metacharacters), no workaround is actually needed:

& sqlcmd -v MDF_FILEPATH=C:\Databases\junk.mdf

However, if the value part - to the right of = - indeed needs selective quoting in sqlcmd (I don't know), a workaround is indeed needed for values that do contain spaces or other metacharacters.

A workaround is then needed, because - behind the scenes, justifiably - PowerShell passes something like
MDF_FILEPATH="C:\My Databases\junk.mdf" as "MDF_FILEPATH=C:\My Databases\junk.mdf", i.e. double-quoted as a whole. External programs should treat these forms as interchangeable, but, regrettably, owing to the anarchy that is argument-passing on Window, some do not.

However, a workaround like MDF_FILEPATH=`"C:\Databases\junk.mdf`" should never have worked, and only works if the broken legacy behavior is in place - hence the need for $PSNativeCommandArgumentPassing = 'Legacy'

A built-in, program-agnostic workaround that would make (non-workaround) calls simply work as-is going forward, was proposed in #15143, but, unfortunately, not implemented.

The upshot is:

  • An eternal game of catch-up will be played of putting specific external programs on the "legacy list" that the default $PSNativeCommandArgumentPassing value of Windows then implicitly treats as if $PSNativeCommandArgumentPassing = 'Legacy' were in effect, which guarantees confusion as to which specific PowerShell version has which external programs on that list and whether a workaround is needed or not.

  • Given that the legacy-mode workaround builds on broken legacy behavior, future generations of PowerShell users will be saddled with having to know the specifics of this broken behavior.

@rmegal
Copy link
Author

rmegal commented Nov 15, 2022

@rmegal, note that there's generally no reason to use Invoke-Expression for invoking external programs.

@mklement0 Right. I could have reproduced the problem without using Invoke-Expresstion. The original script uses it because the command is dynamically built; there are a bunch of different parameters that might get used.

The -v parameter accepts [var = value]. I only included the one that was failing. You are also correct that the quotes are not needed when there are no spaces in the value.

A workaround is then needed, because - behind the scenes, justifiably - PowerShell passes something like MDF_FILEPATH="C:\My Databases\junk.mdf" as "MDF_FILEPATH=C:\My Databases\junk.mdf", i.e. double-quoted as a whole. External programs should treat these forms as interchangeable, but, regrettably, owing to the anarchy that is argument-passing on Window, some do not.

Thanks for the nice explanation. I remember building that script years ago and wondering why including the back ticks made it work. Regardless, your point is, if I'm understanding correctly, sqlcmd will need the 'Legacy' support unless it is changed to accept "MDF_FILEPATH=C:\My Databases\junk.mdf"?

I tried entering sqlcmd -v MDF_FILEPATH=c:\test\junk.mdf and got:
image

Is there something I'm missing regarding passing a colon in the value? Note: This is the same behavior in the cmd shell. I have to wrap the value in quotes to get it to work there too. I'm sure the problem lies in how sqlcmd parses the value containing a colon, which forces the user to wrap the value in quotes.

@jborean93
Copy link
Collaborator

jborean93 commented Nov 15, 2022

The original script uses it because the command is dynamically built; there are a bunch of different parameters that might get used.

You can still do that without Invoke-Expression

$sqlCmdArgs = @(
    'arg that is always set'
    if ($someCondition) { 'args for sqlcml' }
    'arg that is always set'
)
sqlcmd @sqlCmdArgs 

You can also splat multiple arrays or use args inline with splats so you aren't restricted to just this. Using Invoke-Expression is dangerous if the data comes from a user source and adds yet another layer you need to deal with when it comes to strings. It is best to try and avoid it in as many cases as you can.

Is there something I'm missing regarding passing a colon in the value? Note: This is the same behavior in the cmd shell. I have to wrap the value in quotes to get it to work there too. I'm sure the problem lies in how sqlcmd parses the value containing a colon, which forces the user to wrap the value in quotes.

You need to quote the argument to avoid pwsh parameter binder from reading it differently

sqlcmd -v 'MDF_FILEPATH=c:\test\junk.mdf'

@mklement0
Copy link
Contributor

mklement0 commented Nov 15, 2022

@jborean93 , good point about avoiding Invoke-Expression, but quoting MDF_FILEPATH=c:\test\junk.mdf doesn't make a difference, because the problem is on the sqlcmd side.

@rmegal:

if I'm understanding correctly, sqlcmd will need the 'Legacy' support unless it is changed to accept "MDF_FILEPATH=C:\My Databases\junk.mdf"?

In principle, yes, but there seems to be another wrinkle:

PowerShell - again, justifiably - never uses double quotes behind the scenes if an argument doesn't contain spaces, even if in the original PowerShell command line quotes were used; that is, 'MDF_FILEPATH=c:\test\junk.mdf' "MDF_FILEPATH=c:\test\junk.mdf" and MDF_FILEPATH=c:\test\junk.mdf all end up as unquoted MDF_FILEPATH=c:\test\junk.mdf on the process command line.

It seems that sqlcmd.exe (which I'm not familiar with) breaks an unqoted value that contains : by that character, which is what your error message suggests (the docs don't seem to mention that).

If that is the case, then truly only partial quoting helps; that is, the following must be placed on the process command line: MDF_FILEPATH="c:\test\junk.mdf", which you can only do in one of the following ways:

  • with your original legacy workaround that builds on the broken behavior, which now requires you to (temporarily) set $PSNativeCommandArgumentPassing to 'Legacy'

  • with --%, which comes with pitfalls and fundamental limitations, however

    • sqlcmd -v --% MDF_FILEPATH="c:\test\junk.mdf"
  • by calling via cmd /c

    • cmd /c 'sqlcmd -v MDF_FILEPATH="c:\test\junk.mdf"'

P.S.: I've written up a generalization of the above with additional background information in this Stack Overflow answer (albeit without discussing the specific sqlcmd challenges).

@ghost ghost added Resolution-Fixed The issue is fixed. and removed In-PR Indicates that a PR is out for the issue Needs-Triage The issue is new and needs to be triaged by a work group. labels Nov 15, 2022
@daxian-dbw
Copy link
Member

@rmegal Can you please set $PSNativeCommandArgumentPassing = 'Windows' in PS 7.3.0, and then run the following and see if it works?

$test = "&sqlcmd -v MDF_FILEPATH=`"C:\Databases\junk.mdf`""
invoke-expression $test

In your original repro, the extra two backticks surrounding C:\Databases\junk.mdf basically constructs a string like this:

&sqlcmd -v MDF_FILEPATH=`"C:\Databases\junk.mdf`"

When running with Invoke-Expression $test, the `" was previous ignored by PowerShell (Legacy mode) when passing args to native command, so the args passed in was actually

Arg 0 is <-v>
Arg 1 is <MDF_FILEPATH=C:\Databases\junk.mdf>

In 7.3.0, the `" is now recognized (Windows mode, which is the default mode on Windows) and the args passed to sqlcmd becomes the following, and causes sqlcmd to fail.

Arg 0 is <-v>
Arg 1 is <MDF_FILEPATH="C:\Databases\junk.mdf">

With the above explanation, you should be able to make your script work again by just removing the extra two backticks when constructing $test. Please let me know if that works. Thanks

@mklement0
Copy link
Contributor

@daxian-dbw, it seems to me this issue is entirely unrelated to Invoke-Expression:

The legacy workaround places the following on the process command line, which, I presume, worked:

MDF_FILEPATH="C:\Databases\junk.mdf"

In 7.3.0, the following is placed on the process command line:

MDF_FILEPATH=\"C:\Databases\junk.md\"

If the former worked, why would you expect the latter to work, which is obviously different?

Only if you interpreted the latter verbatim would you end up with MDF_FILEPATH="C:\Databases\junk.mdf", but that is beside the point, given that syntactically relevant " characters are the issue here.

@rmegal
Copy link
Author

rmegal commented Nov 16, 2022

@daxian-dbw I agree with @mklement0, the issue is unrelated to Invoke-Expression as I can recreate the problem without using it, and using the legacy workaround does allow the command to complete as before.

@daxian-dbw
Copy link
Member

daxian-dbw commented Nov 16, 2022

I'm not saying it's related to Invoke-Expression, but just using that as the reported repro to understand the issue. With that specific repro, it looks to be a user error because the extra 2 backticks surrounding C:\Databases\junk.mdf seems to be unnecessary even when running on 7.2.x.

When the extra 2 backticks are removed, the original repro should just work in 7.3.0, and that's why I asked @rmegal to try if the following works in 7.3.0 after setting $PSNativeCommandArgumentPassing = 'Windows'.

$test = "&sqlcmd -v MDF_FILEPATH=`"C:\Databases\junk.mdf`""
invoke-expression $test

I can recreate the problem without using it, and using the legacy workaround does allow the command to complete as before.

Can you please provide another repo without using Invoke-Expression? We need to evaluate whether this is a user error to decide whether or not sqlcmd should be added to the legacy list.

@mklement0
Copy link
Contributor

mklement0 commented Nov 16, 2022

$test = "&sqlcmd -v MDF_FILEPATH=`"C:\Databases\junk.mdf`""; `invoke-expression $test

can be reduced to &sqlcmd -v MDF_FILEPATH="C:\Databases\junk.mdf", which - due to not containing spaces - is placed as unqouoted, verbatim MDF_FILEPATH=C:\Databases\junk.mdf on the process command line.

Since there are no embedded " characters to pass, v7.2- and v7.3 behave the same.

As @rmegal reports, such an unquoted token appears not to work with sqlcmd, because the unquoted : appears to have special meaning to sqlcmd.

The workaround is to ensure double-quoting, selectively around the value part, so that verbatim MDF_FILEPATH="C:\Databases\junk.mdf" is placed on the process command line.

  • In v7.3, with (effective) Standard behavior, this cannot be done - except with the --% and cmd /c workarounds previously discussed.

  • In v7.2, or in v7.3 with Legacy in effect, you can build a workaround on the old, broken behavior, which is what the command in the OP does:

& { $PSNativeCommandArgumentPassing = 'Legacy'; sqlcmd -v MDF_FILEPATH=`"C:\Databases\junk.mdf`" }

@daxian-dbw
Copy link
Member

daxian-dbw commented Nov 16, 2022

In v7.2, or in v7.3 with Legacy in effect, you can build a workaround on the old, broken behavior, which is what the command in the OP does:

& { $PSNativeCommandArgumentPassing = 'Legacy'; sqlcmd -v MDF_FILEPATH=`"C:\Databases\junk.mdf`" }

This is the part that I'm confused with. See my screenshots below (running in 7.2.7, which essentially uses the Legacy mode):

image

I replaced sqlcmd with testexe.exe -echoargs to print out the passed in arguments. The results show no difference in having `" or " surrounding C:\Databases\junk.mdf. Both have verbatim MDF_FILEPATH=C:\Databases\junk.mdf on the process command line. Did I do something wrong?

@mklement0
Copy link
Contributor

mklement0 commented Nov 16, 2022

You need to look at the raw command line to see the difference, which testexe -echoargs doesn't show.

Allow me to use some canned advice on how to get a utility that does:

  • If you use Chocolatey, you can use choco install echoargs -y from an elevated session to install echoargs.exe, which shows the raw command line that PowerShell builds behind the scenes, and how (most) external programs parse it into arguments. Alternatively, you can compile a utility ad hoc: see this answer.

The difference is that your first command places MDF_FILEPATH="C:\Databases\junk.mdf" on the process command line (note the double quotes), whereas it is MDF_FILEPATH=C:\Databases\junk.mdf for the second command.

To an executable that uses the runtime-provided Microsoft C/C++/.NET argument parsing of the process command line, as testexe -echoargs does, these two arguments result in the same verbatim argument, namely MDF_FILEPATH=C:\Databases\junk.mdf - that is, the (unescaped) " characters are assumed to have syntactic function and are removed.

However, here we're talking about an executable, sqlcmd.exe, that seemingly defines its own rules and treats those raw arguments differently - that is the curse of the anarchy that is argument-passing on Windows, which - rather than passing an array of verbatim tokens, as is the case on Unix - forces each executable to act as its own quasi-shell and parse its arguments from a single string that encodes all arguments.

To an executable that uses the runtime-provided Microsoft C/C++/.NET argument parsing, all of the following are equivalent, but to sqlcmd and other high-profile CLIs such as msiexec and msdeploy, they aren't:

  • MDF_FILEPATH=C:\Databases\junk.md
  • MDF_FILEPATH="C:\Databases\junk.md"
  • "MDF_FILEPATH=C:\Databases\junk.md"

The related executable-agnostic accommodation I proposed as part of #15143 focuses on values (the part of the argument to the right of =) that require double-quoting, due to containing spaces, so that PowerShell would place whatever PowerShell syntax variation results in verbatim value foo=bar baz to pass as selectively double-quoted foo="bar baz" on the process command line.

However, it seems that sqlcmd elevates the anarchy to new heights, by also distinguishing between a space-less argument that is placed on the process command line as foo=c:\bar vs. foo="c:\bar"

@daxian-dbw
Copy link
Member

However, here we're talking about an executable, sqlcmd.exe, that seemingly defines its own rules and treats those raw arguments differently ...
To an executable that uses the runtime-provided Microsoft C/C++/.NET argument parsing, all of the following are equivalent, but to sqlcmd and other high-profile CLIs such as msiexec and msdeploy, they aren't:

@mklement0 This makes perfect sense to me now, thank you!
So, it makes sense to have sqlcmd in the legacy command list, and we are all good.

@daxian-dbw
Copy link
Member

@mklement0 One more question regarding Environment.CommandLine.

When using the utility compiled from this answer, the Environment.CommandLine shows raw: ["E:\yard\tmp\echoArgsPause.exe" -v MDF_FILEPATH="C:\Databases\junk.mdf"]

image

However, when using the testexe.exe -echoargs after updating it to write out CommandLine, it shows different value: raw: [testexe.dll -echoargs -v MDF_FILEPATH=C:\Databases\junk.mdf]

image

Both were on PS 7.2.7.
Do you know why there is such a difference between echoArgsPause.exe (built in PowerShell.exe, so targeting .NET Framework) and testexe.exe -echoargs (built targeting .NET 7)? Does Environment.CommandLine not reflect the RAW process argument anymore?

@mklement0
Copy link
Contributor

Indeed, a .NET (Core) application (as opposed to a .NET Framework application) does not report the true raw command line: see dotnet/runtime#11305 (comment)

@rmegal
Copy link
Author

rmegal commented Nov 17, 2022

@daxian-dbw and @mklement0 This was an eye opening conversation. Thanks a ton for weighing in.

@jborean93
Copy link
Collaborator

@daxian-dbw if it helps this is what I use to test out argv stuff on Windows

Add-Type -OutputType ConsoleApplication -OutputAssembly print_argv.exe -TypeDefinition @'
using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;

namespace PrintArgv
{
    class Program
    {
        [DllImport("Kernel32.dll")]
        public static extern IntPtr GetCommandLineW();

        static void Main(string[] args)
        {
            IntPtr cmdLinePtr = GetCommandLineW();
            string cmdLine = Marshal.PtrToStringUni(cmdLinePtr);

            Console.WriteLine(cmdLine);
            for (int i = 0; i < args.Length; i++)
            {
                Console.WriteLine("[{0}] {1}", i, args[i]);
            }
        }
    }
}
'@

This unfortunately needs to run in WinPS but it can be run anywhere. It will output the raw command line value as well as the arguments as parsed by dotnet

image

Note I haven't updated to 7.3.0 yet

For Linux I've also used the following before

#include<stdio.h>

int main(int argc, char *argv[])
{
    int i;
    for(i = 1;i < argc;i++)
    {
        printf("[%d] %s\n", i, argv[i]);
    }
    return 0;
}

You can compile it with gcc print_argv.c -o print_argv

image

It is a bit more confusing there because Linux doesn't have a command line string but uses char *argv[].

@daxian-dbw
Copy link
Member

daxian-dbw commented Nov 17, 2022

@jborean93 Thank you for sharing the code!
I submitted #18591 to update testext with a -echoraw switch, to show the raw command line and the individual args.
I think the existing testexe -echoraw is sufficient on Unix platforms, but let me know if that's not the case.

image

@jborean93
Copy link
Collaborator

Looks good, I have no idea what dotnet does for Environment.CommandLine outside of Windows, I'm not as familiar to process args there.

@mklement0
Copy link
Contributor

mklement0 commented Nov 17, 2022

I think the existing testexe -echoraw is sufficient on Unix platforms, but let me know if that's not the case.

I assume you meant testexe -echoargs and, yes, that would be sufficient on Unix, given that there is no such thing as a raw command line there (only an array of verbatim arguments passed via the relevant system functions).

However, is there really a need for a new, separate option?

Can't the existing -echoargs option:

  • either: just also show the raw command line, on Windows only?

  • or - with an appropriate hint - on Unix show a hypothetical command line that would work from a POSIX-compatible shell?

    • However, note that [Environment]::CommandLine does not provide such a hypothetical command line robustly, because it doesn't \-escape $-prefixed tokens, neither in unquoted arguments nor in arguments with spaces, which it invariably encloses in "...", which are interpolating strings in POSIX-compatible shells.
    • For instance, a test.ps1 file that contains [Environment]::CommandLine invoked from Bash with
      pwsh -file test.ps1 'There is no place like $HOME' (note that $HOME is not expanded, due to use of '...') is echoed as follows, which does not roundtrip properly, because the hypothetical reconstruction results in an interpolating POSIX-shell string (that the pwsh executable is reported as a .dll is a separate problem - see Environment.CommandLine for published  dotnet/runtime#11305):
      • /usr/bin/pwsh.dll -nop -file test.ps1 "There is no place like $HOME"

@mklement0
Copy link
Contributor

@daxian-dbw

This makes perfect sense to me now, thank you!

Glad to hear it.

So, it makes sense to have sqlcmd in the legacy command list, and we are all good.

I know I'm repeating myself, but I feel it bears repeating: such a legacy command list is a terrible idea; while it may selectively, obscurely help backward compatibility, it guarantees everlasting confusion going forward.

@daxian-dbw
Copy link
Member

@mklement0 testexe -echoargs is used in tests, and the tests only expect the individual arguments, not extra texts for the raw command line. The -echoraw flag is only for interactive use in cases like this one.

@mklement0
Copy link
Contributor

mklement0 commented Nov 17, 2022

Thanks for clarifying, @daxian-dbw.
On second thought re -echoraw, showing a hypothetical command line on Unix is probably not called for in a diagnostic utility, given that it would basically arbitrarily have to pick a shell whose syntax to conform to (even though there are only two logical candidates, POSIX-compatible shells and PowerShell) when reverse-engineering a (presumed) shell command line that the program itself never saw (and whose faithful reconstruction would be impossible).

@Scordo
Copy link

Scordo commented Nov 24, 2022

Hi,

we are facing the same problem with msbuild and property values containing a semicolon or comma.
We have to support powershell 5.1 up to pwsh 7.3 and are using escaped quotes which are breaking in pwsh 7.3.

We dont want to set $PSNativeCommandArgumentPassing script-wide because it may cause incompatibilities when including 3rd party modules/scripts which dont expect this.

So we decided to go with local changes in scripts with the help of the following function:

function Use-LegacyNativeCommandArgumentPassing {
  [CmdletBinding()]
  param (
    [Parameter(Mandatory=$true)]
    [scriptblock]
    $Script
  )

  if (!(test-path variable:PSNativeCommandArgumentPassing)) {
    & $Script
    return
  }

  $oldPSNativeCommandArgumentPassing = $PSNativeCommandArgumentPassing

  try {
    $PSNativeCommandArgumentPassing = 'Legacy'
    & $Script
  }
  finally {
    $PSNativeCommandArgumentPassing = $oldPSNativeCommandArgumentPassing
  }
}

Which we are using this way:

Use-LegacyNativeCommandArgumentPassing {
  & 'msbuild.exe' @('test.target', "/p:Category=`"CI,Nightly`"")
}

Hope this helps someone else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution-Fixed The issue is fixed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants