We continue to check Microsoft projects: PowerShell analysis

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

Powershell

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

The project code is available for download from the repository on GitHub .

PVS-Studio

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

The PVS-Studio static code analyzer was used for the verification. A version was being developed. That is, it is no longer PVS-Studio 6.08, but also not PVS-Studio 6.09. Such an approach allows a wider approach to testing the most recent version of the analyzer, and, if possible, correct the defects found. Of course, this does not negate the use of a multi-level test system (see the seven testing methods in the article on the development of the Linux version), but is another way to test the analyzer.

The current version of the analyzer can be downloaded here .

Preparation for analysis

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

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

As it turned out, the developers themselves do not recommend using the Visual Studio development environment for building the project, which they directly write on GitHub about: “We would not recommend building the PowerShell solution from Visual Studio.”

But the temptation of assembling through Visual Studio and checking out from under it was too great, so I decided to try. The result is shown in the figure below:

Figure 1. Compile project errors using Visual Studio.

Unpleasant What did it mean to me? The fact that just because all the possibilities of the analyzer on the project will not be able to reveal. There are several possible scenarios.

Scenario 1. Checking an unassembled project.

We tried to build a project. Not going? Well, okay, check it out.

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

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

However, even with this scenario, you can find enough errors and write about this article.

Scenario 2. Deal and assemble the project.

The pros and cons are opposite to those described above. Yes, you have to spend more time on assembly, but it’s not a fact that the desired result will be achieved. But if successful, you will be able to check the source code more thoroughly and, perhaps, find some more interesting errors.

Unambiguous advice on how to proceed is not here, everyone decides for himself.

Having suffered from 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 going out from under Visual Studio, it is quite calmly going through a script ( build.sh ), which lies at the root.

Note 2. One of the developers (thanks a lot to him for this) suggested that the * .sln-file is necessary for ease of operation, 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 the warning V3001 does not find suspicious places should be issued medals. PowerShell, alas, in this case would have remained without a medal and that is 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' operator. System.Management.Automation ImportModuleCommand.cs 1663

Link to GitHub code .

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

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

 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 operator. System.Management.Automation serverremotesession.cs 633

Link to GitHub code .

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

Unused value returned by method

There are errors characteristic of the fact that the return value of the method is not used. The causes, as well as the consequences, can be very different. Sometimes a programmer forgets that String type objects are immutable, and string editing methods return a new sink 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 met here.

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

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

Link to GitHub code .

Immediately I want to draw attention to the fact that the errorAsts parameter is used with the ref keyword, which implies changing the link in the method body. The code logic is simple - if the errorAsts reference 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, with the second part came the pad. The Concat method returns a new collection without modifying the existing one. Thus, the errorAsts collection will 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 List class's AddRange method, which will add new items to the current list;
• Use the result of the Concat method, without forgetting to call the ToList method, to bring it to the desired type.

Checking the wrong link after casting using the as operator

Gold Medal Diagnostic Rule V3019 ! Surely I will not say, but almost in all C # projects, on which I wrote articles, this error was encountered. Regular readers should already learn by heart about the need to carefully recheck whether you are checking the link for null equality after casting with the as operator.

 internal List<Job> 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 GitHub code .

The result of casting j to type PSRemotingChildJob is written to the child reference, which means that the value null can be written there (if the initial reference is null or if the casting could not be performed). However, the initial reference, j , is checked below for the null inequality, and the appeal to the Runspace property of the child object goes even lower. Thus, if j! = Null , and child == null , checking j == null will not help, and an exception of the type NullReferenceException will be generated when accessing the member members of the derived reference.

Two more similar places met:

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

Invalid order of operations

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

PVS-Studio warning : V3027 The variable 'remoteFileStreams' has been used. System.Management.Automation FileSystemProvider.cs 4126

Link to GitHub code .

If you’re lucky, the code will work fine; if you’re unlucky, when you try to dereference a zero reference, you will get an exception of type NullReferenceException . The remoteFileStreams! = Null subexpression does not play any role in this expression, nor does it protect against an exception. Obviously, to work properly, you need to swap subexpressions in some places.

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

Possible dereference of the zero 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 GitHub code .

Potential exception of type NullReferenceException . The ItemSelectionCondition.SafeForExport () subexpression will only be evaluated 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 calculated - ItemSelectionCondition.SafeForExport () , where the problem of dereferencing the zero reference will arise (and as a result, an exception will be generated).

A similar code met once again. Corresponding warning: V3080 Possible null dereference. Consider inspecting 'EntrySelectedBy'. System.Management.Automation displayDescriptionData_Wide.cs 247

Another case.

 internal Collection<ProviderInfo> 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 GitHub code .

Sometimes there is a code like this - they wanted to generate 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 instance method ToString is called on the same reference. The result will be an NullReferenceException type exception , not a ProviderNotFoundException , as planned.

Another similar code snippet met. Corresponding warning: V3080 Possible null dereference. Consider inspecting a 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 verified against null. Check lines: 430, 436. System.Management.Automation FormatViewGenerator_Complex.cs 430

Link to GitHub code .

The inputParameters! = Null check implies that the link being checked can be null . Reinsured so that when accessing the mshParameterList field, you do not get a NullReferenceException exception. All right Here are just above already turned to another instance field of the same object - shapeParameters . Since inputParameters between these two operations does not change, if the link was initially null , checking for null inequality will not save the exception.

A similar case:

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

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

Link to GitHub code .

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

Unused constructor parameter

It is nice to see the results of the new diagnostic rules immediately after their appearance. It happened with the diagnostics 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 GitHub code .

In the ErrorRecord constructor, the PopulateProperties method is called , which performs field initialization and some other actions. The analyzer warns that one of the constructor parameters, errorCategory_Message, is not used in its body. Indeed, if you take a close look at the call to the PopulateProperties method, you will notice that the errorDetails_Message argument is passed 2 times to the method, but the errorCategory_Message is not passed. We look at the parameters of the PopulateProperties method and make sure that there is an error.

Condition which is always false

One of the features of PVS-Studio, which 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 the calculation of virtual values" . Based on this mechanism, diagnostic rules such as V3022 and V3063 are built . With their help, it is often possible to find interesting errors. It happened this time too, therefore, 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 GitHub code .

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

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

More mistakes?

Yes, there are many more places that the analyzer found suspicious. Those who regularly read our articles know that often we do not write out all the errors. Maybe up to the size of the article about checking Mono it would not have come, but the writing material still remained. The greatest interest in all warnings should arise from the developers, the rest of the readers, I just try to show the most interesting suspicious places.

“Did you tell the developers?”

Oddly enough, we are still periodically asked this question. We always inform the developers about the errors 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 about the analyzer, maybe something to correct in the article. It's nice when people understand the benefits of static analysis. The developers noted that the code snippets found by the analyzer were indeed errors, thanked them, and said that errors would be corrected over time. And I, in turn, decided to help them a little by giving links to these code fragments in the repository. We talked a little about the use of the analyzer. It's great that people understand that static analysis should be used regularly. I hope it will be so, and the analyzer will be embedded in 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 has insufficient qualifications (of course, it happens, but, I think, not in this case) - the human factor is to blame for everything. This is the essence of man, everyone is mistaken. Static analysis tools try to compensate for this flaw by pointing out errors in the code. Therefore, their regular use is the way to the best code. And it's better to see once than to hear 100 times, so I suggest trying PVS-Studio on your own.

Analysis of other Microsoft projects

C ++

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

C #

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

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 .