# Checking the Source Code of MSBuild with PVS\-Studio As we continue developing PVS\-Studio static code analyzer, we often have to check large open\-source projects by renowned developers\. The fact that even such projects contain a certain amount of bugs adds even more sense and weight to our work\. Unfortunately, everybody makes mistakes\. No matter how carefully you control your code's quality, there is just no way to avoid "human error"\. As long as software is developed by humans, analysis tools like PVS\-Studio will remain relevant and needed\. Today, we are going to discuss errors found in the source code of MSBuild, which is, unfortunately, not perfect either\. ![0424_MSBuild/image1.png](https://import.viva64.com/docx/blog/0424_MSBuild/image1.png) ## Introduction Microsoft Build Engine \([MSBuild](https://msdn.microsoft.com/en-us/library/dd393574.aspx)\) is a platform by Microsoft for building applications\. It is usually used together with Microsoft Visual Studio but does not depend on it\. MSBuild provides an XML schema for project files \(\*\.csproj, \*\.vbproj, \*\.vcxproj\) that controls how the build platform processes and builds software\. The tool is shipped as part of the \.NET platform and is written in C\#\. Microsoft provides MSBuild source files for free upload to [GitHub](https://github.com/dotnet/msbuild)\. Taking into account the high development standards applied by Microsoft, finding bugs in MSBuild might be a hard task even for top\-quality static analyzers\. But success comes with tenacity\. With the help of PVS\-Studio, version 6\.07, we have checked the source code of MSBuild project, and here's what we have found\. ## Analysis data and statistics MSBuild consists of 14 projects, which include a total of 1256 source files in C\#\. That makes approximately 440,000 LOC\. PVS\-Studio issued 262 warnings for this project\. The general analysis statistics with differentiation of warnings across severity levels are shown in the following chart: ![0424_MSBuild/image2.png](https://import.viva64.com/docx/blog/0424_MSBuild/image2.png) As you can see from the chart, the tool issued 73 high\-level warnings, 107 medium\-level warnings, and 82 low\-level warnings\. We will focus on the High and Medium levels, as they contain potentially dangerous constructs and genuine bugs, while Low\-level warnings, though dealing with errors as well, often turn out to be false positives, so we don't usually discuss them in our articles\. The analysis of MSBuild has revealed that about 45% of the High\- and Medium\-level warnings point to incorrect code \(81 errors\), while the rest warnings simply refer to constructs that PVS\-Studio finds suspicious, and false positives rather than real errors\. Some of the warnings were triggered by unit tests or code with comments about potentially dangerous constructs used to check for exceptions\. In any case, the remaining warnings need to be examined by the developers as they are the only people who have the full knowledge of the code and can reliably estimate if the warnings are correct or not\. Based on these data, the PVS\-Studio ratio of High\- and Medium\-level errors to 1 KLOC \(i\.e\. error density\) is only 0\.184 \(errors per 1 KLOC\)\. This figure is not something to be surprised at in case of Microsoft projects and is a sign of the high quality of MSBuild's code\. Now let's discuss the analysis results in detail\. Note also that the job of examining all the warnings issued for this project is beyond the scope of our article, so we'll only talk about the most interesting defects, classifying them into groups\. ## Analysis results **Incorrect null check** **PVS\-Studio diagnostic message**: [V3019](https://pvs-studio.com/en/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'obj', 'name'\. AssemblyRemapping\.cs 64 This is probably a classical error: we see it in nearly every project we check\. It has to do with casting a variable to a different type using the _as _operator and testing the same variable, instead of the resulting one, for _null_: ```cpp AssemblyNameExtension name = obj as AssemblyNameExtension; if (obj == null) // <= { return false; } ``` Instead, it is the _name_ variable that should be checked: ```cpp AssemblyNameExtension name = obj as AssemblyNameExtension; if (name == null) { return false; } ``` **Late null check** **PVS\-Studio diagnostic message**: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'diskRoots' object was used before it was verified against null\. Check lines: 2656, 2659\. ToolLocationHelper\.cs 2656 Note the _diskRoots_ parameter\. It is an instance of the _List_ class and can have a value of _null_\. However, the corresponding check is done only in the second _if _block, after the _diskRoots_ variable has already been used for inserting values into a list: ```cpp private static void ExtractSdkDiskRootsFromEnvironment (List diskRoots, string directoryRoots) { if (!String.IsNullOrEmpty(directoryRoots)) { .... diskRoots.AddRange(splitRoots); // <= } if (diskRoots != null) // <= .... } ``` There are 8 more potentially dangerous constructs like that in MSBuild: * V3095 The 'propertyValue' object was used before it was verified against null\. Check lines: 2760, 2799\. Expander\.cs 2760 * V3095 The 'publicKeyToken' object was used before it was verified against null\. Check lines: 232, 236\. GenerateBindingRedirects\.cs 232 * V3095 The 'searchLocation' object was used before it was verified against null\. Check lines: 170, 178\. Resolver\.cs 170 * V3095 The 'assemblyName' object was used before it was verified against null\. Check lines: 176, 194\. Resolver\.cs 176 * V3095 The 'searchLocation' object was used before it was verified against null\. Check lines: 249, 264\. Resolver\.cs 249 * V3095 The 'ReferenceInfo' object was used before it was verified against null\. Check lines: 87, 97\. AxReference\.cs 87 * V3095 The 'packageFileName' object was used before it was verified against null\. Check lines: 1448, 1457\. BootstrapperBuilder\.cs 1448 * V3095 The 'metadataNames' object was used before it was verified against null\. Check lines: 243, 253\. CommandLineBuilderExtension\.cs 243 **Wrong assumption about string length** **PVS\-Studio diagnostic message**: [V3057](https://pvs-studio.com/en/docs/warnings/v3057/) The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. Utilities\.cs 328 For the _if_ block to execute, there must be a string consisting of one or more characters, while inside that block, the programmer attempts to get a substring from the original string\. Obviously, the second parameter of the _Substring _method will be negative for a one\-character string, so the method will throw an _ArgumentOutOfRangeException_: ```cpp if (toolsVersionList.Length > 0) { toolsVersionList = toolsVersionList.Substring(0, toolsVersionList.Length - 2); } ``` This is what a safe version of this code could look like: ```cpp if (toolsVersionList.Length > 1) { toolsVersionList = toolsVersionList.Substring(0, toolsVersionList.Length - 2); } ``` Other similar errors: * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. SolutionFile\.cs 1217 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. XMake\.cs 2929 * V3057 The 'Remove' function could receive the '\-1' value while non\-negative value is expected\. Inspect the first argument\. BootstrapperBuilder\.cs 767 **Type conversion with the loss of significance** **PVS\-Studio diagnostic message**: [V3041](https://pvs-studio.com/en/docs/warnings/v3041/) The expression was implicitly cast from 'long' type to 'float' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. CommunicationsUtilities\.cs 593 The variables _now_ and _s\_lastLoggedTicks_ are of _long_ type\. They take part in some computations that should yield a value of _float_ type\. However, since the division operation is done over values of type _long_ and only then is the resulting value cast to type _float_, it will result in the loss of precision: ```cpp float millisecondsSinceLastLog = (float)((now - s_lastLoggedTicks)/10000L); ``` Fixed code: ```cpp float millisecondsSinceLastLog = (float)(now - s_lastLoggedTicks)/10000; ``` Always be careful with computations where integer and floating\-point values are used together\. **Method that always returns _true_** **PVS\-Studio diagnostic message**: [V3009](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. ComReference\.cs 304 The _GetTypeLibNameForITypeLib _method returns _true_ no matter what the conditions are: ```cpp internal static bool GetTypeLibNameForITypeLib(....) { .... if (typeLib2 == null) { .... return true; // <= } .... try { if (data == null || ...) { .... return true; // <= } .... } catch (COMException ex) { .... return true; // <= } return true; // <= } ``` At the same time, the value of type _bool _returned by the _GetTypeLibNameForITypeLib_ method is checked in the caller\. Effects of such behavior are unpredictable\. This code needs to be refactored and fixed\. **Meaningless comparison** **PVS\-Studio diagnostic message**: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'itemsAndMetadataFound\.Metadata\.Values\.Count \> 0' is always true\. Evaluator\.cs 1752 After the _itemsAndMetadataFound\.Metadata\.Values\.Count \> 0_ expression is evaluated in the external _if _block, the same check, this time pointless, is done _inside_ that block: ```cpp if (itemsAndMetadataFound.Metadata != null && itemsAndMetadataFound.Metadata.Values.Count > 0) { .... if (itemsAndMetadataFound.Metadata.Values.Count > 0) // <= { needToProcessItemsIndividually = true; } .... } ``` In addition, MSBuild contains 7 more issues of this kind: * V3022 Expression 'fixedPathInfo \!\= null' is always true\. FrameworkLocationHelper\.cs 794 * V3022 Expression '\_shutdownException \!\= null' is always false\. InProcNode\.cs 527 * V3022 Expression 'proj \!\= null' is always true\. SolutionFile\.cs 817 * V3022 Expression '\_directMetadata \=\= null' is always false\. ProjectItem\.cs 755 * V3022 Expression 'Constants\.defaultToolsVersion \=\= "2\.0"' is always true\. ToolsetReader\.cs 194 * V3022 Expression '\!isQuotedTransform && functionCapture \=\= null' is always true\. ExpressionShredder\.cs 281 * V3022 Expression '\!isQuotedTransform && functionCapture \=\= null' is always true\. ExpressionShredder\.cs 414 **Mutually exclusive comparisons** **PVS\-Studio diagnostic message**: [V3011](https://pvs-studio.com/en/docs/warnings/v3011/) Two opposite conditions were encountered\. The second condition is always false\. Check lines: 2840, 2838\. XMake\.cs 2840 For the _if_ block to execute, the _logger _variable must have the _null_ value\. However, this variable is again tested for _null _inside that block, in the _VerifyThrow_ method\. So, that second check will always be false: ```cpp if (logger == null) { InitializationException.VerifyThrow(logger != null, // <= "LoggerNotFoundError", unquotedParameter); } ``` I'm not sure what this code should actually look like, but certainly not like that\. Perhaps the _if_ operator is not necessary at all\. **Unused arguments in string formatting methods** **PVS\-Studio diagnostic message**: [V3025](https://pvs-studio.com/en/docs/warnings/v3025/) Incorrect format\. A different number of format items is expected while calling 'WriteLine' function\. Arguments not used: 1st\. Scheduler\.cs 2216 The error is lurking in the second line\. The programmer must have written it by copying the first line \(the infamous copy\-paste\) and changing the first parameter in the copied code, but they forgot to remove the second parameter, _\_schedulingData\.EventTime\.Ticks_, which was no more necessary: ```cpp file.WriteLine("Scheduler state at timestamp {0}:", _schedulingData.EventTime.Ticks); file.WriteLine("------------------------------------------------", _schedulingData.EventTime.Ticks); // <= ``` So, the method _WriteLine\(string format, object arg0\)_ is overridden incorrectly in the second line\. Other similar defects: * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: resource\. XmlUtil\.cs 75 * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: resource\. XmlUtil\.cs 82 * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: resource\. XmlUtil\.cs 91 * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: resource\. XmlUtil\.cs 112 **Unused parameter** **PVS\-Studio diagnostic message**: [V3061](https://pvs-studio.com/en/docs/warnings/v3061/) Parameter 'numericValue' is always rewritten in method body before being used\. NodePacketTranslator\.cs 320 The list of the method's formal parameters includes variable _numericValue_ whose value is never used as it is immediately replaced with a new value: ```cpp public void TranslateEnum(ref T value, int numericValue) { numericValue = _reader.ReadInt32(); // <= Type enumType = value.GetType(); value = (T)Enum.ToObject(enumType, numericValue); } ``` Perhaps the programmer did some refactoring, but changing the method's signature \(unlike its body\) was not possible\. Otherwise, it is better to fix the method: ```cpp public void TranslateEnum(ref T value) { int numericValue = _reader.ReadInt32(); Type enumType = value.GetType(); value = (T)Enum.ToObject(enumType, numericValue); } ``` Another similar warning: * V3061 Parameter 'defaultToolsVersion' is always rewritten in method body before being used\. ToolsetProvider\.cs 118 **Redundant assignment** **PVS\-Studio diagnostic message**: [V3005](https://pvs-studio.com/en/docs/warnings/v3005/) The '\_nextProjectId' variable is assigned to itself\. LoggingService\.cs 325 The analyzer detected a construct with an extra assignment to field _\_nextProjectId_\. The result of the _MaxCPUCount_ _\+ 2_ expression is added to the value of _\_nextProjectId_, and then the resulting value is assigned to the same field using the _\+\=_ operator\. After that, this value is again assigned to the _\_nextProjectId_ field: ```cpp public int NextProjectId { get { lock (_lockObject) { _nextProjectId = _nextProjectId += MaxCPUCount + 2; // <= return _nextProjectId; } } } ``` There is no error in this code, but it does look odd\. The construct should be simplified: ```cpp public int NextProjectId { get { lock (_lockObject) { _nextProjectId += MaxCPUCount + 2; return _nextProjectId; } } } ``` ## Conclusion As a conclusion, I'd like to say that even such high\-quality projects as MSBuild could benefit quite a lot from regular checks of their source code for potential and actual errors by static analyzers like PVS\-Studio\. Feel free to use the demo version of [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) analyzer to check this project and take a look at the warnings we have discussed, as well as to check your own projects\.