PowerShell is a well-known Microsoft tool—but what's hiding in its source code? Our analyzer is on the hunt for bugs. Let's take a look at it in the article.
PowerShell is a Microsoft automation tool that combines a command-line shell and a scripting language for executing a wide range of tasks. The seemingly intricate PowerShell has become an essential solution for many projects, assisting in efficient system control and process automation.
Developers can leverage PowerShell to customize complex scripts that interact with other systems, greatly optimizing workflows and boosting the efficiency of different applications and services.
Today, we're going to peek behind the scenes of this tool and see what PVS-Studio static analyzer has found there.
Note. We've already checked this project with PVS-Studio analyzer back in 2016. You can read that articlehere.
Based NullReferenceException
No checks for open-source projects go withoutnull dereferencing. And this article? This article is no exception, literally.
Fragment 1
....CimInstanceinstance=GetCimInstanceParameter(cmdlet);nameSpace=ConstValue.GetNamespace(instance.CimSystemProperties.Namespace<=);foreach(CimSessionProxyproxyinproxys){proxy.GetInstanceAsync(nameSpace,instance);}....The PVS-Studio warning:
V3080 Possible null dereference. Consider inspecting 'instance'. CimGetInstance.cs 168
The analyzer warns that theinstance variable can benull when dereferenced. To see if this is truly possible, just check theGetCimInstanceParameter method:
protectedstaticCimInstanceGetCimInstanceParameter(CimBaseCommandcmdlet){if(cmdletisGetCimInstanceCommand){return(cmdletasGetCimInstanceCommand).CimInstance;}elseif(cmdletisRemoveCimInstanceCommand){return(cmdletasRemoveCimInstanceCommand).CimInstance;}elseif(cmdletisSetCimInstanceCommand){return(cmdletasSetCimInstanceCommand).CimInstance;}returnnull;}This method can returnnull and the analyzer is absolutely right—just like in the next fragment.
Fragment 2
....if(_isRunspacePushed){returnRunspaceRef.OldRunspaceasLocalRunspace;<=}if(RunspaceRef==null){returnnull;}....The PVS-Studio warning:
V3095 The 'RunspaceRef' object was used before it was verified against null. Check lines: 781, 784. ConsoleHost.cs 781
Just looking like a wow:RunspaceRef gets dereferenced, and then on the next line—ignoring the curly bracket—it's checked fornull. Another snippet, same story—just different heroes...
Fragment 3
....if(vd==null||mainControlType!=vd.mainControl.GetType()){ActiveTracer.WriteLine("NOT MATCH {0} NAME: {1}",ControlBase.GetControlShapeName(vd.mainControl),(vd!=null?vd.name:string.Empty));continue;}....The PVS-Studio warning:
V3095 The 'vd' object was used before it was verified against null. Check lines: 415, 415. typeDataQuery.cs 415
Let's imagine that thevd variable will benull. The first condition allows it. WhenWriteLine is called, the method will dereferencevd, which we already assumed to benull.
Note. Houston, we're going down!
Moreover, right after that, we checkvd fornull, but by then, it's too late.
Fragment 5
....if(providers!=null&&providers.ContainsKey(providerName)){stringmessage=StringUtil.Format(ConsoleInfoErrorStrings.PSSnapInDuplicateProviders,providerName,psSnapInInfo.Name);s_PSSnapInTracer.TraceError(message);thrownewPSSnapInException(psSnapInInfo.Name,<=message);}SessionStateProviderEntryprovider=newSessionStateProviderEntry(providerName,type,helpfile);if(psSnapInInfo!=null){....}<=....V3095 The 'psSnapInInfo' object was used before it was verified against null. Check lines: 5408, 5412. InitialSessionState.cs 5408 undefined
Honestly, when I saw this warning, I felt like printing it out and hanging it next to my desk in a golden frame. This is aNullReferenceException thrown while throwing another exception! In this fragment, everything seems fine at first: the variable is used before being checked fornull, but unfortunately, things won't play out so smoothly.
Indeed, it could lead to a real program crash when an exception is thrown, but with a nullpsSnapInfo, the program would crash much earlier. Here's a code snippet from a method higher up in the call chain:
if(assembly==null){s_PSSnapInTracer.TraceError("....",psSnapInInfo.Name);<=warning=null;returnnull;}s_PSSnapInTracer.WriteLine("....",psSnapInInfo.Name);<=PSSnapInHelpers.AnalyzePSSnapInAssembly(assembly,psSnapInInfo.Name,psSnapInInfo,moduleInfo:null,outcmdlets,outaliases,outproviders,outhelpFile);Here, thepsSnapInfo variable is dereferenced without a check, but it happens slightly earlier than the previous fragment.
Fragment 6
if(this.ParameterSets!=null){this.CommandType=(CommandTypes)(other.Members["CommandType"].Value<=);this.Module=other.Members["Module"].Value<=asShowCommandModuleInfo;}The PVS-Studio warning:
V3095 The 'other.Members["Module"]' object was used before it was verified against null. Check lines: 81, 91. ShowCommandCommandInfo.cs 81
The issue lies in the dereferencing ofother.Members["Module"]. Before we get anything out of it, we need to check that it even exists. Amusingly, one of the following uses has such a check:
....if(other.Members["Module"]?.ValueisPSObject){....}....I think we've seen enough null dereferences. These are certainly not all such examples in the project, but we still have plenty more to explore :)
Here, I'd like to emphasize that the static analyzer easily detected such errors and warned about them. Someone might say, "Do you really think that your analyzer is needed? Like, for what? I never make mistakes like that!" To that, I'd answer with a great quote: "To err is human." These errors happen to every developer—speaking as someone who has fixed countless crashes caused by a missing? after a potentially null value.
Double-checking
Double-checked locking is a parallel design pattern that helps reduce the overhead of acquiring a lock. First, we check the lock condition without any synchronization, then the thread attempts to get a lock if the result of the recheck indicates that it's necessary.
Sounds straightforward, right? Now, let's take a look at a real code snippet from PowerShell.
Fragment 7
....if(!logInitialized){lock(logLock){if(!logInitialized){DebugHelper.GenerateLog=File.Exists(logFile);logInitialized=true;}}}....Everything looks nice and clear—until we look at the initialization of thelogInitialized field:
....privatestaticboollogInitialized=false;....Notice that? The field is declared without thevolatile modifier, which may cause a change in the operator order during compiler optimization. That's exactly what the analyzer warned about.
The PVS-Studio warning:
V3054 Potentially unsafe double-checked locking. Use volatile variable(s) or synchronization primitives to avoid this. Utils.cs 305
In this specific example, it won't lead to anything catastrophic. Rather, this snippet serves as a reminder to keep a closer eye on situations like this.
Such an error can be incredibly tricky to track down. If it doesn't cause the program to crash, we probably won't notice that the program isn't behaving as intended. Worse yet, operator reordering is quite rare and depends on the processor architecture used, CLR version, and other conditions—making it extremely difficult to reproduce.
Prioritize your priorities
Sometimes things are a little more complicated than we expect, as demonstrated by the following code fragment.
Fragment 8
....intcapacity=length+prependStr?.Length??0+appendStr?.Length??0;returnnewStringBuilder(prependStr,capacity).Append(str,startOffset,length).Append(appendStr).ToString();....You've probably already spotted the issue—the waycapacity is assigned. Developers might have wanted to check if the variable existed and use its length or 0 (when the variable is equal tonull). After that, they would simply add up the values obtained and get the result. What could go wrong? Your turn, analyzer!
The PVS-Studio warning:
V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. StringUtil.cs 246
The addition operator has a higher priority than??! Therefore, the addition will be first, but its result will be checked for existence. Developers just needed to add parenthesis to ensure correct expression processing:
....intcapacity=length+(prependStr?.Length??0)+(appendStr?.Length??0);....Like, for what?
The section title illustrates my exact reaction when I first saw the following code snippets.
Fragment 9
....foreach(stringlogNamein_logNamesMatchingWildcard){queriedLogsQueryMap.Add(logName.ToLowerInvariant(),string.Format(CultureInfo.InvariantCulture,queryOpenerTemplate,queryId++,logName));queriedLogsQueryMapSuppress.Add(logName.ToLowerInvariant(),string.Format(<=CultureInfo.InvariantCulture,suppressOpener,queryId++,logName));}....The PVS-Studio warning:
V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: queryId++, logName. GetEventCommand.cs 1067
Something's wrong with the number of parameters passed to the format string. Well, let's see what kind of string is being so "carefully" formatted here!
privateconststringsuppressOpener="<Suppress>*";
I spent a good amount of time trying to figure out why this happened, but honestly, I still can't think of a single reason. The result? No values are actually inserted into the string because there's simply nowhere for them to go.
Fragment 10
....if(Reader.NodeType==System.Xml.XmlNodeType.Element){UnknownNode((object)o,string.Empty);}else{UnknownNode((object)o,string.Empty);}....The PVS-Studio warning:
V3004 The 'then' statement is equivalent to the 'else' statement. cmdlets-over-objects.xmlSerializer.autogen.cs 1471
I'm not going to put that image in again, okay? I'm sure you get the point...
In this case, the branching tries to seem important, but in reality, it's totally unnecessary.
Note. If you'd like to argue that this bug is more of a refactoring issue, you'd be absolutely right. And if you're wondering why such a small thing is mentioned among serious errors, welcome tomy other article where I go into more detail on why these things matter.
Left to right
Now we explore a couple of interesting warnings related to the use ofEnum.
Fragment 11
....switch(Context.LanguageMode){casePSLanguageMode.ConstrainedLanguage:....break;casePSLanguageMode.NoLanguage:casePSLanguageMode.RestrictedLanguage:....break;}....The PVS-Studio warning:
V3002 The switch statement does not cover all values of the 'PSLanguageMode' enum: FullLanguage. New-Object.cs 190
I've intentionally removed all the details from thisswitch-case because they're not relevant for now. The problem is that thisswitch-case doesn't cover all values of thePSLanguageMode enumeration:
publicenumPSLanguageMode{FullLanguage=0,RestrictedLanguage=1,NoLanguage=2,ConstrainedLanguage=3}TheFullLanguage value isn't processed and it doesn't go into thedefault branch because it simply doesn't exist here...
Fragment 12
[Flags]publicenumPipelineResultTypes{None,Output,Error,Warning,Verbose,Debug,Information,All,Null}The PVS-Studio warning:
V3121 An enumeration 'PipelineResultTypes' was declared with 'Flags' attribute, but does not set any initializers to override default values. Command.cs 779
The analyzer warns that thisEnum is created with theFlags attribute but doesn't override the default values. What's the big deal?
WhenFlags is used, the enumeration will behave like a bit field, a set of flags. These flags are typically assigned values that are powers of 2 to avoid overlap, which is a potential issue when using default values.
In thatenum found in the PowerShell code, the values range from 0 to 8. This leads to some quirky behavior when flags are combined:
_mergeUnclaimedPreviousCommandResults=PipelineResultTypes.Error|PipelineResultTypes.Output;CombiningError andOutput leads to...Warning'! And there are plenty of similar combinations of constants from thisenumthroughout the project code.
ADL (Assert Driven Logging)
I've saved next warnings for dessert—assert for dessert. Okay, here we go.
Fragment 13
....elseif(navigationProvider==null)<={Dbg.Diagnostics.Assert(navigationProvider!=null,<="...."}....V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 2858, 2855. LocationGlobber.cs 2858
The analyzer detected two opposite conditions here, and it was right: the condition inAssert contradicts the one specified in the branching logic. The real question is: why? The code author probably just wanted to generate a message to the console during debugging. But the way it was done is... well, staggering.
I can't deny myself the pleasure of showing another such example.
....if(key==null)<={Dbg.Diagnostics.Assert(key!=null,<="....");return;}....V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 4026, 4023. RegistryProvider.cs 4026
It's the exact same contradictory conditions, serving the same dubious purpose. Sure, you could try to pitch this as a new concept in software engineering, but I'm still not convinced it's anything close to innovation :)
Wrap-up
This concludes our journey through the source code of the PowerShell project. We haven't gone through all the warnings from this project, but definitely the most interesting ones. Any bugs we found will be reported to the developers via GitHub Issues.
Top comments(0)
For further actions, you may consider blocking this person and/orreporting abuse
