Continuing to test Microsoft projects: PowerShell analysis

For Microsoft, it has recently become a 'good tradition' to open the source code for its software products. Here you can recall about CoreFX, .Net Compiler Platform (Roslyn), Code Contracts, MSBuild and other projects. For us, developers of the PVS-Studio static analyzer, this is an opportunity to check well-known projects, tell people (including developers) about the errors found, and test the analyzer. Today we will talk about errors found in another Microsoft project - PowerShell.

Powerhell

PowerShell is a cross-platform project of Microsoft, consisting of a shell with a command-line interface and an accompanying scripting language. Windows PowerShell is built on and integrated with the Microsoft .NET Framework . Additionally, PowerShell provides convenient access to COM , WMI, and ADSI , as well as allowing you to execute regular command-line commands to create a single environment in which administrators can perform various tasks on local and remote systems.

Project code is available for download from the repository on GitHub .

PVS-Studio

According to statistics from the project repository, approximately 93% of the code is written using the C # programming language.

For verification, we used the PVS-Studio static code analyzer. The version under development was used. That is, it is no longer PVS-Studio 6.08, but also not PVS-Studio 6.09. This approach allows a wider approach to testing the most recent version of the analyzer, and, if possible, to correct the deficiencies found. Of course, this does not negate the use of a multi-level test system (see seven testing methods in the article devoted to the development of the Linux version), but serves as another way to test the analyzer.

The current version of the analyzer can be downloaded here .

Analysis preparation

The analyzer is updated, the project code is loaded. But sometimes difficulties can arise in the process of preparing a project for analysis - at the stage of its assembly. Before checking, it is advisable to assemble the project. Why is it important? So the analyzer will be available more information, therefore, it becomes possible to conduct a deeper analysis.

The most familiar (and convenient) way to use the analyzer is to check the project from the Visual Studio development environment. It is fast, easy and convenient. But then one unpleasant nuance surfaced.

As it turned out, the developers themselves do not recommend using the Visual Studio development environment to build the project, which is written directly on GitHub: “We do not recommend building the PowerShell solution from Visual Studio.”

But the temptation to build with Visual Studio and check from under it was too great, so I decided to try it. The result is shown in the figure below:

Figure 1. Project compilation errors using Visual Studio.

Unpleasant. What did it mean to me? The fact that just like that all the capabilities of the analyzer on the project cannot be revealed. Several scenarios are possible here.

Scenario 1. Verification of an unassembled project.

We tried to assemble a project. Not going to? Well, let's check it as it is.

What are the advantages of this approach? No need to bother with the assembly, figuring out what the problem is, how to solve it, or how to dodge and check the assembled project. This saves time, and there is no guarantee that after transporting long enough, the project can still be assembled, and then time will be wasted.

The disadvantages of this decision are also obvious. Firstly, this is an inferior analysis. Some errors will slip away from the analyzer. Perhaps a number of false positives will appear. Secondly, in this case, it makes no sense to give false / positive statistics, since it can change in any way on the assembled project.

Nevertheless, even in this scenario, you can find enough errors and write an article about it.

Scenario 2. Understand and assemble the project.

Pros and cons are opposite to those described above. Yes, you will have to spend more time on the assembly, but it is not a fact that the desired result will be achieved. But if successful, you can verify the source code more thoroughly and possibly find some other interesting errors.

There is no definite advice on what to do here, everyone decides for himself.

After suffering with the assembly, I still decided to check the project 'as is'. For writing an article, this option is quite acceptable.

Note. Despite the fact that the project is not built from under Visual Studio, it is quite calmly built through the script ( build.sh ), which is at the root.

Note 2. One of the developers (thanks a lot to him for this) suggested that the * .sln-file is necessary for convenience in work, but not for assembly. This is another argument in favor of the correct choice of the analysis scenario.

Analysis results

Duplicate subexpressions

Projects in which warning V3001 does not find suspicious locations should be issued medals. PowerShell, alas, in that case would have been left without a medal, and here's why:

internal Version BaseMinimumVersion { get; set; }
internal Version BaseMaximumVersion { get; set; }
protected override void ProcessRecord()
{
  if (BaseMaximumVersion != null && 
      BaseMaximumVersion != null && 
      BaseMaximumVersion < BaseMinimumVersion)
  {
    string message = StringUtil.Format(
      Modules.MinimumVersionAndMaximumVersionInvalidRange,
      BaseMinimumVersion, 
      BaseMaximumVersion);
    throw new PSArgumentOutOfRangeException(message);
  }
  ....
}

PVS-Studio Warning: V3001 There are identical sub-expressions 'BaseMaximumVersion! = Null' to the left and to the right of the '&&' operator. System.Management.Automation ImportModuleCommand.cs 1663

Link to the code on GitHub .

As you can see from the code snippet, the BaseMaximumVersion link was checked twice for the null inequality , although it is clear that the BaseMinimumVersion link needed to be checked instead . Due to a successful combination of circumstances, this error may not manifest itself for a long time. However, when the error manifests itself, information about BaseMinimumVersion will not fall into the text of the message used in the exception, since the BaseMinimumVersion linkwill be null . As a result, we will lose some useful information.

I want to note that when writing the article, I fixed the code formatting, so that it became easier to notice the error. In the project code, the entire condition is written on one line. This is another reminder of how important good code design is. It not only facilitates reading and understanding of the code, but also helps to quickly spot errors.

internal static class RemoteDataNameStrings
{
  ....
  internal const string MinRunspaces = "MinRunspaces";
  internal const string MaxRunspaces = "MaxRunspaces";
  ....
}
internal void ExecuteConnect(....)
{
  ....
  if 
  (
    connectRunspacePoolObject.Data
    .Properties[RemoteDataNameStrings.MinRunspaces] != null 
    &&   
    connectRunspacePoolObject.Data
    .Properties[RemoteDataNameStrings.MinRunspaces] != null
  )
  {
    try
    {
      clientRequestedMinRunspaces = RemotingDecoder.GetMinRunspaces(
        connectRunspacePoolObject.Data);
      clientRequestedMaxRunspaces = RemotingDecoder.GetMaxRunspaces(
        connectRunspacePoolObject.Data);
      clientRequestedRunspaceCount = true;
    }
    ....
  }
  ....
}

PVS-Studio Warning: V3001 There are identical sub-expressions to the left and to the right of the '&&' operator. System.Management.Automation serverremotesession.cs 633

Link to the code on GitHub .

Due to a typo, the same check was performed twice again. Most likely, in the second case the constant field MaxRunspaces of the static class RemoteDataNameStrings should have been used .

Unused value returned by the method

There are errors characteristic of the fact that the return value of the method is not used. Causes, as well as consequences, can be very different. Sometimes a programmer forgets that objects of type Stringare immutable, and line editing methods return a new drain as a result, rather than changing the current one. A similar case is the use of LINQ, when the result of the operation is a new collection. Similar errors occurred here.

private CatchClauseAst CatchBlockRule(.... 
  ref List errorAsts)
{
  ....
  if (errorAsts == null)
  {
    errorAsts = exceptionTypes;
  }
  else
  {
    errorAsts.Concat(exceptionTypes); // <=
  }
  ....
}

PVS-Studio Warning: V3010 The return value of function 'Concat' is required to be utilized. System.Management.Automation Parser.cs 4973

Link to the code on GitHub .

Immediately I want to draw attention to the fact that the errorAsts parameter is used with the ref keyword , which means changing the link in the body of the method. The logic of the code is simple - if the errorAsts link is null, then we assign it a link to another collection, otherwise we add the elements of the exceptionTypes collection to the existing one. True, the overlay came out with the second part. The Concat method returns a new collection without modifying the existing one. So the errorAsts collectionwill remain unchanged, and the new one (containing the errorAsts and exceptionTypes elements ) will be ignored.

The problem can be solved in several ways:

• Use the AddRange method of the List class , which will add new elements to the current list;
• Use the result of the Concat method , without forgetting to call the ToList method , to cast to the desired type.

Checking the wrong link after casting using the as operator

Gold medal to diagnostic rule V3019 ! Surely I will not say, but in almost all C # -projects on which I wrote articles, this error occurred. Regular readers should already learn by heart the rule of the need to carefully double-check whether you check the link for null after casting using the as operator .

internal List GetJobsForComputer(String computerName)
{
  ....
  foreach (Job j in ChildJobs)
  {
    PSRemotingChildJob child = j as PSRemotingChildJob;
    if (j == null) continue;
    if (String.Equals(child.Runspace
                           .ConnectionInfo
                           .ComputerName, 
                      computerName,
                      StringComparison.OrdinalIgnoreCase))
    {
      returnJobList.Add(child);
    }
  }
  return returnJobList;
} 

PVS-Studio Warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'j', 'child'. System.Management.Automation Job.cs 1876

Link to the code on GitHub .

The result of casting j to the type PSRemotingChildJob is written to the child link , which means that null can be written there (if the original link is null or if the cast failed). Nevertheless, below the null inequality is checked for the original link - j , and even lower is the reference to the object's Runspace propertychild . Thus, if j! = Null and child == null , checking j == null will not help, and an exception of type NullReferenceException will be thrown when accessing instance members of a derived link .

Met two more such places:

• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'j', 'child'. System.Management.Automation Job.cs 1900
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'j', 'child'. System.Management.Automation Job.cs 1923

Incorrect order of operations

private void CopyFileFromRemoteSession(....)
{
  ....
  ArrayList remoteFileStreams = 
    GetRemoteSourceAlternateStreams(ps, sourceFileFullName);
  if ((remoteFileStreams.Count > 0) && (remoteFileStreams != null))
  ....
}

PVS-Studio Warning: V3027 The variable 'remoteFileStreams' was utilized in the logical expression before it was verified against null in the same logical expression. System.Management.Automation FileSystemProvider.cs 4126

Link to the code on GitHub .

If you are lucky, the code will work fine, if you are not lucky - when you try to dereference a null reference, an exception of type NullReferenceException will be thrown . The remoteFileStreams! = Null subexpression does not play any role in this expression, nor does it protect against an exception. Obviously, for proper operation it is necessary to swap subexpressions.

Well, we are all human, and we all make mistakes. And analyzers are needed to find these errors.

Possible dereferencing of a null reference

internal bool SafeForExport()
{
  return DisplayEntry.SafeForExport() &&
         ItemSelectionCondition == null 
      || ItemSelectionCondition.SafeForExport();
}

PVS-Studio Warning: V3080 Possible null dereference. Consider inspecting 'ItemSelectionCondition'. System.Management.Automation displayDescriptionData_List.cs 352

Link to the code on GitHub .

A potential exception of type NullReferenceException . A subexpression ItemSelectionCondition.SafeForExport () will be evaluated only if the result of the first subexpression is false . It follows that if DisplayEntry.SafeForExport () returns false and ItemSelectionCondition == null , the second subexpression will be evaluated - ItemSelectionCondition.SafeForExport (), where there will be a problem of dereferencing a null reference (and as a result, an exception will be generated).

A similar code occurred one more time. Relevant Warning: V3080 Possible null dereference. Consider inspecting 'EntrySelectedBy'. System.Management.Automation displayDescriptionData_Wide.cs 247

Another case.

internal Collection GetProvider(
  PSSnapinQualifiedName providerName)
{
  ....
  if (providerName == null)
  {
    ProviderNotFoundException e =
      new ProviderNotFoundException(
          providerName.ToString(),
          SessionStateCategory.CmdletProvider,
          "ProviderNotFound",
          SessionStateStrings.ProviderNotFound);
    throw e;
  }
  ....
}

PVS-Studio Warning: V3080 Possible null dereference. Consider inspecting 'providerName'. System.Management.Automation SessionStateProviderAPIs.cs 1004

Link to the code on GitHub .

Sometimes a code like this is found - they wanted to throw an exception of one type, but it turned out another. Why? In this case, it is checked that the providerName reference is null , but below, when an exception object is created, the ToString instance method is called on the same link . The result will be an exception of type NullReferenceException , not a ProviderNotFoundException , as planned.

Another similar piece of code met. Relevant Warning:V3080 Possible null dereference. Consider inspecting 'job'. System.Management.Automation PowerShellETWTracer.cs 1088

Using a link before checking for null

internal ComplexViewEntry 
  GenerateView(...., FormattingCommandLineParameters inputParameters)
{
  _complexSpecificParameters = 
    (ComplexSpecificParameters)inputParameters.shapeParameters;
  int maxDepth = _complexSpecificParameters.maxDepth;
  ....
  if (inputParameters != null)
    mshParameterList = inputParameters.mshParameterList;
  ....
}

PVS-Studio Warning: V3095 The 'inputParameters' object was used before it was verified against null. Check lines: 430, 436. System.Management.Automation FormatViewGenerator_Complex.cs 430

Link to the code on GitHub .

Checking inputParameters! = Null implies that the reference being checked can be null . Reinsured so that when accessing the field mshParameterList not to get a NullReferenceException . Everything is correct. Here are just above already appealing to another instance field of the same object - shapeParameters . Since inputParametersbetween these two operations does not change, if the link was initially null , checking for null inequality will not save from an exception.

A similar case:

public CommandMetadata(CommandMetadata other)
{
  ....
  _parameters = new Dictionary(
    other.Parameters.Count, StringComparer.OrdinalIgnoreCase);
  // deep copy
  if (other.Parameters != null)
  ....
}

PVS-Studio Warning: V3095 The 'other.Parameters' object was used before it was verified against null. Check lines: 189, 192. System.Management.Automation CommandMetadata.cs 189

Link to the code on GitHub .

Check that the Parameters property of the other object is not null , but a couple of lines above refer to the instance property Count . Something is clearly wrong here.

Unused constructor parameter

It's nice to see the results of the new diagnostic rules immediately after they appear. So it happened with the diagnostics of V3117 .

private void PopulateProperties(
  Exception exception,
  object targetObject,
  string fullyQualifiedErrorId,
  ErrorCategory errorCategory,
  string errorCategory_Activity,
  string errorCategory_Reason,
  string errorCategory_TargetName,
  string errorCategory_TargetType,
  string errorCategory_Message,
  string errorDetails_Message,
  string errorDetails_RecommendedAction,
  string errorDetails_ScriptStackTrace)
{ .... }
internal ErrorRecord(
  Exception exception,
  object targetObject,
  string fullyQualifiedErrorId,
  ErrorCategory errorCategory,
  string errorCategory_Activity,
  string errorCategory_Reason,
  string errorCategory_TargetName,
  string errorCategory_TargetType,
  string errorCategory_Message,
  string errorDetails_Message,
  string errorDetails_RecommendedAction)
{
  PopulateProperties(
    exception, targetObject, fullyQualifiedErrorId, 
    errorCategory, errorCategory_Activity,
    errorCategory_Reason, errorCategory_TargetName, 
    errorCategory_TargetType, errorDetails_Message,     
    errorDetails_Message, errorDetails_RecommendedAction, 
    null);
}

PVS-Studio warning : V3117 Constructor parameter 'errorCategory_Message' is not used. System.Management.Automation ErrorPackage.cs 1125

» Link to the code on GitHub .

The ErrorRecord constructor calls the PopulateProperties method , which initializes the fields and some other actions. The analyzer warns that one of the constructor parameters - errorCategory_Message - is not used in its body. Indeed, if you look closely at the call to the PopulateProperties method , you will notice that the errorDetails_Message argument is passed 2 times to the method , but errorCategory_Message is not passed. We look at the parameters of the PopulateProperties method and make sure that there is an error.

A condition that is always false

One of the capabilities of PVS-Studio that helps to implement complex diagnostics and find interesting errors is the so-called 'virtual values', with which you can find out what ranges of values ​​a variable can take at a certain stage of program execution. More information can be obtained from the article 'Search for Errors Using Virtual Value Computation' . Based on this mechanism, diagnostic rules such as V3022 and V3063 are built. With their help, it is often possible to detect interesting errors. This happened this time, so I propose to consider one of the errors found:

public enum RunspacePoolState
{
  BeforeOpen = 0,
  Opening = 1,
  Opened = 2,
  Closed = 3,
  Closing = 4,
  Broken = 5,
  Disconnecting = 6,
  Disconnected = 7,
  Connecting = 8,
}
internal virtual int GetAvailableRunspaces()
{
  ....
  if (stateInfo.State == RunspacePoolState.Opened)
  {
    ....
    return (pool.Count + unUsedCapacity);
  }
  else if (stateInfo.State != RunspacePoolState.BeforeOpen && 
           stateInfo.State != RunspacePoolState.Opening)
  {
    throw new InvalidOperationException(
      HostInterfaceExceptionsStrings.RunspacePoolNotOpened);
  }
  else if (stateInfo.State == RunspacePoolState.Disconnected)
  {
    throw new InvalidOperationException(
      RunspacePoolStrings.CannotWhileDisconnected);
  }
  else
  {
    return maxPoolSz;
  }
 ....
}

PVS-Studio warning : V3022 Expression 'stateInfo.State == RunspacePoolState.Disconnected' is always false. System.Management.Automation RunspacePoolInternal.cs 581

» Link to the code on GitHub .

The analyzer claims that the expression stateInfo.State == RunspacePoolState.Disconnected is always false. Is it so? Of course, otherwise, why would I write this code!

The developer made a mistake in the previous condition. The fact is that if stateInfo.State == RunspacePoolState.Disconnected , the previous if statement will always be executed . To fix the error, just swap the last two if statements ( else if)

More mistakes?

Yes, there were many more places that the analyzer considered suspicious. Those who regularly read our articles know that often we do not write out all the errors. Maybe it wouldn’t have reached the size of the Mono verification article , but there is still material for writing. The greatest interest in all warnings should arise from the developers, but to the rest of the readers, I just try to show the most interesting suspicious places.

“Have you told the developers?”

Oddly enough, we are still periodically asked this question. We always inform the developers about the bugs found, but this time I decided to go a little further.

I talked to one of the developers (hello, Sergey!) Personally through Gitter. The advantages of this solution are obvious - you can discuss the errors found, get feedback on the analyzer, maybe fix something in the article. It's nice when people understand the benefits of static analysis. The developers noted that the code fragments found by the analyzer are indeed errors, thanked, and said that over time the errors will be fixed. And I, in turn, decided to help them a bit by giving links to these code fragments in the repository. We talked a bit about the use of the analyzer. It’s great that people understand that static analysis needs to be used regularly. I hope that it will be so, and the analyzer will be introduced into the development process.

Here is a mutually beneficial cooperation.

Conclusion

As expected, the analyzer was able to detect a number of suspicious places. And the point is not that someone writes the wrong code or is not qualified enough (of course, this happens, but I think not in this case) - the human factor is to blame. Such is the essence of man; everyone is mistaken. Static analysis tools try to compensate for this shortcoming by indicating errors in the code. Therefore, their regular use is the way to the best code. And it’s better to see it once than hear it 100 times, so I suggest trying PVS-Studio yourself.

Analysis of other Microsoft projects

C ++

• CNTK verification ;
• Check ChakraCore ;
• Check CoreCLR ;
• Check Windows 8 Driver Samples ;
• Validation of Microsoft Word 1.1a ;
• Checking Visual C ++ Libraries: 1 , 2 ;
• Check Casablanca ;

C #

• Check CoreFX ;
• Checking .Net Compiler Platform (Roslyn) ;
• Verification of Code Contracts ;
• Check MSBuild ;
• Check WPF Samples .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. We continue checking Microsoft projects: analysis of PowerShell .