Skip to content

Add fix for nested (Debug-Runspace) debugging on non-Windows platforms and re-enable Windows identity impersonation flow#8451

Merged
daxian-dbw merged 8 commits intoPowerShell:masterfrom
PaulHigin:fix-unix-ipc-debugging
Dec 19, 2018
Merged

Add fix for nested (Debug-Runspace) debugging on non-Windows platforms and re-enable Windows identity impersonation flow#8451
daxian-dbw merged 8 commits intoPowerShell:masterfrom
PaulHigin:fix-unix-ipc-debugging

Conversation

@PaulHigin
Copy link
Contributor

PR Summary

Nested runspace debugging is now supported on non-Windows platforms after Enter-PSHostProcess was added. However, nested debugging was broken because the Windows impersonation code portion was not fully ifdefed out.

This change removes Windows only impersonation code for cross platform builds. It also updates the Windows impersonation flow code to work with dotNet Core.

Repro:

# On one PowerShell shell instance run:
$pid
14931

$ps = [powershell]::Create()
$ps.AddScript('1..20 | % { sleep 1; "Output $_" }').Invoke()

# On second PowerShell shell run:
Enter-PSHostProcess -Id 14931
[Process:14931]: PS > Debug-Runspace -Id 2

Expect:
Should be able to run a nested debug session on script.

Result:
Cannot debug script due to "Unsupported platform error".

PR Checklist

@PaulHigin PaulHigin added Issue-Bug Issue has been identified as a bug in the product OS-macOS OS-Linux WG-Remoting PSRP issues with any transport layer labels Dec 11, 2018
@PaulHigin PaulHigin requested a review from TravisEz13 December 11, 2018 18:35
@PaulHigin PaulHigin requested a review from mirichmo as a code owner December 11, 2018 18:35
@PaulHigin PaulHigin removed the request for review from mirichmo December 11, 2018 21:32
@TravisEz13 TravisEz13 added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Dec 11, 2018
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

@TravisEz13 @PaulHigin We ran into an issue before with WindowsIdentity.RunImpersonated(safeAccessTokenHandle, action) on Windows when porting powershell to CoreCLR, and it caused powershell to crash. We (us and .NET Core team) never figured out what was the root cause. I don't know if that issue is gone with the latest .NET Core. I will forward you the email thread on this.

@TravisEz13 TravisEz13 changed the title Add fix for nested (Debug-Runspace) debugging on non-Windows platforms WIP: Add fix for nested (Debug-Runspace) debugging on non-Windows platforms Dec 13, 2018
@PaulHigin PaulHigin requested a review from BrucePay as a code owner December 14, 2018 17:29
@PaulHigin
Copy link
Contributor Author

Looking through the code I realized there were many more places in the engine where Windows identity impersonation flow was disabled. This is needed to support rare but important Windows scenarios. I submitted a commit that re-enables these using the new dotNet RunImpersonated API.

I will also update the title to reflect these new changes.

@PaulHigin PaulHigin changed the title WIP: Add fix for nested (Debug-Runspace) debugging on non-Windows platforms Add fix for nested (Debug-Runspace) debugging on non-Windows platforms and re-enable Windows identity impersonation flow Dec 14, 2018
@PaulHigin
Copy link
Contributor Author

@daxian-dbw All tests have passed. Can this be merged?

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw daxian-dbw merged commit bc29337 into PowerShell:master Dec 19, 2018
@PaulHigin PaulHigin deleted the fix-unix-ipc-debugging branch December 19, 2018 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log Issue-Bug Issue has been identified as a bug in the product OS-Linux OS-macOS WG-Remoting PSRP issues with any transport layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants