From f9cd8c592dcf30ece05948070507b0bac078d698 Mon Sep 17 00:00:00 2001 From: James Truher Date: Fri, 29 Jan 2021 11:32:47 -0800 Subject: [PATCH 01/22] Support using ArgumentList with native app execution Support both behaviors with PowerShell variable --- .../engine/NativeCommandParameterBinder.cs | 42 ++++++++++++++++++- .../NativeCommandParameterBinderController.cs | 11 +++++ .../engine/NativeCommandProcessor.cs | 36 ++++++++++++++-- 3 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index e0c643c3d3b..dd81c248d4a 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -3,6 +3,7 @@ using System.Collections; using System.Collections.ObjectModel; +using System.Collections.Generic; using System.IO; using System.Linq; using System.Management.Automation.Internal; @@ -151,6 +152,16 @@ internal string Arguments private readonly StringBuilder _arguments = new StringBuilder(); + internal string[] ArgumentList + { + get + { + return _argumentList.ToArray(); + } + } + + private List _argumentList = new List(); + #endregion internal members #region private members @@ -199,11 +210,15 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array if (!string.IsNullOrEmpty(arg)) { _arguments.Append(separator); + // Don't add the separator to the argument list, it's + // an artifact of constructing the string to be used. if (sawVerbatimArgumentMarker) { arg = Environment.ExpandEnvironmentVariables(arg); _arguments.Append(arg); + // we need to split the argument on spaces + _argumentList.AddRange(arg.Split(" ", StringSplitOptions.RemoveEmptyEntries)); } else { @@ -227,10 +242,12 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array if (stringConstantType == StringConstantType.DoubleQuoted) { _arguments.Append(ResolvePath(arg, Context)); + _argumentList.Add(ResolvePath(arg, Context)); } else { _arguments.Append(arg); + _argumentList.Add(arg); } // need to escape all trailing backslashes so the native command receives it correctly @@ -244,7 +261,22 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array } else { - PossiblyGlobArg(arg, stringConstantType); + if (argArrayAst != null) + { + // Ok, we have a literal array, so take the extent, break it on spaces + // and add them to the argument list + // Question? Should the break be space or tab (bash says no) + // _argumentList.AddRange(argArrayAst.Extent.Text.Split(" ", StringSplitOptions.RemoveEmptyEntries)); + foreach (string element in argArrayAst.Extent.Text.Split(" ", StringSplitOptions.RemoveEmptyEntries)) + { + PossiblyGlobArg(element, stringConstantType); + } + break; + } + else + { + PossiblyGlobArg(arg, stringConstantType); + } } } } @@ -291,6 +323,8 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) // Fallthrough will append the pattern unchanged. } + // TODO: we may need to add our own thing here + // Expand paths, but only from the file system. if (paths?.Count > 0 && paths.All(p => p.BaseObject is FileSystemInfo)) { @@ -298,6 +332,7 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) foreach (var path in paths) { _arguments.Append(sep); + // _argumentList.Add(sep); sep = " "; var expandedPath = (path.BaseObject as FileSystemInfo).FullName; if (normalizePath) @@ -311,10 +346,12 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) _arguments.Append('"'); _arguments.Append(expandedPath); _arguments.Append('"'); + _argumentList.Add(expandedPath); } else { _arguments.Append(expandedPath); + _argumentList.Add(expandedPath); } argExpanded = true; @@ -331,12 +368,14 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) if (string.Equals(arg, "~")) { _arguments.Append(home); + _argumentList.Add(home); argExpanded = true; } else if (arg.StartsWith("~/", StringComparison.OrdinalIgnoreCase)) { var replacementString = home + arg.Substring(1); _arguments.Append(replacementString); + _argumentList.Add(replacementString); argExpanded = true; } } @@ -351,6 +390,7 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) if (!argExpanded) { _arguments.Append(arg); + _argumentList.Add(arg); } } diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs index ecdd4554abe..25e4624bdb2 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs @@ -38,6 +38,17 @@ internal string Arguments } } + /// + /// Gets the command arguments as an array of strings + /// + internal string[] ArgumentList + { + get + { + return ((NativeCommandParameterBinder)DefaultParameterBinder).ArgumentList; + } + } + /// /// Passes the binding directly through to the parameter binder. /// It does no verification against metadata. diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index 96ad84a6c46..47c39422269 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -357,7 +357,7 @@ internal override void ProcessRecord() /// /// Indicate if we have called 'NotifyBeginApplication()' on the host, so that - /// we can call the counterpart 'NotifyEndApplication' as approriate. + /// we can call the counterpart 'NotifyEndApplication' as appropriate. /// private bool _hasNotifiedBeginApplication; @@ -1157,10 +1157,40 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE startInfo.CreateNoWindow = mpc.NonInteractive; } - startInfo.Arguments = NativeParameterBinderController.Arguments; - ExecutionContext context = this.Command.Context; + // We provide the user a way to select the new behavior via a new preference variable + // If the variable is set (and it 1) we will use the new behavior + bool OldStyleNativeInvocation = true; + string preference = LanguagePrimitives.ConvertTo(context.GetVariableValue(new VariablePath("PSNativeApplicationUsesArgumentList"))); + if (!string.IsNullOrEmpty(preference) && preference == "1") + { + OldStyleNativeInvocation = false; + } + using (ParameterBinderBase.bindingTracer.TraceScope( "BIND NAMED native application line args [{0}]", this.Path)) + { + if (OldStyleNativeInvocation) + { + using (ParameterBinderBase.bindingTracer.TraceScope( "BIND argument [{0}]", NativeParameterBinderController.Arguments)) + { + startInfo.Arguments = NativeParameterBinderController.Arguments; + } + } + else + { + // Use new API for running native application + int position = 0; + foreach (string nativeArgument in NativeParameterBinderController.ArgumentList) { + if (!string.IsNullOrEmpty(nativeArgument)) { + using (ParameterBinderBase.bindingTracer.TraceScope( "BIND cmd line arg [{0}] to position [{1}]", nativeArgument, position++)) + { + startInfo.ArgumentList.Add(nativeArgument); + } + } + } + } + } + // Start command in the current filesystem directory string rawPath = context.EngineSessionState.GetNamespaceCurrentLocation( From 8dc0d521e2318a4c392dfca1d4316864066ffa55 Mon Sep 17 00:00:00 2001 From: James Truher Date: Fri, 29 Jan 2021 12:14:50 -0800 Subject: [PATCH 02/22] add test code for changed behavior --- .../NativeExecution/NativeCommandArguments.Tests.ps1 | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index df71542a6bd..a1c4b1d0b48 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -1,5 +1,6 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. +$PSNativeApplicationUsesArgumentList = 1 Describe "Native Command Arguments" -tags "CI" { # When passing arguments to native commands, quoted segments that contain # spaces need to be quoted with '"' characters when they are passed to the @@ -11,8 +12,8 @@ Describe "Native Command Arguments" -tags "CI" { $a = 'a"b c"d' $lines = testexe -echoargs $a 'a"b c"d' a"b c"d $lines.Count | Should -Be 3 - $lines[0] | Should -BeExactly 'Arg 0 is ' - $lines[1] | Should -BeExactly 'Arg 1 is ' + $lines[0] | Should -BeExactly 'Arg 0 is ' + $lines[1] | Should -BeExactly 'Arg 1 is ' $lines[2] | Should -BeExactly 'Arg 2 is ' } @@ -30,8 +31,8 @@ Describe "Native Command Arguments" -tags "CI" { It "Should handle spaces between escaped quotes" { $lines = testexe -echoargs 'a\"b c\"d' "a\`"b c\`"d" $lines.Count | Should -Be 2 - $lines[0] | Should -BeExactly 'Arg 0 is ' - $lines[1] | Should -BeExactly 'Arg 1 is ' + $lines[0] | Should -BeExactly 'Arg 0 is ' + $lines[1] | Should -BeExactly 'Arg 1 is ' } It "Should correctly quote paths with spaces: " -TestCases @( From 7e74f9e31a8a1230431f28bec83ceaba684356b3 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 1 Feb 2021 16:03:15 -0800 Subject: [PATCH 03/22] change logic for dealing with nulls, but allow empty spaces '' as arguments --- .../engine/NativeCommandParameterBinder.cs | 26 +++++++++++++++++-- .../NativeCommandParameterBinderController.cs | 11 ++++++++ .../engine/NativeCommandProcessor.cs | 11 ++------ 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index dd81c248d4a..6b31453e9af 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -162,6 +162,20 @@ internal string[] ArgumentList private List _argumentList = new List(); + /// + /// If true, we will use the new ArgumentList variant for StartInfo + /// + internal bool UseArgumentList + { + get + { + string preference = LanguagePrimitives.ConvertTo( + Context.GetVariableValue(new VariablePath("PSNativeApplicationUsesArgumentList")) + ); + return (preference == "1"); + } + } + #endregion internal members #region private members @@ -187,9 +201,11 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array do { string arg; + object currentObj; if (list == null) { arg = PSObject.ToStringParser(context, obj); + currentObj = obj; } else { @@ -198,7 +214,8 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array break; } - arg = PSObject.ToStringParser(context, ParserOps.Current(null, list)); + currentObj = ParserOps.Current(null, list); + arg = PSObject.ToStringParser(context, currentObj); currentElement += 1; if (currentElement != 0) @@ -261,7 +278,7 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array } else { - if (argArrayAst != null) + if (argArrayAst != null && UseArgumentList) { // Ok, we have a literal array, so take the extent, break it on spaces // and add them to the argument list @@ -280,6 +297,11 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array } } } + else if (UseArgumentList && currentObj != null) + { + // add empty strings to arglist, but not nulls + _argumentList.Add(arg); + } } while (list != null); } diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs index 25e4624bdb2..6eaffde895b 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs @@ -49,6 +49,17 @@ internal string[] ArgumentList } } + /// + /// Get whether to use the new API for StartInfo + /// + internal bool UseArgumentList + { + get + { + return ((NativeCommandParameterBinder)DefaultParameterBinder).UseArgumentList; + } + } + /// /// Passes the binding directly through to the parameter binder. /// It does no verification against metadata. diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index 47c39422269..30a1b1bf033 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -1160,16 +1160,9 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE ExecutionContext context = this.Command.Context; // We provide the user a way to select the new behavior via a new preference variable - // If the variable is set (and it 1) we will use the new behavior - bool OldStyleNativeInvocation = true; - string preference = LanguagePrimitives.ConvertTo(context.GetVariableValue(new VariablePath("PSNativeApplicationUsesArgumentList"))); - if (!string.IsNullOrEmpty(preference) && preference == "1") - { - OldStyleNativeInvocation = false; - } using (ParameterBinderBase.bindingTracer.TraceScope( "BIND NAMED native application line args [{0}]", this.Path)) { - if (OldStyleNativeInvocation) + if (!NativeParameterBinderController.UseArgumentList) { using (ParameterBinderBase.bindingTracer.TraceScope( "BIND argument [{0}]", NativeParameterBinderController.Arguments)) { @@ -1181,7 +1174,7 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE // Use new API for running native application int position = 0; foreach (string nativeArgument in NativeParameterBinderController.ArgumentList) { - if (!string.IsNullOrEmpty(nativeArgument)) { + if (nativeArgument != null) { using (ParameterBinderBase.bindingTracer.TraceScope( "BIND cmd line arg [{0}] to position [{1}]", nativeArgument, position++)) { startInfo.ArgumentList.Add(nativeArgument); From 581b91a168857c04371a140ee09123ba86afc302 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 1 Feb 2021 16:03:43 -0800 Subject: [PATCH 04/22] update test to handle both new and old behaviors --- .../NativeCommandArguments.Tests.ps1 | 131 ++++++++++-------- 1 file changed, 72 insertions(+), 59 deletions(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index a1c4b1d0b48..fd0b4bb85e1 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -1,71 +1,84 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -$PSNativeApplicationUsesArgumentList = 1 -Describe "Native Command Arguments" -tags "CI" { - # When passing arguments to native commands, quoted segments that contain - # spaces need to be quoted with '"' characters when they are passed to the - # native command (or to bash or sh on Linux). - # - # This test checks that the proper quoting is occuring by passing arguments - # to the testexe native command and looking at how it got the arguments. - It "Should handle quoted spaces correctly" { - $a = 'a"b c"d' - $lines = testexe -echoargs $a 'a"b c"d' a"b c"d - $lines.Count | Should -Be 3 - $lines[0] | Should -BeExactly 'Arg 0 is ' - $lines[1] | Should -BeExactly 'Arg 1 is ' - $lines[2] | Should -BeExactly 'Arg 2 is ' - } +foreach ( $argumentListValue in 0,1 ) { + $PSNativeApplicationUsesArgumentList = $argumentListValue + Describe "Native Command Arguments" -tags "CI" { + # When passing arguments to native commands, quoted segments that contain + # spaces need to be quoted with '"' characters when they are passed to the + # native command (or to bash or sh on Linux). + # + # This test checks that the proper quoting is occuring by passing arguments + # to the testexe native command and looking at how it got the arguments. + It "Should handle quoted spaces correctly (ArgumentList=${PSNativeApplicationUsesArgumentList})" { + $a = 'a"b c"d' + $lines = testexe -echoargs $a 'a"b c"d' a"b c"d + $lines.Count | Should -Be 3 + if ( $PSNativeApplicationUsesArgumentList -eq 1 ) { + $lines[0] | Should -BeExactly 'Arg 0 is ' + $lines[1] | Should -BeExactly 'Arg 1 is ' + } + else { + $lines[0] | Should -BeExactly 'Arg 0 is ' + $lines[1] | Should -BeExactly 'Arg 1 is ' + } + $lines[2] | Should -BeExactly 'Arg 2 is ' + } - # In order to pass '"' characters so they are actually part of command line - # arguments for native commands, they need to be escaped with a '\' (this - # is in addition to the '`' escaping needed inside '"' quoted strings in - # PowerShell). - # - # This functionality was broken in PowerShell 5.0 and 5.1, so this test - # will fail on those versions unless the fix is backported to them. - # - # This test checks that the proper quoting and escaping is occurring by - # passing arguments with escaped quotes to the testexe native command and - # looking at how it got the arguments. - It "Should handle spaces between escaped quotes" { - $lines = testexe -echoargs 'a\"b c\"d' "a\`"b c\`"d" - $lines.Count | Should -Be 2 - $lines[0] | Should -BeExactly 'Arg 0 is ' - $lines[1] | Should -BeExactly 'Arg 1 is ' - } + # In order to pass '"' characters so they are actually part of command line + # arguments for native commands, they need to be escaped with a '\' (this + # is in addition to the '`' escaping needed inside '"' quoted strings in + # PowerShell). + # + # This functionality was broken in PowerShell 5.0 and 5.1, so this test + # will fail on those versions unless the fix is backported to them. + # + # This test checks that the proper quoting and escaping is occurring by + # passing arguments with escaped quotes to the testexe native command and + # looking at how it got the arguments. + It "Should handle spaces between escaped quotes (ArgumentList=${PSNativeApplicationUsesArgumentList})" { + $lines = testexe -echoargs 'a\"b c\"d' "a\`"b c\`"d" + $lines.Count | Should -Be 2 + if ( $PSNativeApplicationUsesArgumentList -eq 1 ) { + $lines[0] | Should -BeExactly 'Arg 0 is ' + $lines[1] | Should -BeExactly 'Arg 1 is ' + } + else { + $lines[0] | Should -BeExactly 'Arg 0 is ' + $lines[1] | Should -BeExactly 'Arg 1 is ' + } + } - It "Should correctly quote paths with spaces: " -TestCases @( - @{arguments = "'.\test 1\' `".\test 2\`"" ; expected = @(".\test 1\",".\test 2\")}, - @{arguments = "'.\test 1\\\' `".\test 2\\`""; expected = @(".\test 1\\\",".\test 2\\")} - ) { - param($arguments, $expected) - $lines = Invoke-Expression "testexe -echoargs $arguments" - $lines.Count | Should -Be $expected.Count - for ($i = 0; $i -lt $lines.Count; $i++) { - $lines[$i] | Should -BeExactly "Arg $i is <$($expected[$i])>" + It "Should correctly quote paths with spaces (ArgumentList=${PSNativeApplicationUsesArgumentList}): " -TestCases @( + @{arguments = "'.\test 1\' `".\test 2\`"" ; expected = @(".\test 1\",".\test 2\")}, + @{arguments = "'.\test 1\\\' `".\test 2\\`""; expected = @(".\test 1\\\",".\test 2\\")} + ) { + param($arguments, $expected) + $lines = Invoke-Expression "testexe -echoargs $arguments" + $lines.Count | Should -Be $expected.Count + for ($i = 0; $i -lt $lines.Count; $i++) { + $lines[$i] | Should -BeExactly "Arg $i is <$($expected[$i])>" + } } - } - It "Should handle PowerShell arrays with or without spaces correctly: " -TestCases @( - @{arguments = "1,2"; expected = @("1,2")} - @{arguments = "1,2,3"; expected = @("1,2,3")} - @{arguments = "1, 2"; expected = "1,", "2"} - @{arguments = "1 ,2"; expected = "1", ",2"} - @{arguments = "1 , 2"; expected = "1", ",", "2"} - @{arguments = "1, 2,3"; expected = "1,", "2,3"} - @{arguments = "1 ,2,3"; expected = "1", ",2,3"} - @{arguments = "1 , 2,3"; expected = "1", ",", "2,3"} - ) { - param($arguments, $expected) - $lines = @(Invoke-Expression "testexe -echoargs $arguments") - $lines.Count | Should -Be $expected.Count - for ($i = 0; $i -lt $expected.Count; $i++) { - $lines[$i] | Should -BeExactly "Arg $i is <$($expected[$i])>" + It "Should handle PowerShell arrays with or without spaces correctly (ArgumentList=${PSNativeApplicationUsesArgumentList}): " -TestCases @( + @{arguments = "1,2"; expected = @("1,2")} + @{arguments = "1,2,3"; expected = @("1,2,3")} + @{arguments = "1, 2"; expected = "1,", "2"} + @{arguments = "1 ,2"; expected = "1", ",2"} + @{arguments = "1 , 2"; expected = "1", ",", "2"} + @{arguments = "1, 2,3"; expected = "1,", "2,3"} + @{arguments = "1 ,2,3"; expected = "1", ",2,3"} + @{arguments = "1 , 2,3"; expected = "1", ",", "2,3"} + ) { + param($arguments, $expected) + $lines = @(Invoke-Expression "testexe -echoargs $arguments") + $lines.Count | Should -Be $expected.Count + for ($i = 0; $i -lt $expected.Count; $i++) { + $lines[$i] | Should -BeExactly "Arg $i is <$($expected[$i])>" + } } } } - Describe 'PSPath to native commands' { BeforeAll { $featureEnabled = $EnabledExperimentalFeatures.Contains('PSNativePSPathResolution') From ef03db753bf7e07a3b7facfab1cf4854a13ee889 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 8 Mar 2021 15:57:52 -0800 Subject: [PATCH 05/22] Add passing native args as argumentlist as an experimental feature. Refactor adding variables to initial session state so experimental feature variables can be easily omitted based on whether the experimental feature is enabled --- .../engine/CommandBase.cs | 14 ++++++ .../ExperimentalFeature.cs | 3 ++ .../engine/InitialSessionState.cs | 44 ++++++++++++++++--- .../engine/NativeCommandParameterBinder.cs | 23 +++++++--- .../engine/SpecialVariables.cs | 5 +++ .../resources/RunspaceInit.resx | 3 ++ 6 files changed, 82 insertions(+), 10 deletions(-) diff --git a/src/System.Management.Automation/engine/CommandBase.cs b/src/System.Management.Automation/engine/CommandBase.cs index da2e97e516b..83736d05e7c 100644 --- a/src/System.Management.Automation/engine/CommandBase.cs +++ b/src/System.Management.Automation/engine/CommandBase.cs @@ -272,6 +272,20 @@ internal void InternalDispose(bool isDisposing) namespace System.Management.Automation { + #region NativeArgumentPassingStyle + /// + /// Defines the different native command argument parsing options. + /// + public enum NativeArgumentPassingStyle + { + /// Use legacy argument parsing via ProcessStartInfo.Arguments + Legacy = 0, + + /// Use new style argument parsing via ProcessStartInfo.ArgumentList + Standard = 1 + } + #endregion NativeArgumentPassingStyle + #region ErrorView /// /// Defines the potential ErrorView options. diff --git a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs index 5eeaf814e10..a68b1208294 100644 --- a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs +++ b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs @@ -136,6 +136,9 @@ static ExperimentalFeature() new ExperimentalFeature( name: PSAnsiProgressFeatureName, description: "Enable lightweight progress bar that leverages ANSI codes for rendering"), + new ExperimentalFeature( + name: "PSNativeCommandArgumentPassing", + description: "Use ArgumentList when invoking a native command"), }; EngineExperimentalFeatures = new ReadOnlyCollection(engineFeatures); diff --git a/src/System.Management.Automation/engine/InitialSessionState.cs b/src/System.Management.Automation/engine/InitialSessionState.cs index 257cb79ce7b..698fdcc6cbc 100644 --- a/src/System.Management.Automation/engine/InitialSessionState.cs +++ b/src/System.Management.Automation/engine/InitialSessionState.cs @@ -1308,6 +1308,32 @@ private static void MakeDisallowedEntriesPrivate(InitialSessionStateEntryColl } } + #region VariableHelper + /// + /// A helper for adding variables to session state. + /// Experimental features can be handled here + /// + private void AddVariables(IEnumerablevariables) + { + Variables.Add(variables); + + // If the PSNativeCommandArgumentPassing feature is enabled, create the variable which controls the behavior + // Since the BuiltInVariables list is static, and this should be done dynamically + // we need to do this here. + if (ExperimentalFeature.IsEnabled("PSNativeCommandArgumentPassing")) + { + Variables.Add( + new SessionStateVariableEntry( + SpecialVariables.NativeArgumentPassing, + NativeArgumentPassingStyle.Standard, + RunspaceInit.NativeCommandArgumentPassingDescription, + ScopedItemOptions.None, + new ArgumentTypeConverterAttribute(typeof(NativeArgumentPassingStyle)) + )); + } + } + #endregion + /// /// Creates an initial session state from a PSSC configuration file. /// @@ -1413,7 +1439,7 @@ private static InitialSessionState CreateRestrictedForRemoteServer() } // Add built-in variables. - iss.Variables.Add(BuiltInVariables); + iss.AddVariables(BuiltInVariables); // wrap some commands in a proxy function to restrict their parameters foreach (KeyValuePair proxyFunction in CommandMetadata.GetRestrictedCommands(SessionCapabilities.RemoteServer)) @@ -1477,7 +1503,7 @@ public static InitialSessionState Create() // be causing test failures - i suspect due to lack test isolation - brucepay Mar 06/2008 #if false // Add the default variables and make them private... - iss.Variables.Add(BuiltInVariables); + iss.AddVariables(BuiltInVariables); foreach (SessionStateVariableEntry v in iss.Variables) { v.Visibility = SessionStateEntryVisibility.Private; @@ -1500,7 +1526,7 @@ public static InitialSessionState CreateDefault() InitialSessionState ss = new InitialSessionState(); - ss.Variables.Add(BuiltInVariables); + ss.AddVariables(BuiltInVariables); ss.Commands.Add(new SessionStateApplicationEntry("*")); ss.Commands.Add(new SessionStateScriptEntry("*")); ss.Commands.Add(BuiltInFunctions); @@ -1567,7 +1593,7 @@ public static InitialSessionState CreateDefault2() { InitialSessionState ss = new InitialSessionState(); - ss.Variables.Add(BuiltInVariables); + ss.AddVariables(BuiltInVariables); ss.Commands.Add(new SessionStateApplicationEntry("*")); ss.Commands.Add(new SessionStateScriptEntry("*")); ss.Commands.Add(BuiltInFunctions); @@ -1608,7 +1634,7 @@ public InitialSessionState Clone() { InitialSessionState ss = new InitialSessionState(); - ss.Variables.Add(this.Variables.Clone()); + ss.AddVariables(this.Variables.Clone()); ss.EnvironmentVariables.Add(this.EnvironmentVariables.Clone()); ss.Commands.Add(this.Commands.Clone()); ss.Assemblies.Add(this.Assemblies.Clone()); @@ -4551,6 +4577,14 @@ .ForwardHelpCategory Cmdlet ScopedItemOptions.None), // End: Variables which control remoting behavior + new SessionStateVariableEntry( + SpecialVariables.PSSessionApplicationName, + "wsman", + RemotingErrorIdStrings.PSSessionAppName, + ScopedItemOptions.None), + + // End: Variable which controls native command argument parsing + #region Platform new SessionStateVariableEntry( SpecialVariables.IsLinux, diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index 6b31453e9af..fc6a51e5af4 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -163,16 +163,29 @@ internal string[] ArgumentList private List _argumentList = new List(); /// - /// If true, we will use the new ArgumentList variant for StartInfo + /// If the experimental feature is enabled and the value is not set to Legacy, we will use the new ArgumentList variant for StartInfo /// internal bool UseArgumentList { get { - string preference = LanguagePrimitives.ConvertTo( - Context.GetVariableValue(new VariablePath("PSNativeApplicationUsesArgumentList")) - ); - return (preference == "1"); + if (ExperimentalFeature.IsEnabled("PSNativeCommandArgumentPassing")) + { + try + { + // This will default to the new behavior if it is set to anything other than Legacy + var preference = LanguagePrimitives.ConvertTo( + Context.GetVariableValue(new VariablePath(SpecialVariables.NativeArgumentPassing), NativeArgumentPassingStyle.Standard) + ); + return (preference != NativeArgumentPassingStyle.Legacy); + } + catch + { + // The value is not convertable send back true + return true; + } + } + return false; } } diff --git a/src/System.Management.Automation/engine/SpecialVariables.cs b/src/System.Management.Automation/engine/SpecialVariables.cs index ba757722e91..0f95349664f 100644 --- a/src/System.Management.Automation/engine/SpecialVariables.cs +++ b/src/System.Management.Automation/engine/SpecialVariables.cs @@ -257,6 +257,11 @@ internal static class SpecialVariables #endregion Preference Variables + // Native command argument passing style + internal const string NativeArgumentPassing = "PSNativeCommandArgumentPassing"; + + internal static readonly VariablePath NativeArgumentPassingVarPath = new VariablePath(NativeArgumentPassing); + internal const string ErrorView = "ErrorView"; internal static readonly VariablePath ErrorViewVarPath = new VariablePath(ErrorView); diff --git a/src/System.Management.Automation/resources/RunspaceInit.resx b/src/System.Management.Automation/resources/RunspaceInit.resx index ac900465d7d..a2497961901 100644 --- a/src/System.Management.Automation/resources/RunspaceInit.resx +++ b/src/System.Management.Automation/resources/RunspaceInit.resx @@ -189,6 +189,9 @@ If true, WhatIf is considered to be enabled for all commands. + + Dictates how arguments are passed to native executables. + Dictates the limit of enumeration on formatting IEnumerable objects From c676e1d645f49ed0cdce103b3e8ee9cd4ec21c9a Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 8 Mar 2021 17:02:36 -0800 Subject: [PATCH 06/22] Update test to use experimental feature and new variable --- .../NativeCommandArguments.Tests.ps1 | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index fd0b4bb85e1..0857883685d 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -1,19 +1,30 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -foreach ( $argumentListValue in 0,1 ) { - $PSNativeApplicationUsesArgumentList = $argumentListValue - Describe "Native Command Arguments" -tags "CI" { +Describe "Will error correctly if an attempt to set variable to improper value" { + It "will error when setting variable incorrectly" { + if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled ) { + { $global:PSNativeCommandArgumentPassing = "zzz" } | Should -Throw -ExceptionType System.Management.Automation.ArgumentTransformationMetadataException + } + else { + Set-Test -State skipped -Because "Experimental feature 'PSNativeCommandArgumentPassing' is not enabled" + } + } +} + +foreach ( $argumentListValue in "Standard","Legacy" ) { + $PSNativeCommandArgumentPassing = $argumentListValue + Describe "Native Command Arguments (${PSNativeCommandArgumentPassing})" -tags "CI" { # When passing arguments to native commands, quoted segments that contain # spaces need to be quoted with '"' characters when they are passed to the # native command (or to bash or sh on Linux). # # This test checks that the proper quoting is occuring by passing arguments # to the testexe native command and looking at how it got the arguments. - It "Should handle quoted spaces correctly (ArgumentList=${PSNativeApplicationUsesArgumentList})" { + It "Should handle quoted spaces correctly (ArgumentList=${PSNativeCommandArgumentPassing})" { $a = 'a"b c"d' $lines = testexe -echoargs $a 'a"b c"d' a"b c"d $lines.Count | Should -Be 3 - if ( $PSNativeApplicationUsesArgumentList -eq 1 ) { + if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled -and $PSNativeCommandArgumentPassing -ne "Legacy" ) { $lines[0] | Should -BeExactly 'Arg 0 is ' $lines[1] | Should -BeExactly 'Arg 1 is ' } @@ -35,10 +46,10 @@ foreach ( $argumentListValue in 0,1 ) { # This test checks that the proper quoting and escaping is occurring by # passing arguments with escaped quotes to the testexe native command and # looking at how it got the arguments. - It "Should handle spaces between escaped quotes (ArgumentList=${PSNativeApplicationUsesArgumentList})" { + It "Should handle spaces between escaped quotes (ArgumentList=${PSNativeCommandArgumentPassing})" { $lines = testexe -echoargs 'a\"b c\"d' "a\`"b c\`"d" $lines.Count | Should -Be 2 - if ( $PSNativeApplicationUsesArgumentList -eq 1 ) { + if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled -and $PSNativeCommandArgumentPassing -ne "Legacy" ) { $lines[0] | Should -BeExactly 'Arg 0 is ' $lines[1] | Should -BeExactly 'Arg 1 is ' } @@ -48,7 +59,7 @@ foreach ( $argumentListValue in 0,1 ) { } } - It "Should correctly quote paths with spaces (ArgumentList=${PSNativeApplicationUsesArgumentList}): " -TestCases @( + It "Should correctly quote paths with spaces (ArgumentList=${PSNativeCommandArgumentPassing}): " -TestCases @( @{arguments = "'.\test 1\' `".\test 2\`"" ; expected = @(".\test 1\",".\test 2\")}, @{arguments = "'.\test 1\\\' `".\test 2\\`""; expected = @(".\test 1\\\",".\test 2\\")} ) { @@ -60,7 +71,7 @@ foreach ( $argumentListValue in 0,1 ) { } } - It "Should handle PowerShell arrays with or without spaces correctly (ArgumentList=${PSNativeApplicationUsesArgumentList}): " -TestCases @( + It "Should handle PowerShell arrays with or without spaces correctly (ArgumentList=${PSNativeCommandArgumentPassing}): " -TestCases @( @{arguments = "1,2"; expected = @("1,2")} @{arguments = "1,2,3"; expected = @("1,2,3")} @{arguments = "1, 2"; expected = "1,", "2"} From 7485621de52022cc36aa0536e0d5c9c72f99d1ee Mon Sep 17 00:00:00 2001 From: James Truher Date: Tue, 9 Mar 2021 10:13:42 -0800 Subject: [PATCH 07/22] Address a number of code factor issues. --- .../engine/InitialSessionState.cs | 11 +++++----- .../engine/NativeCommandParameterBinder.cs | 22 ++++++++----------- .../engine/NativeCommandProcessor.cs | 10 +++++---- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/System.Management.Automation/engine/InitialSessionState.cs b/src/System.Management.Automation/engine/InitialSessionState.cs index 698fdcc6cbc..78118b7def9 100644 --- a/src/System.Management.Automation/engine/InitialSessionState.cs +++ b/src/System.Management.Automation/engine/InitialSessionState.cs @@ -1311,9 +1311,11 @@ private static void MakeDisallowedEntriesPrivate(InitialSessionStateEntryColl #region VariableHelper /// /// A helper for adding variables to session state. - /// Experimental features can be handled here + /// Experimental features can be handled here. /// - private void AddVariables(IEnumerablevariables) + /// The variables to add to session state. + /// + private void AddVariables(IEnumerable variables) { Variables.Add(variables); @@ -1328,8 +1330,7 @@ private void AddVariables(IEnumerablevariables) NativeArgumentPassingStyle.Standard, RunspaceInit.NativeCommandArgumentPassingDescription, ScopedItemOptions.None, - new ArgumentTypeConverterAttribute(typeof(NativeArgumentPassingStyle)) - )); + new ArgumentTypeConverterAttribute(typeof(NativeArgumentPassingStyle)))); } } #endregion @@ -4583,8 +4584,6 @@ .ForwardHelpCategory Cmdlet RemotingErrorIdStrings.PSSessionAppName, ScopedItemOptions.None), - // End: Variable which controls native command argument parsing - #region Platform new SessionStateVariableEntry( SpecialVariables.IsLinux, diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index fc6a51e5af4..72032a6d531 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -2,8 +2,8 @@ // Licensed under the MIT License. using System.Collections; -using System.Collections.ObjectModel; using System.Collections.Generic; +using System.Collections.ObjectModel; using System.IO; using System.Linq; using System.Management.Automation.Internal; @@ -163,7 +163,7 @@ internal string[] ArgumentList private List _argumentList = new List(); /// - /// If the experimental feature is enabled and the value is not set to Legacy, we will use the new ArgumentList variant for StartInfo + /// Gets a value indicating whether to use an ArgumentList or string for arguments when invoking a native executable. /// internal bool UseArgumentList { @@ -175,9 +175,8 @@ internal bool UseArgumentList { // This will default to the new behavior if it is set to anything other than Legacy var preference = LanguagePrimitives.ConvertTo( - Context.GetVariableValue(new VariablePath(SpecialVariables.NativeArgumentPassing), NativeArgumentPassingStyle.Standard) - ); - return (preference != NativeArgumentPassingStyle.Legacy); + Context.GetVariableValue(new VariablePath(SpecialVariables.NativeArgumentPassing), NativeArgumentPassingStyle.Standard)); + return preference != NativeArgumentPassingStyle.Legacy; } catch { @@ -185,6 +184,7 @@ internal bool UseArgumentList return true; } } + return false; } } @@ -239,14 +239,14 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array if (!string.IsNullOrEmpty(arg)) { + // Only add the separator to the argument string rather than adding a separator to the ArgumentList. _arguments.Append(separator); - // Don't add the separator to the argument list, it's - // an artifact of constructing the string to be used. if (sawVerbatimArgumentMarker) { arg = Environment.ExpandEnvironmentVariables(arg); _arguments.Append(arg); + // we need to split the argument on spaces _argumentList.AddRange(arg.Split(" ", StringSplitOptions.RemoveEmptyEntries)); } @@ -293,14 +293,12 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array { if (argArrayAst != null && UseArgumentList) { - // Ok, we have a literal array, so take the extent, break it on spaces - // and add them to the argument list - // Question? Should the break be space or tab (bash says no) - // _argumentList.AddRange(argArrayAst.Extent.Text.Split(" ", StringSplitOptions.RemoveEmptyEntries)); + // We have a literal array, so take the extent, break it on spaces and add them to the argument list. foreach (string element in argArrayAst.Extent.Text.Split(" ", StringSplitOptions.RemoveEmptyEntries)) { PossiblyGlobArg(element, stringConstantType); } + break; } else @@ -358,8 +356,6 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) // Fallthrough will append the pattern unchanged. } - // TODO: we may need to add our own thing here - // Expand paths, but only from the file system. if (paths?.Count > 0 && paths.All(p => p.BaseObject is FileSystemInfo)) { diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index 30a1b1bf033..d94da341ec6 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -1160,7 +1160,7 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE ExecutionContext context = this.Command.Context; // We provide the user a way to select the new behavior via a new preference variable - using (ParameterBinderBase.bindingTracer.TraceScope( "BIND NAMED native application line args [{0}]", this.Path)) + using (ParameterBinderBase.bindingTracer.TraceScope("BIND NAMED native application line args [{0}]", this.Path)) { if (!NativeParameterBinderController.UseArgumentList) { @@ -1173,9 +1173,11 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE { // Use new API for running native application int position = 0; - foreach (string nativeArgument in NativeParameterBinderController.ArgumentList) { - if (nativeArgument != null) { - using (ParameterBinderBase.bindingTracer.TraceScope( "BIND cmd line arg [{0}] to position [{1}]", nativeArgument, position++)) + foreach (string nativeArgument in NativeParameterBinderController.ArgumentList) + { + if (nativeArgument != null) + { + using (ParameterBinderBase.bindingTracer.TraceScope("BIND cmd line arg [{0}] to position [{1}]", nativeArgument, position++)) { startInfo.ArgumentList.Add(nativeArgument); } From 4d2b442b57aa5556e90984b196a402f8adcb7aa8 Mon Sep 17 00:00:00 2001 From: James Truher Date: Tue, 9 Mar 2021 10:38:33 -0800 Subject: [PATCH 08/22] Additional CodeFactor fixes --- src/System.Management.Automation/engine/CommandBase.cs | 4 ++-- .../engine/InitialSessionState.cs | 1 - .../engine/NativeCommandParameterBinderController.cs | 7 +++---- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/System.Management.Automation/engine/CommandBase.cs b/src/System.Management.Automation/engine/CommandBase.cs index 83736d05e7c..2e1c5cda56c 100644 --- a/src/System.Management.Automation/engine/CommandBase.cs +++ b/src/System.Management.Automation/engine/CommandBase.cs @@ -278,10 +278,10 @@ namespace System.Management.Automation /// public enum NativeArgumentPassingStyle { - /// Use legacy argument parsing via ProcessStartInfo.Arguments + /// Use legacy argument parsing via ProcessStartInfo.Arguments. Legacy = 0, - /// Use new style argument parsing via ProcessStartInfo.ArgumentList + /// Use new style argument parsing via ProcessStartInfo.ArgumentList. Standard = 1 } #endregion NativeArgumentPassingStyle diff --git a/src/System.Management.Automation/engine/InitialSessionState.cs b/src/System.Management.Automation/engine/InitialSessionState.cs index 78118b7def9..59c1f5094c3 100644 --- a/src/System.Management.Automation/engine/InitialSessionState.cs +++ b/src/System.Management.Automation/engine/InitialSessionState.cs @@ -1314,7 +1314,6 @@ private static void MakeDisallowedEntriesPrivate(InitialSessionStateEntryColl /// Experimental features can be handled here. /// /// The variables to add to session state. - /// private void AddVariables(IEnumerable variables) { Variables.Add(variables); diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs index 6eaffde895b..9cd13269753 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs @@ -39,7 +39,7 @@ internal string Arguments } /// - /// Gets the command arguments as an array of strings + /// Gets the value of the command arguments as an array of strings. /// internal string[] ArgumentList { @@ -50,7 +50,7 @@ internal string[] ArgumentList } /// - /// Get whether to use the new API for StartInfo + /// Get a value indicating whether to use the new API for StartInfo. /// internal bool UseArgumentList { @@ -71,8 +71,7 @@ internal bool UseArgumentList /// Ignored. /// /// - /// True if the parameter was successfully bound. Any error condition - /// produces an exception. + /// True if the parameter was successfully bound. Any error condition produces an exception. /// internal override bool BindParameter( CommandParameterInternal argument, From fadf451bae72524159e6ac1332e7e9bd1e40adb0 Mon Sep 17 00:00:00 2001 From: James Truher Date: Tue, 9 Mar 2021 16:40:20 -0800 Subject: [PATCH 09/22] Additional CodeFactor fixes --- .../engine/NativeCommandParameterBinderController.cs | 2 +- .../engine/NativeCommandProcessor.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs index 9cd13269753..933c71f5503 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinderController.cs @@ -50,7 +50,7 @@ internal string[] ArgumentList } /// - /// Get a value indicating whether to use the new API for StartInfo. + /// Gets a value indicating whether to use the new API for StartInfo. /// internal bool UseArgumentList { diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index d94da341ec6..6899a1de0ef 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -1164,7 +1164,7 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE { if (!NativeParameterBinderController.UseArgumentList) { - using (ParameterBinderBase.bindingTracer.TraceScope( "BIND argument [{0}]", NativeParameterBinderController.Arguments)) + using (ParameterBinderBase.bindingTracer.TraceScope("BIND argument [{0}]", NativeParameterBinderController.Arguments)) { startInfo.Arguments = NativeParameterBinderController.Arguments; } From 93b4c4ba43f49eeab9d97860d9a440291933127b Mon Sep 17 00:00:00 2001 From: James Truher Date: Wed, 10 Mar 2021 15:01:37 -0800 Subject: [PATCH 10/22] Refactor argument list creation to use common method. This is because we need to pass the parameter to handle the case in a native app where '-parm:value' is presented. --- .../engine/NativeCommandParameterBinder.cs | 53 +++++++++++++------ test/powershell/Host/ConsoleHost.Tests.ps1 | 4 +- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index 72032a6d531..07873981393 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -83,7 +83,7 @@ internal void BindParameters(Collection parameters) if (parameter.ParameterNameSpecified) { Diagnostics.Assert(!parameter.ParameterText.Contains(' '), "Parameters cannot have whitespace"); - PossiblyGlobArg(parameter.ParameterText, StringConstantType.BareWord); + PossiblyGlobArg(parameter.ParameterText, parameter, StringConstantType.BareWord); if (parameter.SpaceAfterParameter) { @@ -131,7 +131,7 @@ internal void BindParameters(Collection parameters) stringConstantType = StringConstantType.DoubleQuoted; } - AppendOneNativeArgument(Context, argValue, arrayLiteralAst, sawVerbatimArgumentMarker, stringConstantType); + AppendOneNativeArgument(Context, parameter, argValue, arrayLiteralAst, sawVerbatimArgumentMarker, stringConstantType); } } } @@ -160,6 +160,28 @@ internal string[] ArgumentList } } + /// + /// Add an argument to the ArgumentList. + /// We may need to construct the argument out of the parameter text and the argument + /// in the case that we have a parameter that appears as "-switch:value". + /// + internal void AddToArgumentList(CommandParameterInternal parameter, string argument) + { + + if (parameter.ParameterNameSpecified && parameter.ParameterText.EndsWith(":")) + { + if (argument != parameter.ParameterText) + { + _argumentList.Add(parameter.ParameterText + argument); + } + } + else + { + _argumentList.Add(argument); + } + + } + private List _argumentList = new List(); /// @@ -199,11 +221,12 @@ internal bool UseArgumentList /// each of which will be stringized. /// /// Execution context instance. + /// The parameter we're looking at /// The object to append. /// If the argument was an array literal, the Ast, otherwise null. /// True if the argument occurs after --%. /// Bare, SingleQuoted, or DoubleQuoted. - private void AppendOneNativeArgument(ExecutionContext context, object obj, ArrayLiteralAst argArrayAst, bool sawVerbatimArgumentMarker, StringConstantType stringConstantType) + private void AppendOneNativeArgument(ExecutionContext context, CommandParameterInternal parameter, object obj, ArrayLiteralAst argArrayAst, bool sawVerbatimArgumentMarker, StringConstantType stringConstantType) { IEnumerator list = LanguagePrimitives.GetEnumerator(obj); @@ -272,12 +295,12 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array if (stringConstantType == StringConstantType.DoubleQuoted) { _arguments.Append(ResolvePath(arg, Context)); - _argumentList.Add(ResolvePath(arg, Context)); + AddToArgumentList(parameter, ResolvePath(arg, Context)); } else { _arguments.Append(arg); - _argumentList.Add(arg); + AddToArgumentList(parameter, arg); } // need to escape all trailing backslashes so the native command receives it correctly @@ -296,14 +319,14 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array // We have a literal array, so take the extent, break it on spaces and add them to the argument list. foreach (string element in argArrayAst.Extent.Text.Split(" ", StringSplitOptions.RemoveEmptyEntries)) { - PossiblyGlobArg(element, stringConstantType); + PossiblyGlobArg(element, parameter, stringConstantType); } break; } else { - PossiblyGlobArg(arg, stringConstantType); + PossiblyGlobArg(arg, parameter, stringConstantType); } } } @@ -311,7 +334,7 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array else if (UseArgumentList && currentObj != null) { // add empty strings to arglist, but not nulls - _argumentList.Add(arg); + AddToArgumentList(parameter, arg); } } while (list != null); @@ -322,8 +345,9 @@ private void AppendOneNativeArgument(ExecutionContext context, object obj, Array /// On Unix, do globbing as appropriate, otherwise just append . /// /// The argument that possibly needs expansion. + /// The parameter we're looking at /// Bare, SingleQuoted, or DoubleQuoted. - private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) + private void PossiblyGlobArg(string arg, CommandParameterInternal parameter, StringConstantType stringConstantType) { var argExpanded = false; @@ -363,7 +387,6 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) foreach (var path in paths) { _arguments.Append(sep); - // _argumentList.Add(sep); sep = " "; var expandedPath = (path.BaseObject as FileSystemInfo).FullName; if (normalizePath) @@ -377,12 +400,12 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) _arguments.Append('"'); _arguments.Append(expandedPath); _arguments.Append('"'); - _argumentList.Add(expandedPath); + AddToArgumentList(parameter, expandedPath); } else { _arguments.Append(expandedPath); - _argumentList.Add(expandedPath); + AddToArgumentList(parameter, expandedPath); } argExpanded = true; @@ -399,14 +422,14 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) if (string.Equals(arg, "~")) { _arguments.Append(home); - _argumentList.Add(home); + AddToArgumentList(parameter, home); argExpanded = true; } else if (arg.StartsWith("~/", StringComparison.OrdinalIgnoreCase)) { var replacementString = home + arg.Substring(1); _arguments.Append(replacementString); - _argumentList.Add(replacementString); + AddToArgumentList(parameter, replacementString); argExpanded = true; } } @@ -421,7 +444,7 @@ private void PossiblyGlobArg(string arg, StringConstantType stringConstantType) if (!argExpanded) { _arguments.Append(arg); - _argumentList.Add(arg); + AddToArgumentList(parameter, arg); } } diff --git a/test/powershell/Host/ConsoleHost.Tests.ps1 b/test/powershell/Host/ConsoleHost.Tests.ps1 index 390b461fb90..0cdb393c06a 100644 --- a/test/powershell/Host/ConsoleHost.Tests.ps1 +++ b/test/powershell/Host/ConsoleHost.Tests.ps1 @@ -235,8 +235,8 @@ Describe "ConsoleHost unit tests" -tags "Feature" { $observed | Should -BeExactly "h-llo" } - It "Empty command should fail" { - & $powershell -noprofile -c '' + It "Missing command should fail" { + & $powershell -noprofile -c $LASTEXITCODE | Should -Be 64 } From 15f285184431c3287cc4db2e98cd5c82e9ed6d85 Mon Sep 17 00:00:00 2001 From: James Truher Date: Thu, 11 Mar 2021 12:36:44 -0800 Subject: [PATCH 11/22] Update help function to use new style of arg passing for native apps --- .../engine/InitialSessionState.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.Management.Automation/engine/InitialSessionState.cs b/src/System.Management.Automation/engine/InitialSessionState.cs index 59c1f5094c3..9dca181c7c8 100644 --- a/src/System.Management.Automation/engine/InitialSessionState.cs +++ b/src/System.Management.Automation/engine/InitialSessionState.cs @@ -4298,7 +4298,7 @@ .FORWARDHELPCATEGORY Cmdlet } else { $pagerCommand = 'less' - $pagerArgs = '-Ps""Page %db?B of %D:.\. Press h for help or q to quit\.$""' + $pagerArgs = '-s','-P','Page %db?B of %D:.\. Press h for help or q to quit\.' } # Respect PAGER environment variable which allows user to specify a custom pager. @@ -4340,8 +4340,8 @@ .FORWARDHELPCATEGORY Cmdlet if ($pagerArgs) { # Supply pager arguments to an application without any PowerShell parsing of the arguments. # Leave environment variable to help user debug arguments supplied in $env:PAGER. - $env:__PSPAGER_ARGS = $pagerArgs - $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand --% %__PSPAGER_ARGS% + # $env:__PSPAGER_ARGS = $pagerArgs + $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand $pagerArgs } else { $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand From 37ada297ddb8fcc1eb342b22dbe4d9bc06bd9f7f Mon Sep 17 00:00:00 2001 From: James Truher Date: Thu, 11 Mar 2021 13:43:20 -0800 Subject: [PATCH 12/22] Remove escaped quotes from test as they are no longer needed --- .../Scripting/NativeExecution/NativeStreams.Tests.ps1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 index 59702938375..755ea27f794 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 @@ -12,9 +12,9 @@ Describe "Native streams behavior with PowerShell" -Tags 'CI' { $error.Clear() $command = [string]::Join('', @( - '[Console]::Error.Write(\"foo`n`nbar`n`nbaz\"); ', - '[Console]::Error.Write(\"middle\"); ', - '[Console]::Error.Write(\"foo`n`nbar`n`nbaz\")' + '[Console]::Error.Write("foo`n`nbar`n`nbaz"); ', + '[Console]::Error.Write("middle"); ', + '[Console]::Error.Write("foo`n`nbar`n`nbaz")' )) $out = & $powershell -noprofile -command $command 2>&1 From 310c7310dce2c64218123468fbcfcb7a4c865cea Mon Sep 17 00:00:00 2001 From: James Truher Date: Thu, 11 Mar 2021 14:29:30 -0800 Subject: [PATCH 13/22] Fix test by removing (now) extraneous extra quotes Fix additional codefactor issues --- .../engine/NativeCommandParameterBinder.cs | 10 +++++----- test/powershell/Host/Startup.Tests.ps1 | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index 07873981393..321c3a8cb5a 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -164,10 +164,11 @@ internal string[] ArgumentList /// Add an argument to the ArgumentList. /// We may need to construct the argument out of the parameter text and the argument /// in the case that we have a parameter that appears as "-switch:value". + /// The parameter associated with the operation. + /// The value used with parameter. /// internal void AddToArgumentList(CommandParameterInternal parameter, string argument) { - if (parameter.ParameterNameSpecified && parameter.ParameterText.EndsWith(":")) { if (argument != parameter.ParameterText) @@ -179,7 +180,6 @@ internal void AddToArgumentList(CommandParameterInternal parameter, string argum { _argumentList.Add(argument); } - } private List _argumentList = new List(); @@ -221,7 +221,7 @@ internal bool UseArgumentList /// each of which will be stringized. /// /// Execution context instance. - /// The parameter we're looking at + /// The parameter associated with the operation. /// The object to append. /// If the argument was an array literal, the Ast, otherwise null. /// True if the argument occurs after --%. @@ -230,7 +230,7 @@ private void AppendOneNativeArgument(ExecutionContext context, CommandParameterI { IEnumerator list = LanguagePrimitives.GetEnumerator(obj); - Diagnostics.Assert((argArrayAst == null) || obj is object[] && ((object[])obj).Length == argArrayAst.Elements.Count, "array argument and ArrayLiteralAst differ in number of elements"); + Diagnostics.Assert((argArrayAst == null) || (obj is object[] && ((object[])obj).Length == argArrayAst.Elements.Count), "array argument and ArrayLiteralAst differ in number of elements"); int currentElement = -1; string separator = string.Empty; @@ -345,7 +345,7 @@ private void AppendOneNativeArgument(ExecutionContext context, CommandParameterI /// On Unix, do globbing as appropriate, otherwise just append . /// /// The argument that possibly needs expansion. - /// The parameter we're looking at + /// The parameter associated with the operation. /// Bare, SingleQuoted, or DoubleQuoted. private void PossiblyGlobArg(string arg, CommandParameterInternal parameter, StringConstantType stringConstantType) { diff --git a/test/powershell/Host/Startup.Tests.ps1 b/test/powershell/Host/Startup.Tests.ps1 index 6ce9dfc463d..3c49dbf3152 100644 --- a/test/powershell/Host/Startup.Tests.ps1 +++ b/test/powershell/Host/Startup.Tests.ps1 @@ -90,7 +90,7 @@ Describe "Validate start of console host" -Tag CI { Remove-Item $profileDataFile -Force } - $loadedAssemblies = & "$PSHOME/pwsh" -noprofile -command '([System.AppDomain]::CurrentDomain.GetAssemblies()).manifestmodule | Where-Object { $_.Name -notlike ""<*>"" } | ForEach-Object { $_.Name }' + $loadedAssemblies = & "$PSHOME/pwsh" -noprofile -command '([System.AppDomain]::CurrentDomain.GetAssemblies()).manifestmodule | Where-Object { $_.Name -notlike "<*>" } | ForEach-Object { $_.Name }' } It "No new assemblies are loaded" { From 8ebabbe4230d549545f46ef623cef7aefa3fe9ea Mon Sep 17 00:00:00 2001 From: James Truher Date: Sun, 14 Mar 2021 00:26:19 -0800 Subject: [PATCH 14/22] fix codefactor issue --- .../engine/NativeCommandParameterBinder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index 321c3a8cb5a..baf910371b6 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -164,9 +164,9 @@ internal string[] ArgumentList /// Add an argument to the ArgumentList. /// We may need to construct the argument out of the parameter text and the argument /// in the case that we have a parameter that appears as "-switch:value". + /// /// The parameter associated with the operation. /// The value used with parameter. - /// internal void AddToArgumentList(CommandParameterInternal parameter, string argument) { if (parameter.ParameterNameSpecified && parameter.ParameterText.EndsWith(":")) From 0331d3db432ae9ae1a12b69eb0caa66da89ce337 Mon Sep 17 00:00:00 2001 From: James Truher Date: Sun, 14 Mar 2021 00:37:03 -0800 Subject: [PATCH 15/22] Suppress warning about using global variables In this case, we want to be sure that the global variable for the new native argument handler provides the correct message when assigning an invalid value --- .../Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index 0857883685d..7cac5276518 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -1,5 +1,7 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. +[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidGlobalVars", "")] +param() Describe "Will error correctly if an attempt to set variable to improper value" { It "will error when setting variable incorrectly" { if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled ) { From 4ae012dc1c9a8a62a5e22d2b6af352963213685e Mon Sep 17 00:00:00 2001 From: James Truher Date: Tue, 16 Mar 2021 16:09:13 -0700 Subject: [PATCH 16/22] Address some PR feedback Add a test to handle embedded single quotes. Remove duplicate experimental feature. --- .../engine/InitialSessionState.cs | 6 ------ .../NativeExecution/NativeCommandArguments.Tests.ps1 | 5 +++-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/System.Management.Automation/engine/InitialSessionState.cs b/src/System.Management.Automation/engine/InitialSessionState.cs index 9dca181c7c8..299034b5b0a 100644 --- a/src/System.Management.Automation/engine/InitialSessionState.cs +++ b/src/System.Management.Automation/engine/InitialSessionState.cs @@ -4577,12 +4577,6 @@ .ForwardHelpCategory Cmdlet ScopedItemOptions.None), // End: Variables which control remoting behavior - new SessionStateVariableEntry( - SpecialVariables.PSSessionApplicationName, - "wsman", - RemotingErrorIdStrings.PSSessionAppName, - ScopedItemOptions.None), - #region Platform new SessionStateVariableEntry( SpecialVariables.IsLinux, diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index 7cac5276518..2d6a89c3239 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -24,8 +24,8 @@ foreach ( $argumentListValue in "Standard","Legacy" ) { # to the testexe native command and looking at how it got the arguments. It "Should handle quoted spaces correctly (ArgumentList=${PSNativeCommandArgumentPassing})" { $a = 'a"b c"d' - $lines = testexe -echoargs $a 'a"b c"d' a"b c"d - $lines.Count | Should -Be 3 + $lines = testexe -echoargs $a 'a"b c"d' a"b c"d "a'b c'd" + $lines.Count | Should -Be 4 if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled -and $PSNativeCommandArgumentPassing -ne "Legacy" ) { $lines[0] | Should -BeExactly 'Arg 0 is ' $lines[1] | Should -BeExactly 'Arg 1 is ' @@ -35,6 +35,7 @@ foreach ( $argumentListValue in "Standard","Legacy" ) { $lines[1] | Should -BeExactly 'Arg 1 is ' } $lines[2] | Should -BeExactly 'Arg 2 is ' + $lines[3] | Should -BeExactly 'Arg 3 is ' } # In order to pass '"' characters so they are actually part of command line From ab67f99f8af5a77c6c2c22a9cd2bcf9e969bac8a Mon Sep 17 00:00:00 2001 From: James Truher Date: Thu, 18 Mar 2021 12:15:12 -0700 Subject: [PATCH 17/22] Address some concerns in PR review Added test for empty string command. Protected the call to the pager behind a Get-ExperimentalFeature call so the old path is taken. Made the experimental feature a const to follow guidance. --- .../ExperimentalFeature.cs | 3 ++- .../engine/InitialSessionState.cs | 20 +++++++++++++++---- test/powershell/Host/ConsoleHost.Tests.ps1 | 5 +++++ 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs index a68b1208294..7e09e9997fa 100644 --- a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs +++ b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs @@ -22,6 +22,7 @@ public class ExperimentalFeature internal const string EngineSource = "PSEngine"; internal const string PSAnsiProgressFeatureName = "PSAnsiProgress"; + internal const string PSNativeCommandArgumentPassingFeatureName = "PSNativeCommandArgumentPassing"; #endregion @@ -137,7 +138,7 @@ static ExperimentalFeature() name: PSAnsiProgressFeatureName, description: "Enable lightweight progress bar that leverages ANSI codes for rendering"), new ExperimentalFeature( - name: "PSNativeCommandArgumentPassing", + name: PSNativeCommandArgumentPassingFeatureName, description: "Use ArgumentList when invoking a native command"), }; EngineExperimentalFeatures = new ReadOnlyCollection(engineFeatures); diff --git a/src/System.Management.Automation/engine/InitialSessionState.cs b/src/System.Management.Automation/engine/InitialSessionState.cs index 299034b5b0a..c7e849a5110 100644 --- a/src/System.Management.Automation/engine/InitialSessionState.cs +++ b/src/System.Management.Automation/engine/InitialSessionState.cs @@ -4298,7 +4298,13 @@ .FORWARDHELPCATEGORY Cmdlet } else { $pagerCommand = 'less' - $pagerArgs = '-s','-P','Page %db?B of %D:.\. Press h for help or q to quit\.' + # PSNativeCommandArgumentPassing arguments should be constructed differently. + if ((Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled) { + $pagerArgs = '-s','-P','Page %db?B of %D:.\. Press h for help or q to quit\.' + } + else { + $pagerArgs = '-Ps""Page %db?B of %D:.\. Press h for help or q to quit\.$""' + } } # Respect PAGER environment variable which allows user to specify a custom pager. @@ -4338,10 +4344,16 @@ .FORWARDHELPCATEGORY Cmdlet $consoleWidth = [System.Math]::Max([System.Console]::WindowWidth, 20) if ($pagerArgs) { - # Supply pager arguments to an application without any PowerShell parsing of the arguments. + # Start the pager arguments directly if the PSNativeCommandArgumentPassing feature is enabled. + # Otherwise, supply pager arguments to an application without any PowerShell parsing of the arguments. # Leave environment variable to help user debug arguments supplied in $env:PAGER. - # $env:__PSPAGER_ARGS = $pagerArgs - $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand $pagerArgs + if ((Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled) { + $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand $pagerArgs + } + else { + $env:__PSPAGER_ARGS = $pagerArgs + $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand --% %__PSPAGER_ARGS% + } } else { $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand diff --git a/test/powershell/Host/ConsoleHost.Tests.ps1 b/test/powershell/Host/ConsoleHost.Tests.ps1 index 0cdb393c06a..e57b6357c70 100644 --- a/test/powershell/Host/ConsoleHost.Tests.ps1 +++ b/test/powershell/Host/ConsoleHost.Tests.ps1 @@ -240,6 +240,11 @@ Describe "ConsoleHost unit tests" -tags "Feature" { $LASTEXITCODE | Should -Be 64 } + It "Empty space command should succeed" { + & $powershell -noprofile -c '' | Should -BeNullOrEmpty + $LASTEXITCODE | Should -Be 0 + } + It "Whitespace command should succeed" { & $powershell -noprofile -c ' ' | Should -BeNullOrEmpty $LASTEXITCODE | Should -Be 0 From ee1f5f52e264f82780395379d84f3c053e0002c1 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 22 Mar 2021 16:45:20 -0700 Subject: [PATCH 18/22] Add a windbg example for native argument passing --- .../NativeExecution/NativeCommandArguments.Tests.ps1 | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index 2d6a89c3239..e58f5e1ca23 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -74,6 +74,13 @@ foreach ( $argumentListValue in "Standard","Legacy" ) { } } + It "Should handle arguments that include commas without spaces (windbg example)" { + $lines = testexe -echoargs -k com:port=\\devbox\pipe\debug,pipe,resets=0,reconnect + $lines.Count | Should -Be 2 + $line[0] | Should -BeExactly "Arg 0 is <-k>" + $line[1] | Should -BeExactly "Arg 1 is " + } + It "Should handle PowerShell arrays with or without spaces correctly (ArgumentList=${PSNativeCommandArgumentPassing}): " -TestCases @( @{arguments = "1,2"; expected = @("1,2")} @{arguments = "1,2,3"; expected = @("1,2,3")} From 7ee22e2bc91c4bcd2c619963a6c39e31f5ac0042 Mon Sep 17 00:00:00 2001 From: James Truher Date: Tue, 23 Mar 2021 12:58:47 -0700 Subject: [PATCH 19/22] Fix typo in test. Optimize help function to not invoke Get-ExperimentalFeature. Add additional test for variation in Windows parameter style. --- .../engine/InitialSessionState.cs | 4 ++-- .../NativeExecution/NativeCommandArguments.Tests.ps1 | 11 +++++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/System.Management.Automation/engine/InitialSessionState.cs b/src/System.Management.Automation/engine/InitialSessionState.cs index c7e849a5110..3294c19ac97 100644 --- a/src/System.Management.Automation/engine/InitialSessionState.cs +++ b/src/System.Management.Automation/engine/InitialSessionState.cs @@ -4299,7 +4299,7 @@ .FORWARDHELPCATEGORY Cmdlet else { $pagerCommand = 'less' # PSNativeCommandArgumentPassing arguments should be constructed differently. - if ((Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled) { + if ($EnabledExperimentalFeatures -contains 'PSNativeCommandArgumentPassing') { $pagerArgs = '-s','-P','Page %db?B of %D:.\. Press h for help or q to quit\.' } else { @@ -4347,7 +4347,7 @@ .FORWARDHELPCATEGORY Cmdlet # Start the pager arguments directly if the PSNativeCommandArgumentPassing feature is enabled. # Otherwise, supply pager arguments to an application without any PowerShell parsing of the arguments. # Leave environment variable to help user debug arguments supplied in $env:PAGER. - if ((Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled) { + if ($EnabledExperimentalFeatures -contains 'PSNativeCommandArgumentPassing') { $help | Out-String -Stream -Width ($consoleWidth - 1) | & $pagerCommand $pagerArgs } else { diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index e58f5e1ca23..c1e3a0238bf 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -77,8 +77,15 @@ foreach ( $argumentListValue in "Standard","Legacy" ) { It "Should handle arguments that include commas without spaces (windbg example)" { $lines = testexe -echoargs -k com:port=\\devbox\pipe\debug,pipe,resets=0,reconnect $lines.Count | Should -Be 2 - $line[0] | Should -BeExactly "Arg 0 is <-k>" - $line[1] | Should -BeExactly "Arg 1 is " + $lines[0] | Should -BeExactly "Arg 0 is <-k>" + $lines[1] | Should -BeExactly "Arg 1 is " + } + + It "Should handle DOS style arguments" { + $lines = testexe -echoargs /arg1 /c:"a string" + $lines.Count | Should -Be 2 + $lines[0] | Should -BeExactly "Arg 0 is " + $lines[1] | Should -BeExactly "Arg 1 is " } It "Should handle PowerShell arrays with or without spaces correctly (ArgumentList=${PSNativeCommandArgumentPassing}): " -TestCases @( From 4eae549af96c6dcc8418d49744d6f57b56f92054 Mon Sep 17 00:00:00 2001 From: James Truher Date: Wed, 24 Mar 2021 11:03:07 -0700 Subject: [PATCH 20/22] Change tests to use EnabledExperimentalFeatures rather than invoking Get-ExperimentalFeature --- .../NativeExecution/NativeCommandArguments.Tests.ps1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 index c1e3a0238bf..b279f04c9e8 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1 @@ -4,7 +4,7 @@ param() Describe "Will error correctly if an attempt to set variable to improper value" { It "will error when setting variable incorrectly" { - if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled ) { + if ($EnabledExperimentalFeatures -contains 'PSNativeCommandArgumentPassing') { { $global:PSNativeCommandArgumentPassing = "zzz" } | Should -Throw -ExceptionType System.Management.Automation.ArgumentTransformationMetadataException } else { @@ -26,7 +26,7 @@ foreach ( $argumentListValue in "Standard","Legacy" ) { $a = 'a"b c"d' $lines = testexe -echoargs $a 'a"b c"d' a"b c"d "a'b c'd" $lines.Count | Should -Be 4 - if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled -and $PSNativeCommandArgumentPassing -ne "Legacy" ) { + if (($EnabledExperimentalFeatures -contains 'PSNativeCommandArgumentPassing') -and $PSNativeCommandArgumentPassing -ne "Legacy") { $lines[0] | Should -BeExactly 'Arg 0 is ' $lines[1] | Should -BeExactly 'Arg 1 is ' } @@ -52,7 +52,7 @@ foreach ( $argumentListValue in "Standard","Legacy" ) { It "Should handle spaces between escaped quotes (ArgumentList=${PSNativeCommandArgumentPassing})" { $lines = testexe -echoargs 'a\"b c\"d' "a\`"b c\`"d" $lines.Count | Should -Be 2 - if ( (Get-ExperimentalFeature PSNativeCommandArgumentPassing).Enabled -and $PSNativeCommandArgumentPassing -ne "Legacy" ) { + if (($EnabledExperimentalFeatures -contains 'PSNativeCommandArgumentPassing') -and $PSNativeCommandArgumentPassing -ne "Legacy") { $lines[0] | Should -BeExactly 'Arg 0 is ' $lines[1] | Should -BeExactly 'Arg 1 is ' } From 6564558f6fd8c497ce972a777fba35a61debd9f4 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Wed, 31 Mar 2021 11:51:39 -0700 Subject: [PATCH 21/22] Update src/System.Management.Automation/engine/NativeCommandParameterBinder.cs Co-authored-by: Ilya --- .../engine/NativeCommandParameterBinder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index baf910371b6..238bd30aafd 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -317,7 +317,7 @@ private void AppendOneNativeArgument(ExecutionContext context, CommandParameterI if (argArrayAst != null && UseArgumentList) { // We have a literal array, so take the extent, break it on spaces and add them to the argument list. - foreach (string element in argArrayAst.Extent.Text.Split(" ", StringSplitOptions.RemoveEmptyEntries)) + foreach (string element in argArrayAst.Extent.Text.Split(' ', StringSplitOptions.RemoveEmptyEntries)) { PossiblyGlobArg(element, parameter, stringConstantType); } From 824569001fcf6b66391a9a84fcda23bff554f231 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Wed, 31 Mar 2021 11:51:56 -0700 Subject: [PATCH 22/22] Update src/System.Management.Automation/engine/NativeCommandParameterBinder.cs Co-authored-by: Ilya --- .../engine/NativeCommandParameterBinder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs index 238bd30aafd..1c74043b00a 100644 --- a/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs +++ b/src/System.Management.Automation/engine/NativeCommandParameterBinder.cs @@ -271,7 +271,7 @@ private void AppendOneNativeArgument(ExecutionContext context, CommandParameterI _arguments.Append(arg); // we need to split the argument on spaces - _argumentList.AddRange(arg.Split(" ", StringSplitOptions.RemoveEmptyEntries)); + _argumentList.AddRange(arg.Split(' ', StringSplitOptions.RemoveEmptyEntries)); } else {