PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.87k stars 378 forks source link

AzureRM.* modules held in use by PSSA when Cluster 0.0.0.1 is scanned, prevents uninstall #841

Open rebro-msft opened 6 years ago

rebro-msft commented 6 years ago

Repro steps

Install-Module -Name Cluster -RequiredVersion 0.0.0.1 -Repository PSGallery
# This depends on AzureRM, which in turn depends on 45 AzureRM.* modules.  
# All get downloaded at install.

Invoke-ScriptAnalyzer -Path [PathToCluster] -Recurse -Severity Error

Invoke-ScriptAnalyzer -Path [PathToCluster] -Recurse -CustomerRulePath [someCustomRulesPath]

Uninstall-Module -Name Cluster -RequiredVersion 0.0.0.1
# Succeeds

Uninstall-Module -Name AzureRM.Websites
# One of the dependencies installed.
# Fails to uninstall, says is currently in use.

Expected Behavior

I should be able to uninstall the AzureRM.* modules. I don't expect PSSA to hold on to them.

We are unable to uninstall any of the AzureRM.* modules because something in PSSA is holding on to them. This appears to only happen for the Cluster 0.0.0.1 module.

bergmeister commented 6 years ago

Can you please post the exact error message? Does that happen even if you close PowerShell and try to uninstall in a new session?

rebro-msft commented 6 years ago
PS C:\windows\system32> Uninstall-Module -Name AzureRM.Websites -RequiredVersion 4.0.0
WARNING: The version '4.0.0' of module 'AzureRM.Websites' is currently in use. Retry the operation after closing the applications.
PackageManagement\Uninstall-Package : Module 'AzureRM.Websites' is in currently in use or you don't have the required permissions.
At C:\Program Files\WindowsPowerShell\Modules\PowerShellGet\1.6.0\PSModule.psm1:2459 char:21
+ ...        $null = PackageManagement\Uninstall-Package @PSBoundParameters
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (Microsoft.Power...ninstallPackage:UninstallPackage) [Uninstall-Package], Exception
    + FullyQualifiedErrorId : ModuleIsInUse,Uninstall-Package,Microsoft.PowerShell.PackageManagement.Cmdlets.UninstallPackage

It succeeds if I close and open a new session.

bergmeister commented 6 years ago

How is this related to PSSA? Guessing from the error message I'd say that you are not elevated (i.e. you need to launch PowerShell as an Administrator).

bergmeister commented 6 years ago

When trying to repro, I can confirm that AzureRM.Websites cannot be uninstalled, however can see no link to PSSA but rather to the AzureRM or PowerShellGet. Why do you even want to uninstall a dependency? The error that I get is:

PackageManagement\Uninstall-Package : No match was found for the specified search criteria and module names 'AzureRM.Websites'.
At C:\Program Files\WindowsPowerShell\Modules\PowerShellGet\1.0.0.1\PSModule.psm1:2194 char:21
+ ...        $null = PackageManagement\Uninstall-Package @PSBoundParameters
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (Microsoft.Power...ninstallPackage:UninstallPackage) [Uninstall-Package], Exception
    + FullyQualifiedErrorId : NoMatchFound,Microsoft.PowerShell.PackageManagement.Cmdlets.UninstallPackage

The repro steps were (with this custom rule being used) on a Win10 VM:

Install-Module -Name Cluster -RequiredVersion 0.0.0.1 -Repository PSGallery
install-module psscriptanalyzer
ipmo PSScriptAnalyzer
Invoke-ScriptAnalyzer -Path 'C:\Program Files\WindowsPowerShell\Modules\cluster' -Recurse -Severity Error
Invoke-ScriptAnalyzer -Path 'C:\Program Files\WindowsPowerShell\Modules\cluster' -Recurse -Severity Error -CustomRulePath $PathToCustomRule
Uninstall-Module -Name Cluster -RequiredVersion 0.0.0.1
Uninstall-Module -Name AzureRM.Websites
get-module -ListAvailable AzureRM.Websites # check it is really still installed -> yes it is
Uninstall-Module -Name AzureRM.Websites # fails -> try other methods (fail as well)
Uninstall-Module -Name 'AzureRM.Websites'
get-module -ListAvailable AzureRM.Websites | uninstall-module
# dump info
$error[0] | select *
writeErrorStream      : True
PSMessageDetails      :
Exception             : System.Exception: No match was found for the specified search criteria and module names 'AzureRM.Websites'.
TargetObject          : Microsoft.PowerShell.PackageManagement.Cmdlets.UninstallPackage
CategoryInfo          : ObjectNotFound: (Microsoft.Power...ninstallPackage:UninstallPackage) [Uninstall-Package], Exception
FullyQualifiedErrorId : NoMatchFound,Microsoft.PowerShell.PackageManagement.Cmdlets.UninstallPackage
ErrorDetails          :
InvocationInfo        : System.Management.Automation.InvocationInfo
ScriptStackTrace      : at Uninstall-Module<Process>, C:\Program Files\WindowsPowerShell\Modules\PowerShellGet\1.0.0.1\PSModule.psm1: line 2194
                        at <ScriptBlock>, <No file>: line 1
PipelineIterationInfo : {0, 1}
$PSVersionTable
Name                           Value
----                           -----
PSVersion                      5.1.16299.98
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.16299.98
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
rebro-msft commented 6 years ago

I was definitely running this as elevated.
When you tried to repro, did you run both a regular scan and a scan with custom rules? Or just the regular scan?
The link to PSSA is that if I don't run Invoke-ScriptAnalyzer on the Cluster module, I am able to uninstall the AzureRM.* modules just fine. To me, this indicates something in PSSA is holding on to them. And I was under the impression that PSSA should never load anything, that it's strictly static analysis. Is that correct?
It's necessary for our security scan to uninstall the dependencies to get us back to a clean state between item scans.

bergmeister commented 6 years ago

I tried to repro an a clean WinServer 2016 machine and it uninstalls fine in the same PowerShell session. Yes, I used both a normal scan and one with this custom rule (see my comment above). Maybe your specific rule is holding on to it? The only thing that I noticed is that I had to install Cluster using the -AllowNoClobber option because I had the following error when trying to install it the first time: A command with name 'Get-AzureStorageContainerAcl' is already available on this system. This module 'Azure.Storage' may override the existing commands. If you still want to install this module 'Azure.Storage', use -AllowClobber parameter. Are you using the latest version of PSSA (1.16.1)? Which OS/PowerShell version are you using? To me it sounds rather like that there are other things in your system that make it already start with a non-clean state. I would try to uninstall in order of dependencies. Should everything fail then just cleaning up the corresponding folders in "$env:ProgramFiles\WindowsPowerShell\Modules" might be a workaround. Also, if uninstalling works in a new shell then you could try to perform the uninstall ation in a new powershell process powershell -command 'Uninstall-Module -Name Cluster'

rebro-msft commented 6 years ago

Yep, I've got PSSA 1.16.1 installed.
I thought you were onto something with it being about our custom rules, so I tried the repro steps without the custom rules run. Same issue unfortunately. Interesting, it didn't prompt me for -AllowClobber. What sort of things should I be looking for on my system that would mess with PSSA like this?

bergmeister commented 6 years ago

I had a look a the Cluster installation and I can say that it declares a dependency on AzureRM, therefore you should uninstall AzureRM and not just AzureRM.Websites. Furthermore I looked into the installation folder and Cluster 0.0.0.1 ships with the .git, .vscode folder! Also, it seems that the latest version is already 0.0.0.3 and don't understand why you test an outdated version of something that seems to be pre-release. See here for the announcement 2 days ago that the PSGallery and PowerShellGet support PreRelease versioning now, which you might want to consider. The last thing that you could try is to do a remove-module psscriptanalyzer before the uninstall but that's all of the advice that I can give since I cannot really repro it, sorry. Maybe someone else can help. Disclaimer: I am not a mantainer of PSSA, only an active community member.

rebro-msft commented 6 years ago

I should clarify, I work on the PowerShell Gallery team. I implemented the prerelease versioning feature outlined in that blog post :) The PowerShell Gallery runs a security scan on all published items, regardless if prerelease or not. We have been seeing the above issue in our security scan for all versions of Cluster, not just 0.0.0.1.
Thanks for your help investigating. I think you're right, it's definitely something specific to just the Cluster module. I'll dig into it further and respond back if I find anything.

bergmeister commented 6 years ago

Haha, what a coincidence. Well in that case thanks for implementing this new feature. :-)

rebro-msft commented 6 years ago

You're welcome, hope you find it useful :-) Whether the issue is in Cluster or PSSA or our system, I think PSSA should analyze the module in a different appdomain or process. Then if something does get loaded somehow, we won't hit this issue.

rebro-msft commented 6 years ago

A coworker talked with Kapil who used to work on PSSA. His findings:

After talking to Kapil, it seems like it's the "Get-Command" calls in PSSA that are causing all the DLLs to load. We verified by (A) Running only one rule which happens to use Get-Command and (B) Running Get-Command manually and verifying the DLLs get loaded.

Blackbaud-ShaydeNofziger commented 6 years ago

@rebro-msft Sorry to revive an old thread, but we're seeing extreme slowness when analyzing script modules that are heavy on AzureRM cmdlets. Are you saying the issue stems from the Get-Command loading the DLLs?

@bergmeister We noticed a huge improvement in script analysis times (34+ minutes down to less than 3 minutes) for our module when uninstalling AzureRM from the build machine. But we're afraid that will mean we aren't getting good analysis coverage on commands it isn't able to find definitions for. Can you help me understand what we might lose in terms of script analysis quality if we decide to proceed with this workaround?

Happy to open a separate issue if requested.

bergmeister commented 6 years ago

@Blackbaud-ShaydeNofziger Although the tool is mostly static is consumes the local session to get more information about available commands, aliases or other custom definitions in the loaded session, various rules use the command/alias cache (around 5-10). Could you give an example so that I could repro your performance problem? The AzureRM module is known to be not the very best in terms of performance and will always be a bit of a bottleneck but half an hour is definitely too much. The issue of @rebro-msft is a different one, it is about DLLs being held in use that prevent uninstallation

Blackbaud-ShaydeNofziger commented 6 years ago

This is a very basic example but demonstrates what I mean - example.zip

Below are my findings with our module in question. The times I was speaking of were from our build agent, but these results are from my laptop. Please note I am unable to share the codebase in a public forum.

These tests were performed in brand new sessions without any Azure[RM] modules explicitly loaded

With AzureRM Modules Installed

C:\Repos\blackbaud-iaas-host-azure [master ≡]> Measure-Command { Invoke-ScriptAnalyzer -Path .\Blackbaud.Host.Azure\*.ps1 }

Days              : 0
Hours             : 0
Minutes           : 13
Seconds           : 32
Milliseconds      : 269
Ticks             : 8122691194
TotalDays         : 0.00940126295601852
TotalHours        : 0.225630310944444
TotalMinutes      : 13.5378186566667
TotalSeconds      : 812.2691194
TotalMilliseconds : 812269.1194

Without AzureRM Modules Installed

C:\Repos\blackbaud-iaas-host-azure [master ≡]> Measure-Command { Invoke-ScriptAnalyzer -Path .\Blackbaud.Host.Azure\*.ps1 }

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 24
Milliseconds      : 388
Ticks             : 243888910
TotalDays         : 0.000282278831018518
TotalHours        : 0.00677469194444444
TotalMinutes      : 0.406481516666667
TotalSeconds      : 24.388891
TotalMilliseconds : 24388.891

Would you like me to open a separate issue?

bergmeister commented 6 years ago

Thanks, I will take a look at it in the next days with your example and depending on the outcome we can then open a more specific issue (or multiple) once we have more information. Does your code base include azurerm itself or is it only calling its cmdlets?

Blackbaud-ShaydeNofziger commented 6 years ago

The codebase only includes calls to the AzureRM cmdlets, and the AzureRM roll-up module is listed as a RequiredModule in the Manifest file.

Script Analyzer is only scanning our *.ps1 files that make up our module, nothing from the AzureRM module itself.

My example is admittedly basic. But any script that includes several different calls to the AzureRM cmdlets sees a significant increase in analysis time.

bergmeister commented 6 years ago

@Blackbaud-ShaydeNofziger OK, I ran a quick test with your example, and running an Import-module AzureRM before running PSSA 10 times against your example only doubles the execution time (which is kind of expected, since AzureRm simply adds a lot of data). But I cannot see a difference when uninstalling AzureRm (I always tried all steps in a fresh new session). I see that there is quite a bit of initial overhead that is not only due to JITting (i.e. the first time the code gets called, .Net has to compile the IL, which means that the 2nd run is always faster) but also pre-populating the PSSA cache . Issue #991 was recently opened to get rid of this JITTING by using crossgen/ngen. It would be interesting what kind of time you get when you run it the 2nd time. I suppose there will be a fair share of JIT overhead and PSSA having to deal with so many AzureRM lazy-loaded method definitions but maybe also some PSSA inefficiencies (especially with respect to multi-threading and cache population/locking). I do not think there is a specific issue here, it is just the accumulation of a lot (and AzureRM being big). But you could check yourself because you can simply build PSSA using VisualStudio yourself and generate a performance report. My personal recommendation would be to have CI runs of PSSA without the AzureRM module but have a nightly build that Installs azurerm before the run and removes it afterwards as I imagine that the results will not be too different (not installing AzureRM means e.g. that you probably don't get warnings on AzureRM aliases) In general you can expect some performance improvements in the next months but at the moment I could not come to a conclusion of a culprit within PSSA that could be quickly fixed. In general, I recommend using the latest version of PowerShell Core, which is generally at least around 50% faster (and probably even more in this case), especially the next preview of 6.1 (which will have .net core 2.1,, which adds another 20%)

Blackbaud-ShaydeNofziger commented 6 years ago

@bergmeister Thank you so much for the info! I can confirm that re-running analysis after the initial run does speed things up a ton, so I agree it sounds like the it's the initial caching/JITTING process - Thanks for the tip about building in VS to see the performance. I'll look into that this weekend.

But I cannot see a difference when uninstalling AzureRm

Do you have the Azure SDK installed? I think that install path is a default module location, so PS falls back to those DLL's if it can't find the modules installed from psgallery.

I like your recommendation about the nightly builds - it gives us the speed we need during the day without sacrificing deeper analysis, which we can run overnight. I appreciate you looking into this for me and giving some insight into what's happening under the hood.

Example results from re-running in the same session:

C:\Repos\blackbaud-iaas-host-azure [master ≡]> Measure-Command { Invoke-ScriptAnalyzer -Path .\Blackbaud.Host.Azure\ApplicationGateway.ps1 }

Days              : 0
Hours             : 0
Minutes           : 1
Seconds           : 9
Milliseconds      : 255
Ticks             : 692551851
TotalDays         : 0.000801564642361111
TotalHours        : 0.0192375514166667
TotalMinutes      : 1.154253085
TotalSeconds      : 69.2551851
TotalMilliseconds : 69255.1851

C:\Repos\blackbaud-iaas-host-azure [master ≡]> Measure-Command { Invoke-ScriptAnalyzer -Path .\Blackbaud.Host.Azure\ApplicationGateway.ps1 }

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 29
Ticks             : 299614
TotalDays         : 3.46775462962963E-07
TotalHours        : 8.32261111111111E-06
TotalMinutes      : 0.000499356666666667
TotalSeconds      : 0.0299614
TotalMilliseconds : 29.9614
bergmeister commented 6 years ago

@Blackbaud-ShaydeNofziger No, I don't have the Azure SDK installed but I noticed that when I uninstalled AzureRm, it only uninstalled the main module but not all the other AzureRM.* modules, which could explain why I could not see a difference. As I said there is future work on performance planned in general but I might come back to this at some point and see if I can find a 'hot path' but without having something that takes a few seconds to analyze on the first run, it is going to be tricky to get something realistic to repro. There will be a new release coming soon that has quite a few fixes and improvements (especially for older PS versions) but I would not expect the new version to have much better performance but it is always worth a try. Some more background: each file is analysed sequentially, but all rules are run in parallel, therefore the slowest rule pretty much dictates the time to analyze 1 file, You could therefore also play around with the -ExcludeRule parameter to see if there are specific rules that cause the bad performance. Another possibility to squeeze out more performance would be to compile PSSA against a newer version of .Net like net472 (we compile against net451 so that it works on any supported Windows OS out of the box), I could try compiling you such a module if you are interested in that? Bear in mind though that Windows PowerShell relies on the installed version of the .net framework, therefore this includes also subtle changes in PowerShell's behaviour and the properties on the objects.

Blackbaud-ShaydeNofziger commented 6 years ago

@bergmeister I have a task to address all of this in next week's sprint - The information you've provided so far has been super-helpful, and I made some good progress building PSSA locally and measuring performance in VS. I'll dive deep into this next week and will be able to provide my findings so far, with a more comprehensive example.

I'll open a separate issue at that time (referencing this one) with the details above and more information, as I think it could prove helpful to others that may be experiencing performance issues.

In the end, if it seems appropriate, I'd like to turn all of this and the information you provided into a documentation-contribution to help others better understand how PSScriptAnalyzer runs under the hood, as well as tips for addressing performance issues. Do you think that would be a worthwhile contribution?

bergmeister commented 6 years ago

We always welcome contributions. A brief description, especially from a user's point of view about performance would be valuable. The thing about implementation details is that they are likely to go out of date but a high level description would still be good.