FFFF Make 'Start-Job' throw terminating error when PowerShell is being hosted by daxian-dbw · Pull Request #9128 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Make 'Start-Job' throw terminating error when PowerShell is being hosted #9128

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.

Sign up for GitHub

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

Merged
adityapatwardhan merged 3 commits into from
Mar 14, 2019
Merged

Make 'Start-Job' throw terminating error when PowerShell is being hosted #9128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65baccc
Make 'Start-Job' throw terminating error when PowerShell is being hosted
daxian-dbw Mar 12, 2019
3ce785d
Add xUnit test
daxian-dbw Mar 12, 2019
e08f123
[Feature] Fix Codacy issue
daxian-dbw Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 11 additions & 26 deletions src/System.Management.Automation/engine/hostifaces/PowerShellProcessInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,18 @@ namespace System.Management.Automation.Runspaces
/// </summary>
public sealed class PowerShellProcessInstance : IDisposable
{
#region Private Members
#region Fields

private readonly ProcessStartInfo _startInfo;
private static readonly string s_PSExePath;
private RunspacePool _runspacePool;
private readonly object _syncObject = new object();
private bool _started;
private bool _isDisposed;
private bool _processExited;

#endregion Private Members
internal static readonly string PwshExePath;

#endregion Fields

#region Constructors

Expand All @@ -33,11 +34,9 @@ public sealed class PowerShellProcessInstance : IDisposable
static PowerShellProcessInstance()
{
#if UNIX
s_PSExePath = Path.Combine(Utils.DefaultPowerShellAppBase,
"pwsh");
PwshExePath = Path.Combine(Utils.DefaultPowerShellAppBase, "pwsh");
#else
s_PSExePath = Path.Combine(Utils.DefaultPowerShellAppBase,
"pwsh.exe");
PwshExePath = Path.Combine(Utils.DefaultPowerShellAppBase, "pwsh.exe");
#endif
}

Expand All @@ -49,16 +48,16 @@ static PowerShellProcessInstance()
/// <param name="useWow64"></param>
public PowerShellProcessInstance(Version powerShellVersion, PSCredential credential, ScriptBlock initializationScript, bool useWow64)
{
string psWow64Path = s_PSExePath;
string psWow64Path = PwshExePath;

if (useWow64)
{
string procArch = Environment.GetEnvironmentVariable("PROCESSOR_ARCHITECTURE");

if ((!string.IsNullOrEmpty(procArch)) && (procArch.Equals("amd64", StringComparison.OrdinalIgnoreCase) ||
procArch.Equals("ia64", StringComparison.OrdinalIgnoreCase)))
if (!string.IsNullOrEmpty(procArch) && (procArch.Equals("amd64", StringComparison.OrdinalIgnoreCase) ||
Copy link
Member
@SteveL-MSFT SteveL-MSFT Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adityapatwardhan does this need to be updated for ARM?

Copy link
Member
@adityapatwardhan adityapatwardhan Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but what is the expected behavior on using useWow64 when run on arm64?

Copy link
Member Author
@daxian-dbw daxian-dbw Mar 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should support the -RunAs32 parameter on Start-Job anymore. To start a 32-bit process pwsh, it requires the win-32 pwsh package to be installed, and there is no way to guarantee that, and I don't think we should even bother to search for that installation.

If you look at the current implementation closely, you will find that when useWow64 is true, we are actually using the same default pwsh under $PSHOME. So it's already broken. No one reports it because no one is using it.

I think when -RunAs32 is specified in Start-Job, we check if the current process is 64-bit, if so, we throw terminating error. If not, then we continue to run because the underlying pwsh is a 32-bit executable anyways. But this should be done by a separate PR.

Copy link
Member
@SteveL-MSFT SteveL-MSFT Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the -RunAs32 is probably not used much anymore since the world has moved to 64-bit and makes even less sense for pwsh

Copy link
Member
@SteveL-MSFT SteveL-MSFT Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that in a separate PR and not block this one

Copy link
Member Author
@daxian-dbw daxian-dbw Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I will submit a separate PR for that.

procArch.Equals("ia64", StringComparison.OrdinalIgnoreCase)))
{
psWow64Path = s_PSExePath.ToLowerInvariant().Replace("\\system32\\", "\\syswow64\\");
psWow64Path = PwshExePath.ToLowerInvariant().Replace("\\system32\\", "\\syswow64\\");

if (!File.Exists(psWow64Path))
{
Expand All @@ -71,21 +70,7 @@ public PowerShellProcessInstance(Version powerShellVersion, PSCredential credent
}
}

#if CORECLR
string processArguments = " -s -NoLogo -NoProfile";
#else
// Adding Version parameter to powershell
// Version parameter needs to go before all other parameters because the native layer looks for Version or
// PSConsoleFile parameters before parsing other parameters.
// The other parameters get parsed in the managed layer.
Version tempVersion = powerShellVersion ?? PSVersionInfo.PSVersion;
string processArguments = string.Format(CultureInfo.InvariantCulture,
"-Version {0}", new Version(tempVersion.Major, tempVersion.Minor));

processArguments = string.Format(CultureInfo.InvariantCulture,
"{0} -s -NoLogo -NoProfile", processArguments);

#endif

if (initializationScript != null)
{
Expand All @@ -103,7 +88,7 @@ public PowerShellProcessInstance(Version powerShellVersion, PSCredential credent
// to 'false' in our use, we can ignore the 'WindowStyle' setting in the initialization below.
_startInfo = new ProcessStartInfo
{
FileName = useWow64 ? psWow64Path : s_PSExePath,
FileName = useWow64 ? psWow64Path : PwshExePath,
Arguments = processArguments,
UseShellExecute = false,
RedirectStandardInput = true,
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/remoting/commands/PSRemotingCmdlet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ public virtual Hashtable[] SSHConnection
/// </summary>
[Parameter(ValueFromPipelineByPropertyName = true,
ParameterSetName = InvokeCommandCommand.SSHHostParameterSet)]
public string Subsystem { get; set; }
public virtual string Subsystem { get; set; }

#endregion

Expand Down
36 changes: 36 additions & 0 deletions src/System.Management.Automation/engine/remoting/commands/StartJob.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System;
using System.IO;
using System.Management.Automation;
using System.Management.Automation.Internal;
using System.Management.Automation.Remoting;
Expand Down Expand Up @@ -197,6 +198,22 @@ public override string KeyFilePath
get { return null; }
}

/// <summary>
/// Suppress HostName.
/// </summary>
public override string[] HostName
{
get { return null; }
}

/// <summary>
/// Suppress Subsystem.
/// </summary>
public override string Subsystem
{
get { return null; }
}

#endregion

/// <summary>
Expand Down Expand Up @@ -548,6 +565,25 @@ public override object[] ArgumentList
/// </summary>
protected override void BeginProcessing()
{
if (!File.Exists(PowerShellProcessInstance.PwshExePath))
{
// The pwsh executable file is not found under $PSHOME.
// This means that PowerShell is currently being hosted in another application,
// and 'Start-Job' is not supported by design in that scenario.
string message = StringUtil.Format(
RemotingErrorIdStrings.IPCPwshExecutableNotFound,
PowerShellProcessInstance.PwshExePath);

var exception = new PSNotSupportedException(message);
var errorRecord = new ErrorRecord(
exception,
"IPCPwshExecutableNotFound",
ErrorCategory.NotInstalled,
PowerShellProcessInstance.PwshExePath);

ThrowTerminatingError(errorRecord);
}

CommandDiscovery.AutoloadModulesWithJobSourceAdapters(this.Context, this.CommandOrigin);

if (ParameterSetName == DefinitionNameParameterSet)
Expand Down
4 changes: 4 additions & 0 deletions src/System.Management.Automation/resources/RemotingErrorIdStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,10 @@ Do you want to continue?</value>
<data name="IPCSupportsOnlyDefaultAuth" xml:space="preserve">
<value>The specified authentication mechanism "{0}" is not supported. Only "{1}" is supported for this operation.</value>
</data>
<data name="IPCPwshExecutableNotFound" xml:space="preserve">
<value>The pwsh executable cannot be found at "{0}".
Note that 'Start-Job' is not supported by design in scenarios where PowerShell is being hosted in other applications. Instead, usage of the 'ThreadJob' module is recommended in such scenarios.</value>
</data>
<data name="IPCWowComponentNotPresent" xml:space="preserve">
<value>The "{0}" executable file was not found. Verify that the WOW64 feature is installed.</value>
</data>
Expand Down
26 changes: 26 additions & 0 deletions test/xUnit/csharp/test_PowerShellAPI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Management.Automation;
using Xunit;

namespace PSTests.Sequential
{
// Not static because a test requires non-const variables
public static class PowerShellHostingScenario
{
// Test that it does not throw an exception
[Fact]
public static void TestStartJobThrowTerminatingException()
{
using (var ps = PowerShell.Create())
{
ps.AddCommand("Start-Job").AddParameter("ScriptBlock", ScriptBlock.Create("1+1"));
var ex = Assert.Throws<CmdletInvocationException>(() => ps.Invoke());
Assert.IsType<PSNotSupportedException>(ex.InnerException);
Assert.Equal("IPCPwshExecutableNotFound,Microsoft.PowerShell.Commands.StartJobCommand", ex.ErrorRecord.FullyQualifiedErrorId);
}
}
}
}
0