@PowerShell/wg-powershell-cmdlets discussed the issue and agreed that in the credential case, it's ok to not support the job object case with child processes

Docs say:

All processes associated with a job must run in the same session.

So I believe it is safe to remove try-catch and immediately call process.WaitForExit() if Credential parameter present.

Ilya (@iSazonov) yes, that makes sense. I'll update the code.

This PR has 19 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Label      : Extra Small
Size       : +11 -8
Percentile : 7.6%

Total files changed: 1

Change summary by file extension:
.cs : +11 -8

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

Per the docs, job objects can only contain processes within the same session,

That may be true but specifying a credential does not result in the process being run in a new session, it's still the same one. A new session happens when you do something like log into RDP as another user, or run something from a service which is in session 0. Ultimately this isn't the problem here.

The problem that you get today with Start-Process -Credential -Wait is due to PowerShell not using the hProcess object returned by CreateProcessWithLogon on the AssignProcessToJobObject stage. What is happening is:

• PowerShell sees a -Credential was specified
• It calls CreateProcessWithLogonW
  • This returns a process and thread HANDLE which the caller has an ALL_ACCESS mask on
  • Even though normally it doesn't have permissions to open the process handle, this one is special but it's not being used
  • There's actually a handle leak here as the code currently doesn't even close the process and thread handle
• It uses the process id returned from the above to call Process.GetProcessById <-- this is the problem
• It uses that process object and it's internal Handle when calling AssignProcessToJobObject
  • Because dotnet is trying to open a new process handle rather than use the ALL_ACCESS handle returned by CreateProcessWithLogonW this will fail for users who don't have permission to open the process (non-admins)

Now this actually works today when you are running as admin, it will wait for the process, as well as any child processes the new process spawns as expected. Where this fails is if you try and run it as a non-administrator as while the user was able to spawn the object, process permissions on the new one will not allow the caller to get the Handle needed for AssignProcessToJobObject. I've mentioned this problem and possible solutions for it in #17033 (comment).

The problem with this fix is that while it might work in some scenarios it is technically a breaking change as -Wait will no longer wait for any sub processes the new one spawns.

Just to show that this works even for non-admins if done correctly you can run this code.

# Uses Ctypes - https://www.powershellgallery.com/packages/Ctypes/0.1.0

ctypes_struct STARTUPINFOW -CharSet Unicode {
    [int]$CB
    [MarshalAs('LPWStr')][string]$Reserved
    [MarshalAs('LPWStr')][string]$Desktop
    [MarshalAs('LPWStr')][string]$Title
    [int]$X
    [int]$Y
    [int]$XSize
    [int]$YSize
    [int]$XCountChars
    [int]$YCountChars
    [int]$FillAttribute
    [int]$Flags
    [short]$ShowWindow
    [short]$Reserved2
    [IntPtr]$Reserved3
    [IntPtr]$StdInput
    [IntPtr]$StdOutput
    [IntPtr]$StdError
}

ctypes_struct PROCESS_INFORMATION -LayoutKind Sequential {
    [IntPtr]$Process
    [IntPtr]$Thread
    [int]$Pid
    [int]$Tid
}

[Flags()] enum ProcessCreationFlags {
    NONE = 0x00000000
    CREATE_SUSPENDED = 0x00000004
    CREATE_NEW_CONSOLE = 0x00000010
    CREATE_UNICODE_ENVIRONMENT = 0x00000400
}

$si = [STARTUPINFOW]@{
    CB = [System.Runtime.InteropServices.Marshal]::SizeOf([Type][STARTUPINFOW])
}
$pi = [PROCESS_INFORMATION]::new()

$advapi32 = New-CtypesLib Advapi32.dll
$kernel32 = New-CtypesLib Kernel32.dll

$commandLine = [System.Text.StringBuilder]::new("C:\Windows\System32\cmd.exe")
$res = $advapi32.SetLastError().CharSet('Unicode').CreateProcessWithLogonW[bool](
    "vagrant",
    $null,
    "vagrant",
    0,
    $commandLine.ToString(),
    $commandLine,
    [int][ProcessCreationFlags]'CREATE_SUSPENDED, CREATE_NEW_CONSOLE, CREATE_UNICODE_ENVIRONMENT',
    $null,
    "C:\Windows",
    [ref]$si,
    [ref]$pi
)
if (-not $res) {
    throw [System.ComponentModel.Win32Exception]$advapi32.LastError
}

$job = $kernel32.SetLastError().CreateJobObjectW[IntPtr]($null, "MyJob")
if ($job -eq [IntPtr]::Zero) {
    throw [System.ComponentModel.Win32Exception]$kernel32.LastError
}

$res = $kernel32.SetLastError().AssignProcessToJobObject[bool]($job, $pi.Process)
if (-not $res) {
    throw [System.ComponentModel.Win32Exception]$kernel32.LastError
}

$res = $kernel32.SetLastError().ResumeThread($pi.Thread)
if ($res -eq -1) {
    throw [System.ComponentModel.Win32Exception]$kernel32.LastError
}

$kernel32.CloseHandle[void]($pi.Process)
$kernel32.CloseHandle[void]($pi.Thread)
$kernel32.CloseHandle[void]($job)

I even do this in my ProcessEx module with no problems.

Jordan Borean (@jborean93) Can you create a new issue about this?

Paul Higinbotham (@PaulHigin) there is one already #17033. Why would we merge a change where it changes the existing behaiour? It should be fixed properly in the first place.

I'm a bit disappointed this was merged before someone actually read and addressed my last comment. Especially since PowerShell has traditionally tried to shy away from behaviour changes. It's especially concerning that a change like this was merged without:

• Any tests to assert this actual behaviour or that it was fixed
• Any documentation/changelog details for people to see a breaking change actually occurred

The last point also ties into issues I had with the 7.3 release where there were a few breaking/behaviour changes but no real way for people to understand what that might be until they upgraded and things failed.

Jordan Borean (@jborean93) would you be willing to submit a PR to fix this to work with a job object?

Opened #19096, just need to figure out a good way to test it out.

This issue has been resolved as implemented (via #19082)

Thomas Lee (@doctordns) What do you mean?

This issue was raised to the cmdlet working group, and we felt the issue was concluded via #19082. Steve Lee (@SteveL-MSFT) agreed, so I labeled this issue as resolved-fixed. Is there a problem?

You write in 19082 - it is not issue, it is PR.

🎉v7.4.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes