Comparing PVS-Studio for C# and a built-in Visual Studio analyzer, using the CruiseControl.NET codebase

• Introduction
• The research and results
  • Visual Studio
  • PVS-Studio
• Conclusion
• Additional links

Recently I have done comparison of C# analyzers by PVS-Studio and SonarQube on the base of PascalABC.NET code. The research turned out to be pretty engaging, so I decided to continue working in this direction. This time I compared a C# analyzer of PVS-Studio with a static analyzer built into Visual Studio. In my opinion, this is a very worthy adversary. Despite the fact that the analyzer from the Visual Studio kit is primarily designed to improve the quality of the code, not to look for bugs, this does not mean that it cannot be used to detect real errors, although this may be not easy. Let's see which peculiarities in the work of the analyzers will be detected in the course of our investigation. Let's start!

Introduction

First, here is a small FAQ section to clarify some points.

Q: Why CruiseControl.NET? What project is this?

A: CruiseControl.NET is an Automated Continuous Integration server, implemented using the .NET Framework. The source code of CruiseControl.NET is available on GitHub. The project has not been supported and developed for some time already, although it was quite popular until recently. This won't hinder the comparison of the analyzers, but on the contrary, will bring some element of stability. We can be sure that no one has improved the code using the latest version of PVS-Studio analyzer or the analyzer built in Visual Studio. An additional advantage is the small size of CruiseControl.NET: the project has approximately 256 thousand code lines.

Q: Have you used Visual Studio 2017? We would like to see the features of the latest versions of the analysis tools.

A: We used Visual Studio 2017 Community for the analysis of both tools.

Q: What about the settings of the analyzers? Perhaps, everything was "set up on purpose" and that's why PVS-Studio turned out to be better?

A: For both analyzers we used the "default" settings. For the testing integrity, we did the research on a "clean" machine with Windows 10.

Q: Well, good. But surely, you have juggled with the results or did the calculations not quite correctly? For example, in PVS-Studio you could ignore the "Low" level of certainty, by taking just "High" and "Medium" levels. Then PVS-Studio will have an advantage over the analyzer built into Visual Studio, since the latter does not have similar settings.

A: In the analysis of the results we took into account absolutely all alert levels and included all of the available types of diagnostics.

Q: What about the selection of files for analysis? Did you add anything to the exceptions, for example, Unit-tests?

A: We did the analysis of the entire solution for both analyzers, without any exceptions. I should also note that CruiseControl.NET does have a project "UnitTests". There were quite a lot of warnings issued for this project, but they were all not taken into account during the search of real errors, although they appear in the total number of issued warnings.

Q: Real errors? What term is that?

A: In our opinion, these are errors, critical for the program performance, that will likely lead to the exception throw, incorrect program behavior or incorrect results. These are errors that should be fixed immediately. These are not just recommendations of the design improvement or minor flaws like code duplication that do not affect the result. Here is an example of a real bug in CruiseControl.NET:

public override string Generate(IIntegrationResult integrationResult)
{
  ....
  IntegrationSummary lastIntegration = 
    integrationResult.LastIntegration;    // <=
  
  if (integrationResult == null || ....)  // <=
  {
    ....
  }
  ....
}

A lot of analyzers will issue a warning for the given fragment that the variable integrationResult is used without the prior check against null. That's right, but it usually leads to a large number of false positives, among which it is very difficult to find a real error. Our approach is to conduct additional analysis, which increases the probability of detecting real errors. In the code fragment given above, we see that after the variable is used, it is verified against null. I.e. in this case the programmer assumes that the value of a variable can be null after passing to the method, and writes the check. This is exactly the situation that we will consider to be erroneous. If the method didn't have the check of integrationResult against null, then we would consider this to be a false positive:

public override string Generate(IIntegrationResult integrationResult)
{
  ....
  IntegrationSummary lastIntegration = 
    integrationResult.LastIntegration;
  ....
}

PVS-Studio will not issue a warning for this code, while a number of analyzers will. As a result, these warnings will be ignored or disabled. A large number of warnings does not mean that they may be useful.

Q: Suppose, you did everything right. But why should I take all this for granted? How can I repeat your investigation?

A: Nothing could be simpler. The analyzer built in Visual Studio is free. You should just install the free version of Visual Studio 2017 Community. To do the analysis of CruiseControl.NET using PVS-Studio, you will have to just load it and use the demo version. Yes, some limitations of the demo version won't allow doing the complete analysis, but you can write to us and we may give a temporary license key to you.

The research and results

Visual Studio

It took just a couple of minutes to check the code of the project with the help of the analyzer, built in Visual Studio. Right after the analysis, we see the following results (no filters are enabled):

The analyzer issued 10773 warnings. Yes, it won't be an easy thing to find errors here. For a start, I will exclude the warnings issued for "Unit tests: with the help of the filter:

Ok. Almost half of the warnings were issued for the tests, which is of no surprise. But more than 5 thousand remaining messages-not too little. These warnings be grouped as follows:

Microsoft.Design: CA10XX (diagnostics:40, warnings: 1637) 
Microsoft.Globalization: CA13XX (diagnostics: 7, warnings: 1406)
Microsoft.Interoperability: CA14XX (diagnostics: 2, warnings: 10)
Microsoft.Maintainability: CA15XX (diagnostics: 3, warnings: 74)
Microsoft.Mobility: CA16XX (diagnostics: 1, warnings: 1)
Microsoft.Naming: CA17XX (diagnostics: 17, warnings: 1071)
Microsoft.Performance: CA18XX (diagnostics: 15, warnings: 489)
Microsoft.Portability: CA19XX (diagnostics: 1, warnings: 4)
Microsoft.Reliability: CA20XX (diagnostics: 4, warnings: 158)
Microsoft.Globalization, Microsoft.Security: CA21XX (diagnostics: 5,
warnings: 48)
Microsoft.Usage: CA22XX (diagnostics: 18, warnings: 440)

Also, there were several compiler warnings issued. Apparently, there was no other choice than to study the description of every analyzer diagnostic and then examine the warnings if necessary.

I should say that I took time to do this and did not manage to find anything that would help to find errors among more than five thousand of warnings issued by the analyzer. In most cases it all boils down to the recommendations about the design improvement and code optimization. As there are too many of these warnings, I am not going to cite here the full list with the descriptions. If you wish, you can carefully examine this list yourself by checking the project using CruiseControl.NET analyzer built into Visual Studio. A detailed description of diagnostics is available on MSDN.

I have examined a substantial part but not all of the warnings. There was no point in reviewing all of the groups till the end, as they were all similar and they were obviously not errors. Not to be unfounded, I'll cite one example for each group.

Microsoft.Design

CA1002 Change 'List<NameValuePair>' in 'CruiseServerClient.ForceBuild(string, List<NameValuePair>)' to use Collection<T>, ReadOnlyCollection<T> or KeyedCollection<K,V> CruiseServerClient.cs 118

public override void ForceBuild(...., List<NameValuePair> parameters)
{
  ....
}

This is a recommendation to use a universal collection (for example, Collection), instead of List for the parameters parameter of the method.

Microsoft.Globalization

CA1300 Change 'AddProjects.RetrieveListOfProjects(BuildServer)' to call the MessageBox.Show overload that specifies MessageBoxOptions, and make sure to set MessageBoxOptions.RightAlign and MessageBoxOptions.RtlReading if RightToLeft is set to RightToLeft.Yes on the parent control. CCTrayLib AddProjects.cs 86

private void RetrieveListOfProjects(....)
{
  ....
  MessageBox.Show(this, "Unable to connect to server " +
server.DisplayName + ": " + ex.Message, "Error");
  ....
}

Here is a recommendation to use an overload of the method MessageBox.Show() that takes an argument MessageBoxOptions. This is necessary to improve the support a multi-language interface and languages that use right-to-left reading order.

Microsoft.Interoperability

CA1401 Change the accessibility of P/Invoke 'NativeMethods.SetForegroundWindow(IntPtr)' so that it is no longer visible from outside its assembly. CCTrayLib NativeMethods.cs 12

[DllImport("user32.dll")]
[return: MarshalAs(UnmanagedType.Bool)]
public static extern bool SetForegroundWindow(IntPtr handle);

Here is a recommendation that you should not specify the public level of access for the methods with the DllImportAttribute attribute.

Microsoft.Maintainability

CA1500 'errorMessages', a variable declared in 'Response.ConcatenateErrors()', has the same name as an instance field on the type. Change the name of one of these items. Remote Response.cs 152

private List<ErrorMessage> errorMessages;
....
public virtual string ConcatenateErrors()
{
  List<string> errorMessages = new List<string>();
  ....
}

This is warning that a local variable has the same name as the class field.

Microsoft.Mobility

CA1601 Modify the call to 'Timer.Timer(double)' in method FileChangedWatcher.FileChangedWatcher(params string[])' to set the timer interval to a value that's greater than or equal to one second. core FileChangedWatcher.cs 33

public FileChangedWatcher(....)
{
  ....
  timer = new Timer(500);
  ....
}

This warning is that the timer interval is set to less than one second.

Microsoft.Naming

CA1702 In member 'Alienbrain.CreateGetProcess(string)', the discrete term 'filename' in parameter name 'filename' should be expressed as a compound word, 'fileName'. core Alienbrain.cs 378

public ProcessInfo CreateGetProcess(string filename)
{
  ....
}

A warning about the need to use Camel Case for naming the compound variable names.

Microsoft.Performance

CA1800 'action', a variable, is cast to type 'AdministerAction' multiple times in method 'AdministerPlugin.NamedActions.get()'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction. WebDashboard AdministerPlugin.cs 79

public INamedAction[] NamedActions
{
  get
  {
    ....
    if (action is AdministerAction)
    {
      (action as AdministerAction).Password = password;
    }
    ....
  }
  ....
}

A warning about the necessity to optimize the iterative type casting.

Microsoft.Portability

CA1901 As it is declared in your code, parameter 'fdwSound' of P/Invoke 'Audio.PlaySound(byte[], short, long)' will be 8 bytes wide on 32-bit platforms. This is not correct, as the actual native declaration of this API indicates it should be 4 bytes wide on 32-bit platforms. Consult the MSDN Platform SDK documentation for help determining what data type should be used instead of 'long'. CCTrayLib Audio.cs 135

[DllImport ("winmm.dll")]
private static extern int PlaySound (byte[] pszSound, Int16 hMod,
long fdwSound);

A warning that a non-portable type is used in the imported method for the parameter fdwSound. It's necessary to use IntPtr or UIntPtr.

Microsoft.Reliability

CA2000 In method 'About.famfamfamLink_LinkClicked(object, LinkLabelLinkClickedEventArgs)', call System.IDisposable.Dispose on object 'urlLink' before all references to it are out of scope. CCTrayLib About.cs 71

private void famfamfamLink_LinkClicked(....)
{
  Process urlLink = new Process();
  urlLink.StartInfo = new ProcessStartInfo(....);
  urlLink.Start();
}

A warning to free an IDisposable-object urlLink before it is out of scope. For example, you can write using.

Microsoft.Globalization, Microsoft.Security

CA2101 To reduce security risk, marshal parameter 'lpszDomain' as Unicode, by setting DllImport.CharSet to CharSet.Unicode, or by explicitly marshaling the parameter as UnmanagedType.LPWStr. If you need to marshal this string as ANSI or system-dependent, specify MarshalAs explicitly, and set BestFitMapping=false; for added security, also set ThrowOnUnmappableChar=true. core Impersonation.cs 100

[DllImport("advapi32.dll", SetLastError = true)]
private static extern bool LogonUser(
        string lpszUsername,
        string lpszDomain,
        string lpszPassword,
        int dwLogonType,
        int dwLogonProvider,
        ref IntPtr phToken);

The warning that the type of marshaling for the string arguments is not specified, for example by defining the attributes as follows:

[DllImport("advapi32.dll", SetLastError = true,
CharSet = CharSet.Unicode)]

Microsoft.Usage

CA2201 'CruiseServerClientFactory.GenerateClient(string, ClientStartUpSettings)' creates an exception of type 'ApplicationException', an exception type that is not sufficiently specific and should never be raised by user code. If this exception instance might be thrown, use a different exception type. Remote CruiseServerClientFactory.cs 97

public CruiseServerClientBase GenerateClient(....)
{
  ....
  throw new ApplicationException("Unknown transport protocol");
  ....
}

The warning about the exception throw of a too general type. In this case it is recommended to throw an exception of a more specific type, not reserved in the execution environment.

Result

Doing this work, I came to the conclusion that in this case it makes sense to do the search of real errors only for the warnings with the code CA1062 from the group Microsoft.Design. This is a warning for situations where a method parameter has a reference type and there is no verification against null before its usage. After applying the filter for such warnings, we get the following:

733 - is still a lot. But it's already something. And if we review the detected code fragments, they are really potentially unsafe. For example, in the file ItemStatus.cs:

public void AddChild(ItemStatus child)
{
  child.parent = this;
  childItems.Add(child);
}

The child reference to the instance of the ItemStatus class is not checked before it is used. Yes, it is dangerous. But, unfortunately, this can not be called a mistake. Perhaps, the checks may be located in the calling code, although it is not correct. Moreover, the method is declared as public. Of course, the author of the code should take measures and to deal with these warnings, but let me remind you that there are 733 of them. Most likely, the programmer will do nothing because "everything works". This is exactly the danger of issuing a lot of warnings for everything that seems more or less suspicious. For this reason, I gave an example of a real error that a developer should pay attention to. The warnings like this can be considered as false positives. It is really so.

Having spent some more time, I found 5 warnings among those 733 that can be interpreted as errors. Here is an example of one of them (BuildGraph.cs file):

public override bool Equals(object obj)
{
  if (obj.GetType() != this.GetType() )
    return false;
  ....
}

The obj variable is not verified against null before the use. Since we are talking about the overloaded Equals method - we are dealing with an error. The method Equals has to correctly process null references. Perhaps, such situations never occur in CruiseControl.NET project, but the code of the method is still erroneous and it should be fixed.

A meticulous reader might argue that I may have missed such an error without having studied every use of the methods. Perhaps, that is possible. But in practice, no one will examine every warning carefully, but the percentage of the false positives is still very large.

I should note that despite the fact that I managed to find errors in the code of CruiseControl.NET using the analyzer, built in Visual Studio, the process itself took about 8 hours (the whole work day) and it required additional attention and concentration. Perhaps, if the project authors used the static analysis regularly, the overall picture would be more positive.

PVS-Studio

The analysis of the project with PVS-Studio on my machine took a minute. Right after that the results are the following (none of the filters are enabled):

The analyzer issued 198 warnings. 45 warnings were issued for the project "UnitTests", and 32 more warnings have a low priority (it's less likely that there are real errors among them). Subtotal - 121 messages for analysis, which took me 30 minutes. As a result, 19 errors have been identified:

Here is an example of one of them:

V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 120, 125. CCTrayLib CCTrayProject.cs 120

public override bool Equals(object obj)
{
  ....
  if ((buildServer != null) && 
      (objToCompare.buildServer != null))
  {
    // If both instances have a build server then compare the build
    // server settings
    isSame = string.Equals(buildServer.Url,
objToCompare.buildServer.Url);
  }
  else if ((buildServer != null) && 
(objToCompare.buildServer != null))
  {
    // If neither instance has a build server then they are the same
    isSame = true;
  }
  ....
}

Both if blocks contain the same condition. We see a serious bug affecting the logic of the program and resulting in an unexpected result.

I think here I have nothing more to add here. PVS-Studio quickly and accurately did its work in finding real errors. This is exactly what it was made for.

Conclusion

Here is a table showing the results:

Of course, we see that PVS-Studio has a bigger advantage. But again, the analyzer built in Visual Studio was made to improve the design and optimize the code, not for the bug search. While PVS-Studio, on the contrary, was "aimed" at the bug search with the lowest possible percentage of false alarms. Besides that, the developers of CruiseControl.NET, apparently didn't use any analyzers at all. I am sure that if they used the analyzer built in Visual Studio, the quality of the code would be much better, and the possibility of an error is lower, not to mention PVS-Studio. Such tools allow achieving maximum effect if used regularly, rather than "once a year".

Download and try PVS-Studio: http://www.viva64.com/en/pvs-studio/

To purchase a commercial license, please contact us via the email. You can also write to us to get a temporary license key for a comprehensive investigation of PVS-Studio, if you want to avoid the limitations of the demo version.

Additional links

• The way static analyzers handle false positives and why they do it.
• Analysis of PascalABC.NET with the help of SonarQube plugins: SonarC# and PVS-Studio .https://www.viva64.com/en/b/0492/