Performance improvements to security checks when group policies are in effect for ExecutionPolicy by powercode · Pull Request #2588 · PowerShell/PowerShell · GitHub

Fixes #2578
When group policies are in effect for execution policy, a check is made to determine if the current process has been launched by gpscript.exe, and in that case, ignore the execution policy.
This is expensive to determine, is done over and over again, and has resulted in very long load times for scripts and modules.

One of the things that is checked (in an expensive way) is the parent id of the current process. That never changes, and is now cached.

The other change is to check once if we are launched by gpscript.exe or not and save that value.
This results in speedups in the order of a magnitude for module loads.

Hi Staffan Gustafsson (@powercode), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

Seems this should be in #if !UNIX

Yes, you are correct. Fixing.

Fixed

PG do not accept formatting without code changes. It would be good to revert this.

Unneeded empty line.

It would be good to mention here about main - use caching dramatically accelerated GetParentProcess.

Staffan Gustafsson (@powercode) - it looks like these changes cause the test failures.

Lee Holmes (@LeeHolmes) - can you review these changes?

It is my bad. Fixing now.

s_currentProcessId never changes. It seems unnecessary.

Good point. Refactoring...

It is there to avoid creating a new process object every time the id of the current process is needed

Agree!

I maybe stupid here but why you check s_currentProcessId instead of s_currentParentProcessId.HasValue?

Clear - "current" != "current" 😊

This conditional is the opposite of what I think you intended.

Yes, fixed in the updated pull request

Our coding style puts a newline above comment blocks, as it represents a paragraph of thought.

Fixed

Fixing

This would be an error condition (taking a snapshot with no processes), so returning 'parentPid' is misleading. Instead, we should return 0.

Good point. Fixed.

This is very close to being a duplication of GetParentProcess(). I think it'd be OK to add the caching logic to GetParentProcessIdNative. Also, it doesn't have the start time validation that GetParentProcess has, so we should make sure we don't expose anything that looks like a useful helper function without this validation.

Pushed new version with
private static int GetParentProcessIdCandidate
instead of
internal static int GetParentProcessId

Staffan Gustafsson (@powercode) Can you please rebase this PR to the latest from master and push an update? It looks like it is ready to merge and just needs final code review sign off.

Lee Holmes (@LeeHolmes) - Please take another look at this PR and provide feedback as appropriate.
Paul Higinbotham (@PaulHigin) - I am adding you as Lee's backup if he is too busy or if you are curious

I looked through the code changes and comments. I agree with Lee's comments but I notice that two have not yet been addressed:

GetParentProcessIdNative() helper method is not needed and can be merged with GetParentProcessId() method.
The GetParentProcessId() method should include the process start time check and return '0' if start time.
Actually I don't see a need for GetParentProcessId() at all, since it is always used within GetParentProcess() and nowhere else. We can just add the s_currentParentProcessId caching in GetParentProcess().

In certain scenarios, like a domain joined machine with an executionpolicy set via GPO, a lot of checks are done for the ExecutionPolicy, and this is in the hot path.

It is expensive to determine if a script was run as a result of applying a group policy. This does not change during the lifetime of a process. This only affect the scenario when an execution policy has been set by a group policy.

Paul Higinbotham (@PaulHigin) We now have an implementation of GetParentPid available. Using it instead, and inlining the caching as suggested.

LGTM

…n effect for ExecutionPolicy (PowerShell#2588) * Caching the parent id of the current process In certain scenarios, like a domain joined machine with an executionpolicy set via GPO, a lot of checks are done for the ExecutionPolicy, and this is in the hot path. * Caching HasGPScriptParent. It is expensive to determine if a script was run as a result of applying a group policy. This does not change during the lifetime of a process. This only affect the scenario when an execution policy has been set by a group policy. * Using available Microsoft.PowerShell.ProcessCodeMethods.GetParentPid

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Nov 2, 2016

Staffan Gustafsson (powercode) changed the title Performance improvements when group policies are in effect for ExecutionPolicy Performance improvements to security checks when group policies are in effect for ExecutionPolicy Nov 2, 2016

Ilya (iSazonov) reviewed Nov 2, 2016

View reviewed changes

Staffan Gustafsson (powercode) force-pushed the master branch 2 times, most recently from 17cfe31 to 8ad2783 Compare November 2, 2016 12:54

Ilya (iSazonov) reviewed Nov 2, 2016

View reviewed changes

Staffan Gustafsson (powercode) force-pushed the master branch 5 times, most recently from 2721367 to d70dd08 Compare November 2, 2016 14:29

Staffan Gustafsson (powercode) mentioned this pull request Nov 2, 2016

Slow module load caused by inefficient PSUtils.GetParentProcess #2578

Closed

Jason Shirk (lzybkr) assigned Lee Holmes (LeeHolmes) Nov 2, 2016

Staffan Gustafsson (powercode) force-pushed the master branch from d70dd08 to 5df885b Compare November 2, 2016 21:36

Ilya (iSazonov) reviewed Nov 3, 2016

View reviewed changes

Staffan Gustafsson (powercode) force-pushed the master branch from 5df885b to 76bb9ad Compare November 3, 2016 07:32

Ilya (iSazonov) reviewed Nov 3, 2016

View reviewed changes

PowerShellTeam added the Review - Needed label Nov 10, 2016

Lee Holmes (LeeHolmes) reviewed Nov 14, 2016

View reviewed changes

Staffan Gustafsson (powercode) force-pushed the master branch from 76bb9ad to f49c2a1 Compare November 14, 2016 21:46

Lee Holmes (LeeHolmes) reviewed Nov 14, 2016

View reviewed changes

Staffan Gustafsson (powercode) force-pushed the master branch from 1ef7811 to a68a21c Compare November 14, 2016 21:58

Lee Holmes (LeeHolmes) reviewed Nov 15, 2016

View reviewed changes

Staffan Gustafsson (powercode) force-pushed the master branch 2 times, most recently from fce53f2 to 16f0e24 Compare November 15, 2016 08:00

Staffan Gustafsson (powercode) force-pushed the master branch 2 times, most recently from d7baf80 to 636a14c Compare November 20, 2016 20:05

PowerShellTeam added the Review - Needed The PR is being reviewed label Nov 22, 2016

Hemant Mahawar (HemantMahawar) removed the Review-Needed label Dec 1, 2016

Jason Shirk (lzybkr) assigned Mike Richmond (mirichmo) and unassigned Lee Holmes (LeeHolmes) Mar 29, 2017

Mike Richmond (mirichmo) requested a review from Paul Higinbotham (PaulHigin) March 30, 2017 01:07

Paul Higinbotham (PaulHigin) suggested changes Mar 31, 2017

View reviewed changes

Staffan Gustafsson added 3 commits April 19, 2017 08:06

Caching the parent id of the current process

cadb0a5

In certain scenarios, like a domain joined machine with an executionpolicy set via GPO, a lot of checks are done for the ExecutionPolicy, and this is in the hot path.

Caching HasGPScriptParent.

1ad62ce

It is expensive to determine if a script was run as a result of applying a group policy. This does not change during the lifetime of a process. This only affect the scenario when an execution policy has been set by a group policy.

Using available Microsoft.PowerShell.ProcessCodeMethods.GetParentPid

f807df7

Staffan Gustafsson (powercode) force-pushed the master branch from 636a14c to f807df7 Compare April 19, 2017 07:08

Paul Higinbotham (PaulHigin) approved these changes Apr 20, 2017

View reviewed changes

Mike Richmond (mirichmo) merged commit f0eda03 into PowerShell:master Apr 26, 2017

Performance improvements to security checks when group policies are in effect for ExecutionPolicy by powercode · Pull Request #2588 · PowerShell/PowerShell · GitHub

Uh oh!

Performance improvements to security checks when group policies are in effect for ExecutionPolicy - #2588

Performance improvements to security checks when group policies are in effect for ExecutionPolicy#2588Mike Richmond (mirichmo) merged 3 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom powercode:masterpowercode/PowerShell:masterCopy head branch name to clipboard

Conversation

Staffan Gustafsson (powercode) commented Nov 2, 2016

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jason Shirk (lzybkr) commented Nov 2, 2016

Uh oh!

Staffan Gustafsson (powercode) commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Staffan Gustafsson (powercode) commented Nov 14, 2016

Uh oh!

Lee Holmes (LeeHolmes) Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Staffan Gustafsson (powercode) commented Nov 15, 2016

Uh oh!

Mike Richmond (mirichmo) commented Mar 30, 2017

Uh oh!

Paul Higinbotham (PaulHigin) left a comment

Choose a reason for hiding this comment

Uh oh!

Staffan Gustafsson (powercode) commented Apr 19, 2017

Uh oh!

Paul Higinbotham (PaulHigin) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Performance improvements to security checks when group policies are in effect for ExecutionPolicy#2588
Mike Richmond (mirichmo) merged 3 commits into
PowerShell:masterfrom
powercode:master

Lee Holmes (LeeHolmes) Nov 14, 2016 •
edited
Loading