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

powercode · 2016-11-02T11:53:25Z

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.

msftclas · 2016-11-02T11:53:31Z

Hi @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;

iSazonov · 2016-11-02T12:23:30Z

src/System.Management.Automation/utils/PsUtils.cs

Seems this should be in #if !UNIX

Yes, you are correct. Fixing.

iSazonov · 2016-11-02T12:43:29Z

src/System.Management.Automation/utils/PsUtils.cs

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

iSazonov · 2016-11-02T12:48:08Z

src/System.Management.Automation/security/SecuritySupport.cs

Unneeded empty line.

iSazonov · 2016-11-02T12:55:15Z

src/System.Management.Automation/utils/PsUtils.cs

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

lzybkr · 2016-11-02T20:26:06Z

@powercode - it looks like these changes cause the test failures.

@LeeHolmes - can you review these changes?

powercode · 2016-11-02T21:26:26Z

It is my bad. Fixing now.

iSazonov · 2016-11-03T03:12:57Z

src/System.Management.Automation/utils/PsUtils.cs

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

iSazonov · 2016-11-03T08:28:42Z

src/System.Management.Automation/utils/PsUtils.cs

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

Clear - "current" != "current" 😊

LeeHolmes · 2016-11-14T21:30:43Z

src/System.Management.Automation/security/SecuritySupport.cs

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

Yes, fixed in the updated pull request

LeeHolmes · 2016-11-14T21:33:27Z

src/System.Management.Automation/security/SecuritySupport.cs

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

powercode · 2016-11-14T21:34:40Z

Fixing

LeeHolmes · 2016-11-14T21:47:07Z

src/System.Management.Automation/utils/PsUtils.cs

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.

LeeHolmes · 2016-11-15T01:59:46Z

src/System.Management.Automation/utils/PsUtils.cs

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.

powercode · 2016-11-15T08:10:55Z

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

mirichmo · 2017-03-30T01:07:15Z

@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.

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

PaulHigin

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.

powercode · 2017-04-19T07:10:16Z

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

PaulHigin

LGTM

msftclas added the cla-already-signed label Nov 2, 2016

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

iSazonov reviewed Nov 2, 2016

View reviewed changes

src/System.Management.Automation/security/SecuritySupport.cs Outdated

Copy link

Collaborator

iSazonov Nov 2, 2016

Choose a reason for hiding this comment

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

Unneeded empty line.

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

iSazonov reviewed Nov 2, 2016

View reviewed changes

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

powercode mentioned this pull request Nov 2, 2016

Slow module load caused by inefficient PSUtils.GetParentProcess #2578

Closed

lzybkr assigned LeeHolmes Nov 2, 2016

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

iSazonov reviewed Nov 3, 2016

View reviewed changes

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

iSazonov reviewed Nov 3, 2016

View reviewed changes

PowerShellTeam added the Review - Needed label Nov 10, 2016

LeeHolmes reviewed Nov 14, 2016

View reviewed changes

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

LeeHolmes reviewed Nov 14, 2016

View reviewed changes

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

LeeHolmes reviewed Nov 15, 2016

View reviewed changes

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

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

HemantMahawar removed the Review-Needed label Dec 1, 2016

lzybkr assigned mirichmo and unassigned LeeHolmes Mar 29, 2017

mirichmo requested a review from PaulHigin March 30, 2017 01:07

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

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

PaulHigin approved these changes Apr 20, 2017

View reviewed changes

mirichmo merged commit f0eda03 into PowerShell:master Apr 26, 2017

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 #2588

Uh oh!

Conversation

powercode commented Nov 2, 2016

Uh oh!

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!

lzybkr commented Nov 2, 2016

Uh oh!

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!

powercode commented Nov 14, 2016

Uh oh!

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!

powercode commented Nov 15, 2016

Uh oh!

mirichmo commented Mar 30, 2017

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

powercode commented Apr 19, 2017

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

LeeHolmes Nov 14, 2016 •

edited

Loading