# We continue checking Microsoft projects: analysis of PowerShell It has become a "good tradition" for Microsoft to make their products open\-source: CoreFX, \.Net Compiler Platform \(Roslyn\), Code Contracts, MSBuild, and other projects\. For us, the developers of PVS\-Studio analyzer, it's an opportunity to check well\-known projects, tell people \(including the project authors themselves\) about the bugs we find, and additionally test our analyzer\. Today we are going to talk about the errors found in another project by Microsoft, PowerShell\. ![0447_PowerShell/image1.png](https://import.viva64.com/docx/blog/0447_PowerShell/image1.png) ## PowerShell PowerShell is a cross\-platform project by Microsoft consisting of a command\-line shell and associated scripting language built on the Microsoft [\.NET Framework](https://en.wikipedia.org/wiki/.NET_Framework) and integrated with it\. PowerShell also provides convenient access to [COM](https://en.wikipedia.org/wiki/Component_Object_Model), [WMI](https://en.wikipedia.org/wiki/Windows_Management_Instrumentation), and [ADSI](https://msdn.microsoft.com/en-us/library/aa772170(v=vs.85).aspx), and enables administrators to perform various tasks in a single environment both on local and remote Windows systems by running regular command\-line commands\. The project code can be downloaded from the [GitHub repository](https://github.com/powershell/powershell)\. ## PVS\-Studio According to the statistics of the project repository, 93% of the code is written in C\#\. ![0447_PowerShell/image2.png](https://import.viva64.com/docx/blog/0447_PowerShell/image2.png) The project was analyzed with PVS\-Studio static code analyzer\. The version we were using is currently in the development process, so it is newer than PVS\-Studio 6\.08 but it's not PVS\-Studio 6\.09 either\. This approach enables us to put the new version through more extensive testing and fix possible defects\. It does not replace the multilevel system of tests, of course \(read about the seven testing techniques in the [article](https://pvs-studio.com/en/blog/posts/0415/) discussing the development of the Linux\-version\), but rather is another way of testing the tool\. The up\-to\-date version of the analyzer can be downloaded [here](https://pvs-studio.com/en/pvs-studio/download/)\. ## Preparing for analysis I updated the analyzer and downloaded the project's code, so everything was ready to go\. Well, sometimes things get complicated as early as at the stage of preparing a project for analysis, i\.e\. at the building stage\. It is recommended that you build projects before analyzing them\. Why does it matter? The analyzer will have access to more information that way, so it will be able to provide a deeper analysis\. The most common \(and convenient\) way of using PVS\-Studio is to run it from the Visual Studio IDE\. It's quick, easy, and convenient\. For PowerShell, however, it's a problem\. It turned out that the authors themselves didn't recommend using Visual Studio to build the project\. They say it straightforward on GitHub: "We do not recommend building the PowerShell solution from Visual Studio\." Well, I couldn't resist the temptation to build and check it in Visual Studio, so I gave it a try anyway\. This is what I got: ![0447_PowerShell/image4.png](https://import.viva64.com/docx/blog/0447_PowerShell/image4.png) Figure 1\. Project compilation errors \(click to enlarge\) when analyzing PowerShell from Visual Studio\. Well, that's sad\. What did it mean in my situation? That I wouldn't be able to test all the features of the analyzer on this project\. Then you have two scenarios\. **Scenario 1\. Check the project without building it\.** A project would't build? OK, let's check it as it is\. What are the pros of this approach? You don't have to waste time figuring out the problem and trying various tricks to get the project built\. It does help you save time; moreover, it is not guaranteed that your tricks will work out after all\. The cons of this approach are also clear\. First, the analysis will be incomplete; some bugs will slip from the analyzer\. You may also get a certain number of false positives\. Second, it makes the estimate of the false/genuine warnings ratio pointless, as it may vary greatly for the built version\. However, even this scenario allows you to find a decent number of errors and write an article\. **Scenario 2\. Figure it all out and get the project built\.** The pros and cons of this approach are opposite to those of the previous one\. Yes, you'll have to spend more time on building, but it's not guaranteed that it will work out\. If you do succeed, however, you'll be able to analyze the code more thoroughly and maybe find some interesting bugs\. There's no definite suggestion about what way to choose; everyone decides for themselves\. I struggled with the project for a while, trying to build it, and finally decided to go "as is"\. This approach was good enough for my goal to write an article\. **Note\. **While it can't be built from Visual Studio, the project can be easily built by using the script \(_build\.sh_\) located in the root directory\. **Note** **2\.** One of the developers \(many thanks to him\) told me that the \*\.sln\-file was intended to make it more comfortable to work with the project, but it wasn't meant to be used for building, which is just another argument for choosing the first approach\. ## Analysis results **Duplicate subexpressions** Projects that trigger no [V3001](https://pvs-studio.com/en/docs/warnings/v3001/) warnings do deserve a medal\. PowerShell, unfortunately, wouldn't get it, and here's why: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'BaseMaximumVersion \!\= null' to the left and to the right of the '&&' operator\. System\.Management\.Automation ImportModuleCommand\.cs 1663 The _BaseMaximumVersion_ reference is tested for _null _twice, but it's obviously the _BaseMinimumVersion_ reference that should be checked in the second case\. If you are lucky, the program may run for a long time without this error ever showing up, but when it does occur, the information about _BaseMinimumVersion_ will never be included in the error message formed when the exception is thrown, as the _BaseMinimumVersion_ reference will be null\. As a result, some portion of useful information will be lost\. Note that I fixed the code formatting in this example to make the error easier to notice\. In the original code, however, the whole condition is written in one line, which is another example of why good code formatting is so important: not only does it make the code easier to read and understand, but it also makes errors easier to see\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions to the left and to the right of the '&&' operator\. System\.Management\.Automation serverremotesession\.cs 633 Again, there is a typo that causes one check to execute twice\. What should be checked in the second case is most likely the constant field _MaxRunspaces_ of the static class _RemoteDataNameStrings_\. **Unused return value** There are errors that have to do with unused method return values\. The reasons, as well as implications, vary a lot\. Sometimes programmers forget that objects of type _String_ are immutable and that string\- modifying methods return a new string rather than changing the existing one\. In the same way, using LINQ yields a new collection\. Errors of this type were also found in PowerShell\. ```cpp private CatchClauseAst CatchBlockRule(.... ref List errorAsts) { .... if (errorAsts == null) { errorAsts = exceptionTypes; } else { errorAsts.Concat(exceptionTypes); // <= } .... } ``` **PVS\-Studio warning:** [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Concat' is required to be utilized\. System\.Management\.Automation Parser\.cs 4973 Note that the _errorAsts_ parameter is used with the _ref_ keyword, which implies that the reference gets changed in the method body\. The logic of this code is simple: if the _errorAsts_ reference is null, then it is assigned with a reference to another collection; otherwise, the elements of the _exceptionTypes_ collection are added to the existing one\. However, the second part doesn't work properly\. The _Concat_ method returns a new collection without modifying the existing one, so the _errorAsts_ collection will remain unchanged, while the new one \(containing the elements _errorAsts_ and _exceptionTypes_\) will be ignored\. There are two ways to fix this defect: * Use the _AddRange_ method of the _List_ class to add the new elements to the existing list; * Use the return value of the _Concat_ method and make sure that you cast it to the required type by calling to the _ToList_ method\. **Checking a wrong reference after using the 'as' operator** The gold medal goes to the [V3019](https://pvs-studio.com/en/docs/warnings/v3019/) diagnostic rule\! I'm not sure about all projects, but almost every C\#\-project that I checked and discussed in my articles had this bug\. Our long\-time readers must have learned this rule by heart: when casting a reference to another type by using the _as _operator, always make sure that you test the resulting reference, not the original one, for _null_\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/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 The result of casting _j_ to the _PSRemotingChildJob_ type is written to the _child_ reference, which means that this reference may be assigned with the _null _value \(if the original reference is _null_ or if the cast failed\)\. The programmer, however, checks the original reference, _j_, and then attempts to access the _Runspace_ property of the _child_ object\. So, if _j \!\= null_ and _child \=\= null_, the _j \=\= null_ check won't help and you'll get a _NullReferenceException_ when accessing the instance members of the resulting reference\. Two more defects of this type: * 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 operation order** ```cpp private void CopyFileFromRemoteSession(....) { .... ArrayList remoteFileStreams = GetRemoteSourceAlternateStreams(ps, sourceFileFullName); if ((remoteFileStreams.Count > 0) && (remoteFileStreams != null)) .... } ``` **PVS\-Studio warning:** [V3027](https://pvs-studio.com/en/docs/warnings/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 If you are lucky, the code will execute successfully; if not, you'll get a _NullReferenceException_ when attempting to dereference a null reference\. The _remoteFileStreams \!\= null_ subexpression doesn't actually do anything, nor does it protect the code from the exception\. Obviously, you need to swap the subexpressions to make the code work properly\. Well, we are all humans, and we all make mistakes, and static analyzers are the tools whose purpose is to catch our mistakes\. **Potential null dereference** ```cpp internal bool SafeForExport() { return DisplayEntry.SafeForExport() && ItemSelectionCondition == null || ItemSelectionCondition.SafeForExport(); } ``` **PVS\-Studio warning:** [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'ItemSelectionCondition'\. System\.Management\.Automation displayDescriptionData\_List\.cs 352 There is a risk of getting a _NullReferenceException_ when executing this code\. The _ItemSelectionCondition\.SafeForExport\(\)_ subexpression will be evaluated only if the first subexpression evaluates to _false_\. Therefore, if _DisplayEntry\.SafeForExport\(\) _returns _false_ and _ItemSelectionCondition _\=\= _null_, the second subexpression, _ItemSelectionCondition\.SafeForExport\(\)_, will be evaluated, and that's where the null dereference will occur \(and raise the exception\)\. I found another similar code fragment in this project\. The corresponding message: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'EntrySelectedBy'\. System\.Management\.Automation displayDescriptionData\_Wide\.cs 247 Another example\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'providerName'\. System\.Management\.Automation SessionStateProviderAPIs\.cs 1004 Every now and then, you stumble upon code like that\. The programmer intended an exception to be of one type, but it ended up being of another type\. Why does it happen? In our example, the programmer tests the _providerName_ reference for _null_, but later, when forming an exception object, they call to the instance method _ToString _of the same reference\. It will result in forming a _NullReferenceException_ instead of the intended _ProviderNotFoundException_\. There was another similar fragment: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'job'\. System\.Management\.Automation PowerShellETWTracer\.cs 1088 **Using a reference before testing it for _null_** ```cpp internal ComplexViewEntry GenerateView(....) { _complexSpecificParameters = (ComplexSpecificParameters)inputParameters.shapeParameters; int maxDepth = _complexSpecificParameters.maxDepth; .... if (inputParameters != null) mshParameterList = inputParameters.mshParameterList; .... } ``` **PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'inputParameters' object was used before it was verified against null\. Check lines: 430, 436\. System\.Management\.Automation FormatViewGenerator\_Complex\.cs 430 The _inputParameters \!\= null_ check implies that the reference being checked may be _null_\. The programmer wanted to play safe to make sure they wouldn't get a _NullReferenceException_ when accessing the _mshParameterList_ field\. This is a right decision, except that they already accessed another instance field of the same object, _shapeParameters_, earlier\. Since _inputParameters_ doesn't change between these two operations, the null check won't help if the reference has been null from the beginning\. Another similar case: ```cpp public CommandMetadata(CommandMetadata other) { .... _parameters = new Dictionary( other.Parameters.Count, StringComparer.OrdinalIgnoreCase); // deep copy if (other.Parameters != null) .... } ``` **PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'other\.Parameters' object was used before it was verified against null\. Check lines: 189, 192\. System\.Management\.Automation CommandMetadata\.cs 189 The programmer is testing the _Parameters_ property of the _other_ object for _null_, but they already accessed the instance property _Count_ a couple of lines earlier\. Something is obviously wrong here\. **Unused constructor parameter** It's nice to see new diagnostic rules show real results right after we add them to the tool\. [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) is one such diagnostic\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'errorCategory\_Message' is not used\. System\.Management\.Automation ErrorPackage\.cs 1125 Method _PopulateProperties_ is called in the _ErrorRecord_ constructor to initialize the fields and perform some other operations\. The analyzer warns us that one of the constructor's parameters, _errorCategory\_Message_, is not used\. Indeed, the _errorDetails\_Message _argument is passed twice when calling to the _PopulateProperties_ method, while _errorCategory\_Message_ is not passed at all\. Checking out the parameter list of _PopulateProperties_ confirms that we are dealing with an error\. **An always false condition** One of PVS\-Studio's features that help us implement complex diagnostic rules and find complicated bugs is the so called virtual values, which allow the analyzer to track the possible ranges of values that a variable can take at a particular time of execution\. For more information on that feature, see the article [Searching for errors by means of virtual values evaluation](https://pvs-studio.com/en/blog/posts/0394/)\. This mechanism underlies such diagnostics as [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) and [V3063](https://pvs-studio.com/en/docs/warnings/v3063/), which often help us discover interesting errors\. One such error was found in this project too: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'stateInfo\.State \=\= RunspacePoolState\.Disconnected' is always false\. System\.Management\.Automation RunspacePoolInternal\.cs 581 The analyzer insists that the _stateInfo\.State \=\= RunspacePoolState\.Disconnected_ expression is always false\. Is it really so? Sure\! I wouldn't cite this example if it were otherwise\. The programmer made a mistake in the preceding condition: if _stateInfo\.State \=\= RunspacePoolState\.Disconnected_, then the previous _if _statement will execute all the time\. To fix the error, you just need to swap the last two _if_ \(_else if_\) statements\. ## More bugs? Yes, there are lots of other suspicious fragments\. Our regular readers know that we don't usually discuss all of the errors found\. As for this project, there are probably not so many bugs left to make this article as big as the [one about the check of Mono](https://pvs-studio.com/en/blog/posts/csharp/0431/), but there is still some material that could be included\. It's the project authors who ought to be most interested in a complete list of warnings; to all the rest, I just show the most interesting errors and defects\. ## "Have you told the developers about these bugs?" Oddly enough, people still ask us this question from time to time\. We always inform the developers about the bugs we find, but this time I decided to go a little further\. I talked to one of the developers \(Sergey, hi\!\) personally via [Gitter](https://gitter.im/)\. The advantages of such a solution are obvious \- we may discuss the bugs found, get feedback on the analyzer, there might be something to correct in the article\. It's great when people understand the usefulness of static analysis\. The developers told us that the detected code fragments are bugs indeed, thanked a lot and said that they would fix the bugs over the time\. In turn, I decided to help them by giving links to these code fragments in the repository\. We also had a talk about the use of the analyzer\. It's great, when people understand that static analysis should be used regularly\. I hope it will be so, and the analyzer will embedded into the development process\. It was a nice mutually beneficial cooperation\. ## Conclusion As I'd expected, the analyzer managed to find quite a lot of suspicious fragments in PowerShell\. The point of this article, however, is not about people writing incorrect code or lacking skill \(it does happen at times, of course, but obviously not in this case\); it's just that it is the human error which is to blame\. It's the essence of human \- everyone makes mistakes\. Static analysis tools are designed to make up for this flaw of ours by catching errors in program code\. That's why regular use of such tools is the path to better code\. A picture is worth a thousand words, so welcome to [try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) with your own code\. ## Analysis of other Microsoft projects ![0447_PowerShell/image5.png](https://import.viva64.com/docx/blog/0447_PowerShell/image5.png) ### C\+\+ * [Check of CNTK](https://pvs-studio.com/en/blog/posts/cpp/0372/); * [Check of ChakraCore](https://pvs-studio.com/en/blog/posts/cpp/0370/); * [Check of CoreCLR](https://pvs-studio.com/en/blog/posts/cpp/0310/); * [Check of Windows 8 Driver Samples](https://pvs-studio.com/en/blog/posts/cpp/0199/); * [Check of Microsoft Word 1\.1a](https://pvs-studio.com/en/blog/posts/cpp/0245/); * Check of Visual C\+\+ libraries: [1](https://pvs-studio.com/en/blog/posts/cpp/0163/), [2](https://pvs-studio.com/en/blog/posts/cpp/0288/); * [Check of Casablanca](https://pvs-studio.com/en/blog/posts/0189/); ### C\# * [Check of CoreFX](https://pvs-studio.com/en/blog/posts/csharp/0365/); * [Check of \.Net Compiler Platform \(Roslyn\)](https://pvs-studio.com/en/blog/posts/csharp/0363/); * [Check of Code Contracts](https://pvs-studio.com/en/blog/posts/csharp/0361/); * [Check of MSBuild](https://pvs-studio.com/en/blog/posts/csharp/0424/); * [Check of WPF Samples](https://pvs-studio.com/en/blog/posts/csharp/0407/)\.