PowerShell: new features, same old bugs

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 article here.

Based NullReferenceException

No checks for open-source projects go without null dereferencing. And this article? This article is no exception, literally.

Fragment 1

....
CimInstance instance = GetCimInstanceParameter(cmdlet);
nameSpace = ConstValue.GetNamespace(
  instance.CimSystemProperties.Namespace    <=
);
foreach (CimSessionProxy proxy in proxys)
{
  proxy.GetInstanceAsync(nameSpace, instance);
}
....

The PVS-Studio warning:

V3080 Possible null dereference. Consider inspecting 'instance'. CimGetInstance.cs 168

The analyzer warns that the instance variable can be null when dereferenced. To see if this is truly possible, just check the GetCimInstanceParameter method:

protected static CimInstance GetCimInstanceParameter(CimBaseCommand cmdlet)
{
  if (cmdlet is GetCimInstanceCommand)
  {
    return (cmdlet as GetCimInstanceCommand).CimInstance;
  }
  else if (cmdlet is RemoveCimInstanceCommand)
  {
    return (cmdlet as RemoveCimInstanceCommand).CimInstance;
  }
  else if (cmdlet is SetCimInstanceCommand)
  {
    return (cmdlet as SetCimInstanceCommand).CimInstance;
  }
  return null;
}

This method can return null and the analyzer is absolutely right—just like in the next fragment.

Fragment 2

....
if (_isRunspacePushed)
{
  return RunspaceRef.OldRunspace as LocalRunspace;     <=
}

if (RunspaceRef == null)
{
  return null;
}
....

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 for null. 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 the vd variable will be null. The first condition allows it. When WriteLine is called, the method will dereference vd, which we already assumed to be null.

Note. Houston, we're going down!

Moreover, right after that, we check vd for null, but by then, it's too late.

Fragment 5

....
if (providers != null && providers.ContainsKey(providerName))
{
  string message = StringUtil.Format(
    ConsoleInfoErrorStrings.PSSnapInDuplicateProviders, 
    providerName, 
    psSnapInInfo.Name
  );
  s_PSSnapInTracer.TraceError(message);
  throw new PSSnapInException(
    psSnapInInfo.Name,              <=
    message
  );
}

SessionStateProviderEntry provider = new SessionStateProviderEntry(
  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 a NullReferenceException thrown while throwing another exception! In this fragment, everything seems fine at first: the variable is used before being checked for null, 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 null psSnapInfo, 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;
  return null; 
}

s_PSSnapInTracer.WriteLine("....", psSnapInInfo.Name);      <=
PSSnapInHelpers.AnalyzePSSnapInAssembly(
  assembly, 
  psSnapInInfo.Name, 
  psSnapInInfo, 
  moduleInfo: null, 
  out cmdlets, 
  out aliases, 
  out providers, 
  out helpFile
);

Here, the psSnapInfo 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   <=
    as ShowCommandModuleInfo;
}

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 of other.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"]?.Value is PSObject) {....}
....

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 the logInitialized field:

....
private static bool logInitialized = false;
....

Notice that? The field is declared without the volatile 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

....
int capacity = length + 
               prependStr?.Length ?? 0 + 
               appendStr?.Length ?? 0;

return new StringBuilder(prependStr, capacity)
  .Append(str, startOffset, length)
  .Append(appendStr)
  .ToString();
....

You've probably already spotted the issue—the way capacity is assigned. Developers might have wanted to check if the variable existed and use its length or 0 (when the variable is equal to null). 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:

....
int capacity = 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 (string logName in _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!

private const string suppressOpener = "*";

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 to my 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 of Enum.

Fragment 11

....
switch (Context.LanguageMode)
{
  case PSLanguageMode.ConstrainedLanguage:
    ....
    break;

  case PSLanguageMode.NoLanguage:
  case PSLanguageMode.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 this switch-case because they're not relevant for now. The problem is that this switch-case doesn't cover all values of the PSLanguageMode enumeration:

public enum PSLanguageMode
{
  FullLanguage = 0,
  RestrictedLanguage = 1,
  NoLanguage = 2,
  ConstrainedLanguage = 3
}

The FullLanguage value isn't processed and it doesn't go into the default branch because it simply doesn't exist here...

Fragment 12

[Flags]
public enum PipelineResultTypes
{
  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 this Enum is created with the Flags attribute but doesn't override the default values. What's the big deal?

When Flags 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 that enum 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;

Combining Error and Output 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

....
else if (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 in Assert 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.