# 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\!

![0986_MSBuild/image1.png](https://import.viva64.com/docx/blog/0986_MSBuild/image1.png)

## Introduction

Since the [previous check](https://pvs-studio.com/en/blog/posts/csharp/0424/), 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](https://github.com/dotnet/msbuild/tree/c1b61ebeaa0f77d1382c6a0734024557ecf255c3)\. We took the code from this [commit](https://github.com/dotnet/msbuild/tree/c1b61ebeaa0f77d1382c6a0734024557ecf255c3)\.

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

![0986_MSBuild/image2.png](https://import.viva64.com/docx/blog/0986_MSBuild/image2.png)

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](https://pvs-studio.com/en/docs/manual/0010/)\.

Let's look at the most interesting warnings\.

## Wrong increment

**Issue 1**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

```cpp
parsePoint = ScanForPropertyExpressionEnd(expression, ++parsePoint);
```

## Suspicious logical expressions

**Issue 2**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'leftConflictReference\.IsPrimary && rightConflictReference\.IsPrimary' is always false\. ReferenceTable\.cs 2388 
* [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: \!isNonUnified\. ReferenceTable\.cs 2389
* [V3063](https://pvs-studio.com/en/docs/warnings/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:

```cpp
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:

```cpp
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](https://pvs-studio.com/en/blog/posts/cpp/0968/) about that, I highly recommend you to read it\.

**Issue 3**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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?

```cpp
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:

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

PVS\-Studio's warning: [V3008](https://pvs-studio.com/en/docs/warnings/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**

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

PVS\-Studio's warning: [V3066](https://pvs-studio.com/en/docs/warnings/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:

```cpp
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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

```cpp
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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

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

**Issue 8**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0832/)\.

**Issue 9**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3125/) The 'propertyValue' object was used after it was verified against null\. Check lines: 3301, 3253\. Expander\.cs 3301
* [V3095](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/pvs-studio/try-free/) for free and see what it can find in your project\.