Build to order? Checking MSBuild for the second time
Tags: csharp, dotnet, opensource, programming
Published: 2022-09-07 11:22:04

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

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/sqcp90yaw2v314j0zfz2.png)

## Introduction

Since the [previous check](https://pvs-studio.com/en/blog/posts/csharp/0424/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild), 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:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/glvrm0fy0g5sg1v2miow.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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild)\.

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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) Expression 'leftConflictReference\.IsPrimary && rightConflictReference\.IsPrimary' is always false\. ReferenceTable\.cs 2388 
* [V3063](https://pvs-studio.com/en/docs/warnings/v3163/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) A part of conditional expression is always true if it is evaluated: \!isNonUnified\. ReferenceTable\.cs 2389
* [V3063](https://pvs-studio.com/en/docs/warnings/v3163/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild)\.

**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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) 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/?utm_source=dev_to&utm_medium=articles&utm_content=MSBuild) for free and see what it can find in your project\.