Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work around runspacepool deadlock #1316

Merged
merged 6 commits into from
Aug 27, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 77 additions & 33 deletions Rules/UseCmdletCorrectly.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Management.Automation;
using System.Management.Automation.Language;
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
using System.Collections.Concurrent;
#if !CORECLR
using System.ComponentModel.Composition;
#endif
Expand All @@ -22,6 +23,24 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
#endif
public class UseCmdletCorrectly : IScriptRule
{
private static readonly ConcurrentDictionary<string, IReadOnlyList<string>> s_pkgMgmtMandatoryParameters =
rjmholt marked this conversation as resolved.
Show resolved Hide resolved
new ConcurrentDictionary<string, IReadOnlyList<string>>(new Dictionary<string, IReadOnlyList<string>>
{
{ "Find-Package", Array.Empty<string>() },
{ "Find-PackageProvider", Array.Empty<string>() },
{ "Get-Package", Array.Empty<string>() },
{ "Get-PackageProvider", Array.Empty<string>() },
{ "Get-PackageSource", Array.Empty<string>() },
{ "Import-PackageProvider", new string[] { "Name" } },
{ "Install-Package", new string[] { "Name" } },
{ "Install-PackageProvider", new string[] { "Name" } },
{ "Register-PackageSource", new string[] { "ProviderName" } },
{ "Save-Package", new string[] { "Name", "InputObject" } },
{ "Set-PackageSource", new string[] { "Name", "Location" } },
{ "Uninstall-Package", new string[] { "Name", "InputObject" } },
{ "Unregister-PackageSource", new string[] { "Name", "InputObject" } },
});

/// <summary>
/// AnalyzeScript: Check that cmdlets are invoked with the correct mandatory parameter
/// </summary>
Expand Down Expand Up @@ -61,41 +80,69 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
/// <returns></returns>
private bool MandatoryParameterExists(CommandAst cmdAst)
{
CommandInfo cmdInfo = null;
List<ParameterMetadata> mandParams = new List<ParameterMetadata>();
IEnumerable<CommandElementAst> ceAsts = null;
bool returnValue = false;
#region Compares parameter list and mandatory parameter list.

#region Predicates
CommandInfo cmdInfo = Helper.Instance.GetCommandInfoLegacy(cmdAst.GetCommandName());

// Predicate to find ParameterAsts.
Func<CommandElementAst, bool> foundParamASTs = delegate(CommandElementAst ceAst)
// If we can't resolve the command or it's not a cmdlet, we are done
if (cmdInfo == null || (cmdInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet))
{
if (ceAst is CommandParameterAst) return true;
return false;
};

#endregion
return true;
}

#region Compares parameter list and mandatory parameter list.
// We can't statically analyze splatted variables, so ignore them
if (Helper.Instance.HasSplattedVariable(cmdAst))
{
return true;
}

cmdInfo = Helper.Instance.GetCommandInfoLegacy(cmdAst.GetCommandName());
if (cmdInfo == null || (cmdInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet))
// Positional parameters could be mandatory, so we assume all is well
if (Helper.Instance.PositionalParameterUsed(cmdAst) && Helper.Instance.IsKnownCmdletFunctionOrExternalScript(cmdAst))
{
return true;
}

// ignores if splatted variable is used
if (Helper.Instance.HasSplattedVariable(cmdAst))
// If the command is piped to, this also precludes mandatory parameters
if (cmdAst.Parent is PipelineAst parentPipeline
&& parentPipeline.PipelineElements.Count > 1
&& parentPipeline.PipelineElements[0] != cmdAst)
{
return true;
}

// Gets parameters from command elements.
ceAsts = cmdAst.CommandElements.Where<CommandElementAst>(foundParamASTs);
// We want to check cmdlets from PackageManagement separately because they experience a deadlock
// when cmdInfo.Parameters or cmdInfo.ParameterSets is accessed.
// See https://github.com/PowerShell/PSScriptAnalyzer/issues/1297
if (s_pkgMgmtMandatoryParameters.TryGetValue(cmdInfo.Name, out IReadOnlyList<string> pkgMgmtCmdletMandatoryParams))
{
// If the command has no parameter sets with mandatory parameters, we are done
if (pkgMgmtCmdletMandatoryParams.Count == 0)
{
return true;
}

// We make the following simplifications here that all apply to the PackageManagement cmdlets:
// - Only one mandatory parameter per parameter set
// - Any part of the parameter prefix is valid
// - There are no parameter sets without mandatory parameters
IEnumerable<CommandParameterAst> parameterAsts = cmdAst.CommandElements.OfType<CommandParameterAst>();
foreach (string mandatoryParameter in pkgMgmtCmdletMandatoryParams)
{
foreach (CommandParameterAst parameterAst in parameterAsts)
{
if (mandatoryParameter.StartsWith(parameterAst.ParameterName))
{
return true;
}
}
}

return false;
}

// Gets mandatory parameters from cmdlet.
// If cannot find any mandatory parameter, it's not necessary to do a further check for current cmdlet.
var mandatoryParameters = new List<ParameterMetadata>();
try
{
int noOfParamSets = cmdInfo.ParameterSets.Count;
Expand All @@ -119,7 +166,7 @@ private bool MandatoryParameterExists(CommandAst cmdAst)

if (count >= noOfParamSets)
{
mandParams.Add(pm);
mandatoryParameters.Add(pm);
}
}
}
Expand All @@ -129,28 +176,25 @@ private bool MandatoryParameterExists(CommandAst cmdAst)
return true;
}

if (mandParams.Count == 0 || (Helper.Instance.IsKnownCmdletFunctionOrExternalScript(cmdAst) && Helper.Instance.PositionalParameterUsed(cmdAst)))
if (mandatoryParameters.Count == 0)
{
returnValue = true;
return true;
}
else

// Compares parameter list and mandatory parameter list.
foreach (CommandElementAst commandElementAst in cmdAst.CommandElements.OfType<CommandParameterAst>())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth noting that the mandatory parameter logic here is pretty flawed (I haven't changed it from the original). It takes a list of all the mandatory parameters of the cmdlet and checks to see that at least one was supplied.

But:

  • There could be a parameter set specified by a provided non-mandatory parameter which has no mandatory parameters
  • There could be multiple mandatory parameters in a given parameter set

Parameter binding is hard and rather than trying to fix the logic here, any attempt to fix it should be more general-purpose. But nobody seems to have complained about this yet, so I doubt it's actually a huge issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's something I'd like to fix in an external library and reuse in PSSA, PSES and a few other places

{
// Compares parameter list and mandatory parameter list.
foreach (CommandElementAst ceAst in ceAsts)
CommandParameterAst cpAst = (CommandParameterAst)commandElementAst;
if (mandatoryParameters.Count<ParameterMetadata>(item =>
item.Name.Equals(cpAst.ParameterName, StringComparison.OrdinalIgnoreCase)) > 0)
{
CommandParameterAst cpAst = (CommandParameterAst)ceAst;
if (mandParams.Count<ParameterMetadata>(item =>
item.Name.Equals(cpAst.ParameterName, StringComparison.OrdinalIgnoreCase)) > 0)
{
returnValue = true;
break;
}
return true;
}
}

#endregion

return returnValue;
return false;
}

/// <summary>
Expand Down
8 changes: 8 additions & 0 deletions Tests/Engine/DeadlockRegression.Tests.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.

Describe "Complex file analysis" {
It "Analyzes file successfully" {
Invoke-ScriptAnalyzer -Path "$PSScriptRoot/DeadlockTestAssets/complex.psm1"
}
}
Loading