diff --git a/Engine/CommandInfoCache.cs b/Engine/CommandInfoCache.cs index aa9d725f3..dad365f99 100644 --- a/Engine/CommandInfoCache.cs +++ b/Engine/CommandInfoCache.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Management.Automation; using System.Linq; using System.Management.Automation.Runspaces; @@ -14,8 +15,25 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer /// internal class CommandInfoCache : IDisposable { + /// + /// Number of times a command lookup is attempted before giving up. + /// Command lookups can fail transiently because the PowerShell engine is not thread safe, + /// see https://github.com/PowerShell/PowerShell/issues/4003 + /// + private const int MaxLookupAttempts = 3; + private readonly ConcurrentDictionary> _commandInfoCache; - private readonly RunspacePool _runspacePool; + + /// + /// Guards all access to so that only one thread at a time drives the + /// PowerShell engine. The engine is not thread safe, so concurrent lookups can fail transiently, + /// see https://github.com/PowerShell/PowerShell/issues/4003. + /// A monitor is used rather than a semaphore because it is re-entrant, which avoids a deadlock + /// should a lookup ever end up calling back into the cache on the same thread. + /// + private readonly object _runspaceLock = new object(); + + private readonly Runspace _runspace; private bool disposed = false; /// @@ -24,11 +42,13 @@ internal class CommandInfoCache : IDisposable public CommandInfoCache() { _commandInfoCache = new ConcurrentDictionary>(); - _runspacePool = RunspaceFactory.CreateRunspacePool(1, 10); - _runspacePool.Open(); + // A single runspace rather than a pool: all lookups are serialized on it, so that the + // PowerShell engine is never driven concurrently. + _runspace = RunspaceFactory.CreateRunspace(); + _runspace.Open(); } - /// Dispose the runspace pool + /// Dispose the runspace public void Dispose() { Dispose(true); @@ -37,17 +57,23 @@ public void Dispose() protected virtual void Dispose(bool disposing) { - if ( disposed ) + // Always take the lock, also on the finalizer path, so that 'disposed' is never + // published without the runspace being disposed along with it and so that the runspace + // cannot be disposed while a lookup is in flight. + lock (_runspaceLock) { - return; - } + if ( disposed ) + { + return; + } - if ( disposing ) - { - _runspacePool.Dispose(); - } + disposed = true; - disposed = true; + if ( disposing ) + { + _runspace.Dispose(); + } + } } /// @@ -70,7 +96,21 @@ public CommandInfo GetCommandInfo(string commandName, CommandTypes? commandTypes return GetCommandInfoInternal(commandName, commandTypes); } // Atomically either use PowerShell to query a command info object, or fetch it from the cache - return _commandInfoCache.GetOrAdd(key, new Lazy(() => GetCommandInfoInternal(commandName, commandTypes))).Value; + var lazyCommandInfo = _commandInfoCache.GetOrAdd(key, new Lazy(() => GetCommandInfoInternal(commandName, commandTypes))); + try + { + return lazyCommandInfo.Value; + } + catch + { + // Lazy caches exceptions forever, which would make every subsequent lookup of this + // command fail for the lifetime of the process. Evict the entry so that the next lookup + // can try again. Only remove the faulted instance so that a replacement that another + // thread may already have added is left alone. + ((ICollection>>)_commandInfoCache) + .Remove(new KeyValuePair>(key, lazyCommandInfo)); + throw; + } } @@ -99,26 +139,56 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command // For more details see https://github.com/PowerShell/PowerShell/issues/9308 actualCmdName = WildcardPattern.Escape(actualCmdName); - using (var ps = System.Management.Automation.PowerShell.Create()) + for (int attempt = 1; ; attempt++) { - ps.RunspacePool = _runspacePool; - - ps.AddCommand("Get-Command") - .AddParameter("Name", actualCmdName) - .AddParameter("ErrorAction", "SilentlyContinue"); - - if (commandType != null) - { - ps.AddParameter("CommandType", commandType); - } - - if (!string.IsNullOrEmpty(moduleName)) + // Serialize all use of the PowerShell engine. Only cache misses reach this point; + // lookups that are already cached are served without taking the lock. + lock (_runspaceLock) { - ps.AddParameter("Module", moduleName); + if (disposed) + { + return null; + } + + using (var ps = System.Management.Automation.PowerShell.Create()) + { + ps.Runspace = _runspace; + + ps.AddCommand("Get-Command") + .AddParameter("Name", actualCmdName) + .AddParameter("ErrorAction", "SilentlyContinue"); + + if (commandType != null) + { + ps.AddParameter("CommandType", commandType); + } + + if (!string.IsNullOrEmpty(moduleName)) + { + ps.AddParameter("Module", moduleName); + } + + try + { + return ps.Invoke() + .FirstOrDefault(); + } + // 'Get-Command' is invoked with 'SilentlyContinue', so a CommandNotFoundException can only + // mean that the engine failed to resolve 'Get-Command' itself in the runspace. + // That happened intermittently when lookups ran concurrently because the PowerShell engine + // is not thread safe, see https://github.com/PowerShell/PowerShell/issues/4003 and + // https://github.com/PowerShell/PSScriptAnalyzer/issues/2205 + // Lookups are serialized now, so this should no longer occur, but the retry is kept as a + // safety net for hosts that drive the engine from other threads at the same time. + catch (CommandNotFoundException) + { + if (attempt >= MaxLookupAttempts) + { + return null; + } + } + } } - - return ps.Invoke() - .FirstOrDefault(); } } diff --git a/Rules/UseCorrectCasing.cs b/Rules/UseCorrectCasing.cs index f4f2c40b7..de9e2acd6 100644 --- a/Rules/UseCorrectCasing.cs +++ b/Rules/UseCorrectCasing.cs @@ -128,10 +128,17 @@ public override IEnumerable AnalyzeScript(Ast ast, string file // It's a known issue that objects from PowerShell can have a runspace affinity, // therefore if that happens, we query a fresh object instead of using the cache. // https://github.com/PowerShell/PowerShell/issues/4003 - catch (InvalidOperationException) + // The affinity problem surfaces as an InvalidOperationException or as a + // NullReferenceException, see https://github.com/PowerShell/PSScriptAnalyzer/issues/1708 + catch (Exception exception) when (exception is InvalidOperationException || exception is NullReferenceException) { - commandInfo = Helper.Instance.GetCommandInfo(commandName, bypassCache: true); - availableParameters = commandInfo.Parameters; + availableParameters = GetParametersFromFreshCommandInfo(commandName); + } + if (availableParameters is null) + { + // The parameters of this command cannot be determined reliably, + // so skip the parameter casing check instead of failing the analysis. + continue; } foreach (var commandParameterAst in commandParameterAsts) { @@ -161,6 +168,22 @@ public override IEnumerable AnalyzeScript(Ast ast, string file } } + /// + /// Queries a fresh object to work around the runspace affinity problem + /// of the PowerShell engine and returns its parameters, or null if they cannot be determined. + /// + private Dictionary GetParametersFromFreshCommandInfo(string commandName) + { + try + { + return Helper.Instance.GetCommandInfo(commandName, bypassCache: true)?.Parameters; + } + catch (Exception exception) when (exception is InvalidOperationException || exception is NullReferenceException) + { + return null; + } + } + /// /// For a command like "gci -path c:", returns the extent of "gci" in the command /// diff --git a/Tests/Engine/CommandInfoCacheConcurrency.tests.ps1 b/Tests/Engine/CommandInfoCacheConcurrency.tests.ps1 new file mode 100644 index 000000000..10f9c1047 --- /dev/null +++ b/Tests/Engine/CommandInfoCacheConcurrency.tests.ps1 @@ -0,0 +1,64 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +Describe "Concurrent command lookups" { + BeforeAll { + # Run the analyzer once so that the singleton Helper is created by the cmdlet. Touching + # Helper.Instance before that would install a helper without a command invocation context, + # which breaks every later analysis in this process. + $null = Invoke-ScriptAnalyzer -ScriptDefinition 'Get-Item -Path .' + + # The concurrency driver is written in C# so that the lookups really do run on separate + # threads. Invoking a PowerShell script block on a thread pool thread would introduce + # runspace affinity problems of its own and would not test the command info cache. + $analyzerAssembly = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Helper].Assembly.Location + Add-Type -IgnoreWarnings -WarningAction SilentlyContinue -ReferencedAssemblies $analyzerAssembly, ([System.Management.Automation.PSObject].Assembly.Location) -TypeDefinition @' +using System.Threading.Tasks; +using Microsoft.Windows.PowerShell.ScriptAnalyzer; + +public static class ConcurrentCommandLookup +{ + public static string[] Lookup(string[] commandNames) + { + var helper = Helper.Instance; + var tasks = new Task[commandNames.Length]; + for (int i = 0; i < commandNames.Length; i++) + { + string name = commandNames[i]; + tasks[i] = Task.Run(() => + { + var commandInfo = helper.GetCommandInfo(name); + return commandInfo == null ? null : commandInfo.Name; + }); + } + + Task.WaitAll(tasks); + + var results = new string[tasks.Length]; + for (int i = 0; i < tasks.Length; i++) + { + results[i] = tasks[i].Result; + } + + return results; + } +} +'@ + } + + It "resolves commands from several threads without failing" { + $commandNames = @( + 'Get-ChildItem', 'Where-Object', 'ForEach-Object', 'Get-Content', 'Write-Output', + 'Test-Path', 'Get-Command', 'Select-Object', 'Sort-Object', 'Measure-Object' + ) * 4 + + # A lookup that hits the thread safety problem throws, which fails the test. + $results = [ConcurrentCommandLookup]::Lookup($commandNames) + + $results.Count | Should -Be $commandNames.Count + # A failed lookup returns null, so every entry must name the command that was requested. + for ($i = 0; $i -lt $commandNames.Count; $i++) { + $results[$i] | Should -BeExactly $commandNames[$i] + } + } +} diff --git a/Tests/Rules/Issue2205.tests.ps1 b/Tests/Rules/Issue2205.tests.ps1 new file mode 100644 index 000000000..4e1cf945b --- /dev/null +++ b/Tests/Rules/Issue2205.tests.ps1 @@ -0,0 +1,12 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +Describe 'Issue 2205' { + It "does not fail the analysis when a command lookup hits the runspace affinity problem" -Skip:(-not $IsLinux) { + $settingsPath = Join-Path $PSScriptRoot 'Issue2205/PSScriptAnalyzerSettings.psd1' + # $PSScriptRoot is /Tests/Rules, so two levels up is the repository root. + $repositoryRoot = (Resolve-Path (Join-Path $PSScriptRoot '..' '..')).Path + + Invoke-ScriptAnalyzer -Path $repositoryRoot -Recurse -Settings $settingsPath -ErrorAction Stop | Out-Null + } +} diff --git a/Tests/Rules/Issue2205/PSScriptAnalyzerSettings.psd1 b/Tests/Rules/Issue2205/PSScriptAnalyzerSettings.psd1 new file mode 100644 index 000000000..1849a35d3 --- /dev/null +++ b/Tests/Rules/Issue2205/PSScriptAnalyzerSettings.psd1 @@ -0,0 +1,53 @@ +@{ + Severity = @('Error', 'Warning', 'Information') + IncludeRules = @( + 'PSAvoidUsingCmdletAliases', 'PSAvoidDefaultValueForMandatoryParameter', + 'PSAvoidDefaultValueSwitchParameter', 'PSAvoidGlobalAliases', + 'PSAvoidGlobalFunctions', 'PSAvoidGlobalVars', 'PSAvoidInvokingEmptyMembers', + 'PSAvoidNullOrEmptyHelpMessageAttribute', 'PSAvoidShouldContinueWithoutForce', + 'PSAvoidUsingComputerNameHardcoded', 'PSAvoidUsingConvertToSecureStringWithPlainText', + 'PSAvoidUsingDeprecatedManifestFields', 'PSAvoidUsingEmptyCatchBlock', + 'PSAvoidUsingInvokeExpression', 'PSAvoidUsingPlainTextForPassword', + 'PSAvoidUsingPositionalParameters', 'PSAvoidUsingUsernameAndPasswordParams', + 'PSAvoidUsingWMICmdlet', 'PSAvoidUsingWriteHost', 'PSMisleadingBacktick', + 'PSMissingModuleManifestField', 'PSPossibleIncorrectComparisonWithNull', + 'PSPossibleIncorrectUsageOfAssignmentOperator', 'PSPossibleIncorrectUsageOfRedirectionOperator', + 'PSProvideCommentHelp', 'PSReservedCmdletChar', 'PSReservedParams', + 'PSUseApprovedVerbs', 'PSUseBOMForUnicodeEncodedFile', 'PSUseCmdletCorrectly', + 'PSUseConsistentIndentation', 'PSUseConsistentWhitespace', 'PSUseCorrectCasing', + 'PSUseDeclaredVarsMoreThanAssignments', 'PSUseLiteralInitializerForHashtable', + 'PSUseOutputTypeCorrectly', 'PSUsePSCredentialType', 'PSUseSingularNouns', + 'PSUseToExportFieldsInManifest', 'PSUseUTF8EncodingForHelpFile' + ) + ExcludeRules = @( + 'PSAvoidUsingWriteHost', 'PSAvoidUsingPositionalParameters', 'PSUseApprovedVerbs', + 'PSProvideCommentHelp', 'PSAvoidGlobalVars', 'PSAvoidGlobalFunctions', + 'PSUseSingularNouns', 'PSUseOutputTypeCorrectly' + ) + Rules = @{ + PSUseConsistentIndentation = @{ + Enable = $true + IndentationSize = 4 + PipelineIndentation = 'IncreaseIndentationForFirstPipeline' + Kind = 'space' + } + PSUseConsistentWhitespace = @{ + Enable = $true + CheckInnerBrace = $true + CheckOpenBrace = $true + CheckOpenParen = $true + CheckOperator = $true + CheckPipe = $true + CheckPipeForRedundantWhitespace = $false + CheckSeparator = $true + CheckParameter = $false + IgnoreAssignmentOperatorInsideHashTable = $true + } + PSUseCompatibleCmdlets = @{ Enable = $false } + PSUseCorrectCasing = @{ Enable = $true } + PSAvoidUsingCmdletAliases = @{ Enable = $true; allowlist = @() } + PSAlignAssignmentStatement = @{ Enable = $false; CheckHashtable = $false } + PSPlaceOpenBrace = @{ Enable = $true; OnSameLine = $true; NewLineAfter = $true; IgnoreOneLineBlock = $true } + PSPlaceCloseBrace = @{ Enable = $true; NewLineAfter = $true; IgnoreOneLineBlock = $true; NoEmptyLineBefore = $false } + } +}