Geekly Articles each Day
📜 ⬆️ ⬇️

Checking MSBuild source code with PVS-Studio

While working on the development of the PVS-Studio static source code analyzer, we often encounter the need to check for errors from large open source projects from well-known developers. The fact that even in such projects it is possible to find mistakes makes our work much more meaningful. Unfortunately, everyone makes mistakes. No matter how well the quality control system of the released program code is built, there is absolutely no way to avoid the peculiarities of the “human factor”. As long as people develop software, the relevance of using tools for finding errors in the code, such as PVS-Studio, will not decrease. Today we will look for errors in the MSBuild source code, which, alas, is also not perfect.

Introduction


Microsoft Build Engine ( MSBuild ) is a platform for automating the build of applications from Microsoft. MSBuild is usually used with Microsoft Visual Studio, but does not depend on it. MSBuild provides for the project file (* .csproj, * .vbproj, * .vcxproj) an XML schema that controls how the application platform processes and assembles applications. MSBuild is part of the .NET platform and is developed in the C # programming language.

Microsoft provides MSBuild source code for free download on the GitHub resource. Given the high quality standards of application development adopted by Microsoft, the task of finding errors in MSBuild source code is not easy even for a high-quality static analyzer. But the road will be mastered by walking. Let's check the source code of MSBuild with PVS-Studio version 6.07.

Baseline and general verification statistics


The MSBuild solution consists of 14 projects, which, in turn, contain a total of 1,256 files with source code in the C # programming language. The approximate number of lines of code is 440,000.
')
After checking MSBuild with the static analyzer PVS-Studio, 262 warnings were received. The general statistics of testing with the distinction of the importance of received warnings is:


The diagram shows that 73 high level warnings, 107 medium warnings and 82 low warnings were issued. The main emphasis should be placed on the study of messages with levels of High and Medium. It contains potentially hazardous structures and real errors. Low level alerts also indicate errors, but they have a high percentage of false positives, and we usually do not study them when writing articles.

The analysis of the received warnings revealed that the High and Medium levels contain about 45% of erroneous constructions (81 errors). The remaining warnings are not errors, but are simply constructions that are suspicious from the point of view of PVS-Studio and false positives. Some warnings were received for Unit tests or in those parts of the code where there are comments explaining that the design is deliberately unsafe and is used to check for an exception is thrown. Nevertheless, the remaining warnings of the analyzer require close attention of the developers, since only the authors really know their code and are able to give an adequate assessment of the correctness of a warning.

With this in mind, the PVS-Studio error detection rate at the High and Medium levels per thousand lines of code (error density) is only 0.184 (errors / 1 KLoc). This is not surprising for a product being developed by Microsoft, and indicates the high quality of the MSBuild code.

We describe the results in more detail. In this case, we note that the analysis of all warnings issued by the analyzer is beyond the scope of this article. We confine ourselves to the description of the most interesting messages, grouping cases of the same type.

Test results


Error check for null equality

PVS-Studio analyzer warning: V3019 Possibly an incorrect variable compared to null after type conversion using 'as' keyword. Check variables 'obj', 'name'. AssemblyRemapping.cs 64

Perhaps already a classic error, which occurs in almost every project we test. After casting a type using the as operator, the wrong variable is checked for null equality:

AssemblyNameExtension name = obj as AssemblyNameExtension; if (obj == null) // <= { return false; }

In this case, it is necessary to check the name variable for equality. The correct code is:

 AssemblyNameExtension name = obj as AssemblyNameExtension; if (name == null) { return false; }

Late equality test for null

PVS-Studio analyzer warning: V3095 The 'diskRoots' object was used against null. Check lines: 2656, 2659. ToolLocationHelper.cs 2656

Notice the diskRoots parameter. This is an object of the List class , and its value can be null . However, this fact is checked only in the second if block after the variable diskRoots has been used to insert values ​​into the list:

 private static void ExtractSdkDiskRootsFromEnvironment (List<string> diskRoots, string directoryRoots) { if (!String.IsNullOrEmpty(directoryRoots)) { .... diskRoots.AddRange(splitRoots); // <= } if (diskRoots != null) // <= .... }

In MSBuild code, another 8 similar potentially unsafe constructions were found:


Wrong assumption about the length of the string

PVS-Studio analyzer warning: V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. Utilities.cs 328

The condition for entering an if block is a string consisting of one or more characters. In this case, an attempt is made inside the block to get the substring of the source string. Obviously, in a string consisting of one character, the second parameter of the Substring method will be negative, and the method will throw an exception

ArgumentOutOfRangeException :

 if (toolsVersionList.Length > 0) { toolsVersionList = toolsVersionList.Substring(0, toolsVersionList.Length - 2); }

A safe version of this code snippet would look like this:

 if (toolsVersionList.Length > 1) { toolsVersionList = toolsVersionList.Substring(0, toolsVersionList.Length - 2); }

Similar errors in the code:


Type reduction with loss of accuracy

PVS-Studio analyzer warning: V3041 The expression was implicitly cast from 'long' type to 'float' type. Consider using a fractional part. An example: double A = (double) (X) / Y ;. CommunicationsUtilities.cs 593

The variables now and s_lastLoggedTicks are of type long . Calculations are made, the result of which must be a value of type float . However, since the division operation is performed on variables of the long type, and only after this the result of the expression is reduced to the type of float , there will be a loss of precision:

 float millisecondsSinceLastLog = (float)((now – s_lastLoggedTicks)/10000L);

The correct version of this design:

 float millisecondsSinceLastLog = (float)(now – s_lastLoggedTicks)/10000;

You should always be careful about computations that use integer and floating point values ​​together.

Method that always returns true

PVS-Studio analyzer warning: V3009 It's one of those true values. ComReference.cs 304

The GetTypeLibNameForITypeLib method returns true under any conditions:

 internal static bool GetTypeLibNameForITypeLib(....) { .... if (typeLib2 == null) { .... return true; // <= } .... try { if (data == null || ...) { .... return true; // <= } .... } catch (COMException ex) { .... return true; // <= } return true; // <= }

In this case, the value of type bool returned by the GetTypeLibNameForITypeLib method is checked in the calling code. This behavior can lead to unpredictable consequences. You need to refactor the code and fix the error.

Meaningless comparison

PVS-Studio analyzer warning: V3022 Expression 'itemsAndMetadataFound.Metadata.Values.Count> 0' is always true. Evaluator.cs 1752

After itemsAndMetadataFound.Metadata.Values.Count> 0 is checked in the if external block, the same check, already meaningless, is performed inside the block:

 if (itemsAndMetadataFound.Metadata != null && itemsAndMetadataFound.Metadata.Values.Count > 0) { .... if (itemsAndMetadataFound.Metadata.Values.Count > 0) // <= { needToProcessItemsIndividually = true; } .... }

In addition to this, 7 more similar errors were detected in the MSBuild code:


Mutually exclusive comparisons

PVS-Studio analyzer warning: V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 2840, 2838. XMake.cs 2840

The condition for entering the if block is that the variable logger is null . However, already inside the block, the VerifyThrow method uses a check for the null inequality of the same variable. Thus, the verification performed for the VerifyThrow method will always be false:

 if (logger == null) { InitializationException.VerifyThrow(logger != null, // <= "LoggerNotFoundError", unquotedParameter); }

It is difficult to say how the correct code should look, but definitely not. Perhaps the use of the if statement in this case is not required at all.

Unused arguments when formatting a string

PVS-Studio analyzer warning: V3025 Incorrect format. A different number of items is expected while calling the 'WriteLine' function. Arguments not used: 1st. Scheduler.cs 2216

The error is contained in the second line of code. Apparently, it was obtained by copying the first line (the notorious copy-paste) and replacing the first parameter in it. At the same time, the second parameter which became unnecessary _schedulingData.EventTime.Ticks , was forgotten to be deleted:

 file.WriteLine("Scheduler state at timestamp {0}:", _schedulingData.EventTime.Ticks); file.WriteLine("------------------------------------------------", _schedulingData.EventTime.Ticks); // <=

Thus, in the second line of the code, the overload of the WriteLine method (string format, object arg0) is mistakenly used instead of the correct one.

Similar errors found:


Unused parameter

PVS-Studio analyzer warning: V3061 Parameter 'numericValue' is always rewritten. NodePacketTranslator.cs 320

The list of formal parameters of the method contains the variable numericValue , the value of which is never used, since it is immediately replaced by a new value:

 public void TranslateEnum<T>(ref T value, int numericValue) { numericValue = _reader.ReadInt32(); // <= Type enumType = value.GetType(); value = (T)Enum.ToObject(enumType, numericValue); }

The code may have been refactored, but the method signature (unlike its body) could not be changed. Otherwise, it makes sense to adjust this method:

 public void TranslateEnum<T>(ref T value) { int numericValue = _reader.ReadInt32(); Type enumType = value.GetType(); value = (T)Enum.ToObject(enumType, numericValue); }

Another similar warning:
Extra assignment

PVS-Studio analyzer warning: V3005 The '_nextProjectId' variable is assigned to itself. LoggingService.cs 325

The analyzer detected a construction in which an extra assignment is made for the _nextProjectId field. First, the MaxCPUCount + 2 value is calculated, which is added to the _nextProjectId value and assigned to it by the + = operator. And then the resulting value is once again assigned to the _nextProjectId field:

 public int NextProjectId { get { lock (_lockObject) { _nextProjectId = _nextProjectId += MaxCPUCount + 2; // <= return _nextProjectId; } } }

In this case, there is no error, but the code looks strange. It makes sense to simplify this construction:

 public int NextProjectId { get { lock (_lockObject) { _nextProjectId += MaxCPUCount + 2; return _nextProjectId; } } }

Conclusion


In conclusion, I would like to note how useful regular use of static code analyzers, such as PVS-Studio, can be for searching for potential and real errors, even in such quality projects as MSBuild.

You can always repeat the error search examples given in this article, as well as check your own projects with the help of the demo version of the PVS-Studio analyzer.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Checking the Source Code of MSBuild with PVS-Studio .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/308520/

More articles:


All Articles