Build to order? Checking MSBuild for the second time

MSBuild is a popular open-source build platform created by Microsoft. Developers all over the world use MSBuild. In 2016, we checked it for the first time and found several suspicious places. Can we find anything this time? Let's see!

Introduction

Since the previous check, the project has grown a lot. Our analyzer has become more advanced, too. This only makes this task more interesting! Despite the high quality of the MSBuild product and the well-known name of its creator, we again managed to find some issues in MSBuild's source code. The project is almost entirely written in C#. You can see it on GitHub. We took the code from this commit.

To compare the analysis results, let's look at two diagrams:

After the second check, the analyzer issued 839 warnings. Last time, there were only 262. The number of Medium-level warnings has increased fourfold. Warnings of this level of certainty prevail in our article. The number of Low-level warnings increased by about two and a half times. High-level warnings increased by almost two times.

Six years have passed since the first check — and we, the developers of PVS-Studio, weren't wasting our time :). Since the first MSBuild check, we have added 64 GA (General Analysis) and 23 OWASP diagnostics to the C# analyzer. We've also enhanced existing diagnostic rules. But not only C# developers have done significant work. If you want to track how the analyzer has been changing — click here.

Let's look at the most interesting warnings.

Wrong increment

Issue 1

private string ParsePropertyOrItemMetadata()
{
  int start = parsePoint;
  parsePoint++;

  if (parsePoint < expression.Length && expression[parsePoint] != '(')
  {
    errorState = true;
    errorPosition = start + 1;
    errorResource = "IllFormedPropertyOpenParenthesisInCondition";
    unexpectedlyFound = Convert
                        .ToString(expression[parsePoint],
                                  CultureInfo.InvariantCulture);
    return null;
  }

  parsePoint = ScanForPropertyExpressionEnd(expression, parsePoint++); // <=
  ....
}

PVS-Studio's warning: V3133 Postfix increment for variable 'parsePoint' is senseless because this variable is overwritten. Scanner.cs 310

Perhaps the developer expected the ScanForPropertyExpressionEnd to accept the incremented parsePoint value as the second argument. Unfortunately, this won't happen. The problem is in using the postfix notation for the increment. In this case, first the variable's current value is returned, and only then is it incremented.

Therefore, the initial value of parsePoint is passed to the method. The value obtained after executing ScanForPropertyExpressionEnd is assigned to the parsePoint variable. Because of this, the variable's increased value is overwritten. So, the increment operation does not affect anything in this code fragment.

This issue can be fixed by changing the postfix notation to the prefix one:

parsePoint = ScanForPropertyExpressionEnd(expression, ++parsePoint);

Suspicious logical expressions

Issue 2

private static int ResolveAssemblyNameConflict(...., ....);
{
  ....
  if (   leftConflictReference.IsPrimary 
      && !rightConflictReference.IsPrimary)
  {
    ....  
  }
  else if (   !leftConflictReference.IsPrimary 
           && rightConflictReference.IsPrimary)
  {
    ....  
  }
  else if (   !leftConflictReference.IsPrimary 
           && !rightConflictReference.IsPrimary)
  {
    ....
    bool isNonUnified =   leftConflictReference.IsPrimary   // <=
                       && rightConflictReference.IsPrimary; // <=

    bool leftConflictLegacyUnified =   !isNonUnified        // <=
                                    && assemblyReference0
                                       .reference
                                       .IsPrimary;

    bool rightConflictLegacyUnified =    !isNonUnified      // <=
                                      && assemblyReference1
                                         .reference
                                         .IsPrimary;
    ....
  }
}

The analyzer issued three warnings for this code fragment:

V3022 Expression 'leftConflictReference.IsPrimary && rightConflictReference.IsPrimary' is always false. ReferenceTable.cs 2388
V3063 A part of conditional expression is always true if it is evaluated: !isNonUnified. ReferenceTable.cs 2389
V3063 A part of conditional expression is always true if it is evaluated: !isNonUnified. ReferenceTable.cs 2390

The second and the third warnings are a consequence of the problem marked by the first warning. Let's look at the condition of the last if. As we can see, the if body's leftConflictReference.IsPrimary and rightConflictReference.IsPrimary values are always false.

The isNonUnified variable is initialized with the value obtained after leftConflictReference.IsPrimary && rightConflictReference.IsPrimary is executed. These variables are both false. Therefore, isNonUnified is always false.

Then isNonUnified is used as a part of an expression to initialize two more variables:

bool leftConflictLegacyUnified =   !isNonUnified 
                                && assemblyReference0.reference
                                                     .IsPrimary;

bool rightConflictLegacyUnified =    !isNonUnified 
                                  && assemblyReference1.reference
                                                       .IsPrimary;

Therefore, the value of these variables depends only on the right operand of the '&&' operator. The code can be simplified by replacing the if body with the following:

bool leftConflictLegacyUnified = assemblyReference0.reference.IsPrimary;
bool rightConflictLegacyUnified = assemblyReference1.reference.IsPrimary;

Most likely, the code doesn't contain any errors, just some unnecessary operations. However, we cannot ignore the analyzer's warning — it's not a false positive. My teammate wrote an article about that, I highly recommend you to read it.

Issue 3

private bool VerifyArchitectureOfImplementationDll(string dllPath,
                                                   string winmdFile)
{
  try
  {
    UInt16 machineType = _readMachineTypeFromPEHeader(dllPath);
    SystemProcessorArchitecture dllArchitecture = 
                                  SystemProcessorArchitecture.None;
    switch (machineType)
    {
      case NativeMethods.IMAGE_FILE_MACHINE_AMD64:
        dllArchitecture = SystemProcessorArchitecture.Amd64;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_ARM:
      case NativeMethods.IMAGE_FILE_MACHINE_ARMV7:
        dllArchitecture = SystemProcessorArchitecture.Arm;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_ARM64:
        dllArchitecture = (SystemProcessorArchitecture) 6; 
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_I386:
        dllArchitecture = SystemProcessorArchitecture.X86;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_IA64:
        dllArchitecture = SystemProcessorArchitecture.IA64;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_UNKNOWN:
        dllArchitecture = SystemProcessorArchitecture.None;
        break;
      default:
        ....
        break;
    }

    // If the assembly is MSIL or none it can work anywhere
    // so there does not need to be any warning ect.
    if (   dllArchitecture == SystemProcessorArchitecture.MSIL     // <=
        || dllArchitecture == SystemProcessorArchitecture.None)
    {
      return true;
    }
    ....
  }
}

PVS-Studio's warning: V3063 A part of conditional expression is always false if it is evaluated: dllArchitecture == SystemProcessorArchitecture.MSIL. ReferenceTable.cs 2968

The dllArchitecture variable is initialized by the SystemProcessorArchitecture.None value. This variable can be assigned another value only in the switch body. If you look closely, you can notice that SystemProcessorArchitecture.MSIL isn't assigned in any of the case blocks. Note that (SystemProcessorArchitecture) 6 doesn't match the MSIL element. There is no assignment of this variable in the default branch.

Below switch, there's a check that dllArchitecture equals SystemProcessorArchitecture.MSIL. Looks weird — dllArchitecture can't have this value.

The code also contains a comment that explains a part of the condition: "If the assembly is MSIL or none it can work anywhere so there does not need to be any warning ect." So, the check wasn't accidental. This makes the code very suspicious.

Issue 4

Can you find an error here?

internal BuildParameters(BuildParameters other, bool resetEnvironment = false)
{
  ErrorUtilities.VerifyThrowInternalNull(other, nameof(other));
  _buildId = other._buildId;
  _culture = other._culture;
  _defaultToolsVersion = other._defaultToolsVersion;
  _enableNodeReuse = other._enableNodeReuse;
  _buildProcessEnvironment = resetEnvironment
    ? CommunicationsUtilities.GetEnvironmentVariables()
    : other._buildProcessEnvironment != null
      ? new Dictionary<string, string>(other._buildProcessEnvironment)
      : null;
  _environmentProperties = ....
  _forwardingLoggers = ....
  _globalProperties = ....
  HostServices = other.HostServices;
  _loggers = other._loggers != null ? new List<ILogger>(other._loggers) : null;
  _maxNodeCount = other._maxNodeCount;
  _memoryUseLimit = other._memoryUseLimit;
  _nodeExeLocation = other._nodeExeLocation;
  NodeId = other.NodeId;
  _onlyLogCriticalEvents = other._onlyLogCriticalEvents;
  BuildThreadPriority = other.BuildThreadPriority;
  _toolsetProvider = other._toolsetProvider;
  ToolsetDefinitionLocations = other.ToolsetDefinitionLocations;
  _toolsetProvider = other._toolsetProvider;
  _uiCulture = other._uiCulture;
  DetailedSummary = other.DetailedSummary;
  _shutdownInProcNodeOnBuildFinish = other._shutdownInProcNodeOnBuildFinish;
  ProjectRootElementCache = other.ProjectRootElementCache;
  ResetCaches = other.ResetCaches;
  LegacyThreadingSemantics = other.LegacyThreadingSemantics;
  SaveOperatingEnvironment = other.SaveOperatingEnvironment;
  _useSynchronousLogging = other._useSynchronousLogging;
  _disableInProcNode = other._disableInProcNode;
  _logTaskInputs = other._logTaskInputs;
  _logInitialPropertiesAndItems = other._logInitialPropertiesAndItems;
  WarningsAsErrors = ....
  WarningsNotAsErrors = ....
  WarningsAsMessages = ....
  _projectLoadSettings = other._projectLoadSettings;
  _interactive = other._interactive;
  _isolateProjects = other._isolateProjects;
  _inputResultsCacheFiles = other._inputResultsCacheFiles;
  _outputResultsCacheFile = other._outputResultsCacheFile;
  DiscardBuildResults = other.DiscardBuildResults;
  LowPriority = other.LowPriority;
  ProjectCacheDescriptor = other.ProjectCacheDescriptor;
}

Something tells me that you either didn't find it or you did but spent hours searching. Let's shorten this code fragment a bit:

internal BuildParameters(BuildParameters other, bool resetEnvironment = false)
{
  ....
  _toolsetProvider = other._toolsetProvider;
  ToolsetDefinitionLocations = other.ToolsetDefinitionLocations;
  _toolsetProvider = other._toolsetProvider;
  ....
}

PVS-Studio's warning: V3008 The '_toolsetProvider' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 284, 282. BuildParameters.cs 284

Now you can easily find an issue here. The _toolsetProvider field is assigned a value twice. This is absolutely pointless. Hard to say whether it's really an error. It's unclear if there should be something else instead of one of the _toolsetProvider assignments. Perhaps, this is an unnecessary assignment, but it's better to avoid such cases.

This issue is a good example of how static analysis can help. The human eye will almost always fail to find a problem in such code, but the static analyzer won't.

Mixed-up arguments

Issue 5

private SdkResult CloneSdkResult(SdkResult sdkResult)
{
  if (!sdkResult.Success)
  {
    return new SdkResult(sdkResult.SdkReference, 
                         sdkResult.Warnings, 
                         sdkResult.Errors);
  }
  ....
}

PVS-Studio's warning: V3066 Possible incorrect order of arguments passed to 'SdkResult' constructor: 'sdkResult.Warnings' and 'sdkResult.Errors'. InternalEngineHelpers.cs 83

To understand this warning, we need to inspect the SdkResult constructor declaration first:

public SdkResult(SdkReference sdkReference,
                 IEnumerable<string> errors,
                 IEnumerable<string> warnings)
{
  Success = false;
  SdkReference = sdkReference;
  Errors = errors;
  Warnings = warnings;
}

A rather rare and interesting warning. It usually points to a serious error. Judging by the parameters' names, we can conclude that the second parameter is a collection of errors and the third one is a collection of warnings. Now it's clear why the analyzer issued a warning. When an object is created in the CloneSdkResult method, sdkResult.Warnings is passed as the second argument, and sdkResult.Errors is passed as the third argument. Most likely, the order of arguments was mixed up here — it is difficult to imagine a situation where a warning and an error would be interchangeable.

Potential null dereference

Issue 6

private BuildRequest CreateLocalBuildRequest(...., Project project, ....)
{
  ....
  BuildRequest buildRequest =  new BuildRequest(....)
  ....
  if (String.IsNullOrEmpty(toolsVersion) && project != null)  // <=
  {
    buildRequest.ToolsetVersion = project.ToolsVersion;
  }

  if (buildRequest.ProjectFileName == null)
  {
    buildRequest.ProjectFileName = project.FullFileName;     // <=
  }

  return buildRequest;
}

PVS-Studio's warning: V3125 The 'project' object was used after it was verified against null. Check lines: 2446, 2439. Engine.cs 2446

The project variable is checked for null in this condition:

if (String.IsNullOrEmpty(toolsVersion) && project != null)

The following condition accesses the project.FullFileName property. But project isn't checked for null there — hence the problem. This is odd: the developer suspects that the variable could be null seven code lines above this one, but doesn't suspect it now.

It's worth noting that the state of the variable cannot change and buildRequest.ProjectFileName is not related to project in any way. Dereferencing a null reference will lead to NullReferenceException.

Issue 7

internal override void WriteToStream(BinaryWriter writer)
{
  base.WriteToStream(writer);
  if (buildItems == null)
  {
    writer.Write((byte)0);
  }
  else
  {
    ....
    foreach (BuildItem item in buildItems)
    {
      if (item == null)
      {
        writer.Write((byte)0);                    // <=
      }
       writer.Write((byte)1);
       item.WriteToStream(writer);                // <=
    }
  }
}

PVS-Studio's warning: V3125 The 'item' object was used after it was verified against null. Check lines: 139, 134. BuildItemCacheEntry.cs 139

In the foreach body, the item variable is checked for null. If item is null, 0 is written to the stream. Then, without any condition, 1 is written to the stream, and then... Then NullReferenceException is thrown. This will happen because of the item's writeToStream call.

Perhaps the else block is missing here. Below is a possible way to correct the error:

if (item == null)
{
  writer.Write((byte)0);
}
else
{
  writer.Write((byte)1);
  item.WriteToStream(writer)
}

Issue 8

public void LogTelemetry(string eventName,
                         IDictionary<string, string> properties)
{
  ....
  foreach (string key in properties?.Keys)                                // <=
  {
    message += $"  Property '{key}' = '{properties[key]}'{Environment.NewLine}";
  }
  ....
}

PVS-Studio's warning: V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Consider inspecting: properties?.Keys. MockEngine.cs 165

In the code above, the foreach block iterates through a collection. To obtain this collection, the foreach statement uses the '?.' operator. The developer could have assumed that if properties is null, the code in the foreach body simply won't execute. Although that is correct, here's a problem — an exception will be thrown.

The GetEnumerator method is called for the iterated collection. It's not hard to guess the outcome of calling this method for a variable that carries the null value.

You can find a more detailed analysis of such issues in this article.

Issue 9

internal static Function<T> ExtractPropertyFunction(
                string expressionFunction,
                IElementLocation elementLocation,
                object propertyValue,
                UsedUninitializedProperties usedUnInitializedProperties,
                IFileSystem fileSystem)
{
  ....
  if (propertyValue == null && expressionRoot[0] == '[')           // <=
  {
    ....
  }
  else if (expressionFunction[0] == '[')
  {
    ....
    functionBuilder.ReceiverType = propertyValue.GetType();        // <=
    ....
  }
  else
  {
    ....
    if (propertyValue == null && !IsValidPropertyName(functionReceiver))
    {
      ProjectErrorUtilities
      .ThrowInvalidProject(elementLocation,
                           "InvalidFunctionPropertyExpression",
                            expressionFunction, String.Empty);
    }
    var receiverType = propertyValue?.GetType() ?? typeof(string); // <=
    ....
  }
  ....
}

The analyzer issued two warnings for this code fragment:

V3125 The 'propertyValue' object was used after it was verified against null. Check lines: 3301, 3253. Expander.cs 3301
V3095 The 'propertyValue' object was used before it was verified against null. Check lines: 3301, 3324. Expander.cs 3301

Actually, both these warnings point out the same problem. Let's look at the condition of the first if. A part of this condition checks propertyValue for null. This implies that the developer expected this value could be null. There may be a case where propertyValue == null is true while the second part of the condition is false. Therefore, the else branch would be executed. In that branch, the null reference would be dereferenced when the propertyValue.GetType method is called. It is also worth noting that further on, before the method is called*, PropertyValue* is checked for null.

Conclusion

In this article we described not only issues, diagnostics for which didn't exist when we first checked MSBuild, but also warnings from relatively old diagnostics.

Obviously, new diagnostics helped find errors that we didn't see during the first check. The old diagnostics use core mechanisms. We constantly enhance these mechanisms to achieve high-quality analysis, that's why old diagnostics issue new warnings.

You may ask a question: "Why did you describe only 9 warnings?" We wanted to show you the most interesting ones without making the article boring.

Last, but not least, we'd like to acclaim the MSBuild developers' hard work — they really care about the project's quality.

We are constantly working hard to keep enhancing PVS-Studio: new diagnostics are being added, old ones are being modified. This allows you to find more code fragments that could be dangerous for your program. You can try PVS-Studio for free and see what it can find in your project.

Blog