To let people know about PVS-Studio, which is now able to check not only C++ projects, but C# as well, we decided to check the source code of WPF examples, offered by Microsoft.
Upon Windows Vista release, the company introduced a new subsystem for rendering user interfaces in Windows-based applications - Windows Presentation Foundation (WPF). This graphic subsystem is a part of the .NET Framework, starting with version 3.0. It uses XAML markup language. Now, it has almost replaced the outdated WinForms. In my humble opinion, the main disadvantage of WinForms, was the fact that it was doing all the rendering on the CPU. WPF approached this in a more sensible way, and let DirectX do the rendering of the components. Now WPF allows the making of universal interfaces for three platforms at once (PC, XBOXOne, Winphone), and has practically ousted WinForms.
To do the analysis of WPF examples from Microsoft (the source code of the examples), we used PVS-Studio static code analyzer, version 6.05.
An interesting thing about this solution, is the fact that along with the projects written in C#, there are also several C++ projects. But I found it only from the list of the bugs found by PVS-Studio. PVS-Studio plugin for Visual Studio, without any additional settings from the user's side, performed the analysis and displayed warnings for both C++ and C# projects.
Figure 1. As you can see, in the PVS-Studio window there are warnings issued for both C# and C++ code (click on the image to enlarge).
1. Errors made during the forming the conditions of the if statement
For programmers it's a common problem - errors in the comparisons. Let's have a look at them.
In this code there are two absolutely identical conditions:
public int Compare(GlyphRun a, GlyphRun b)
{
....
if (aPoint.Y > bPoint.Y) // <=
{
return -1;
}
else if (aPoint.Y > bPoint.Y) // <=
{
result = 1;
}
else if (aPoint.X < bPoint.X)
{
result = -1;
}
else if (aPoint.X > bPoint.X)
{
result = 1;
}
....
}V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 418, 422. txtserializerwriter.cs 418
It's not really clear what was meant here, but apparently, it was something different from what we see now.
We like to do the verifications against null in the conditions, and thus try to protect the program from emergency events. We can even say that the majority of if conditions are the null-checks of some fields or variables. But sometimes such checks can be redundant and even contain logical errors:
public static string FindNumeric(string content)
{
string[] values = content.Split(' ');
if (values != null)
{
return values[0];
}
return "none";
}V3022 Expression 'values != null' is always true. Util.cs 287
We could assume that the author wanted to check that values has more than 0 elements, I personally couldn't think of a situation where Split returns an empty array. Anyway, the verification against null is completely unnecessary here.
As I have already said, the project contains code from C++ and C# diagnostics. I got the impression that the following code was written by a C++ programmer.
private void LoadNotes()
{
var fs = new FileStream("NotesFile", FileMode.Open);
if (fs != null)
{
....
}V3022 Expression 'fs != null' is always true. MainWindow.cs 66
Actually, even in C++ this variant is erroneous, in C# it will at least look "weird". More details of why it is incorrect to write such code are given in the article "Checking 7-Zip with PVS-Studio analyzer" and we'll continue looking at C# code.
We don't have to go far to find more buggy fragments. There were two practically identical functions in the solution (thanks to copy-paste) with the same error:
private void SerializeObjectTree(object objectTree)
{
TextWriter writer = new StreamWriter(_stream);
try
{
string fileContent =
XamlRtfConverter.ConvertXamlToRtf(
XamlWriter.Save(objectTree));
writer.Write(fileContent);
}
finally
{
if (writer != null)
writer.Close();
}
}V3022 Expression 'writer != null' is always true. htmlserializerwriter.cs 324
Writer won't be a null reference...
Throwing an error in exceptional situations is not the worst decision. But the main thing is not to make an error in the condition when the exception should be thrown, because it can create an unpleasant impression in the eyes of our user, when the program crashes all of a sudden.
protected Size SizeParser(string o)
{
....
if (sizeString.Length == 0 || sizeString.Length != 2)
throw new ApplicationException("Error: a size should
contain two double that seperated by a space
or ',' or ';'");
....
}V3023 Consider inspecting the 'sizeString.Length == 0 || sizeString.Length != 2' expression. The expression is excessive or contains a misprint. MainWindow.cs 140
Judging by the text of the error, the comparison with 0 is excessive, it was enough to check if sizeString.Length is not equal to 2.
In the long bodies of if instruction sometimes it's very hard to notice meaningless checks while doing code review.
private static void WriteElement(....)
{
if (htmlWriter == null)
{
....
}
else
{
if (htmlWriter != null && htmlElementName != null)
{
....
....
}V3063 A part of conditional expression is always true: htmlWriter != null. HtmlFromXamlConverter.cs 491
It's no problem for the analyzer. By the way, thanks to our beloved copy-paste, an error was found in two projects: HtmlToXamlDemo and DocumentSerialization.
Of course meaningless checks can be found not only in long functions, but within several strings.
private void OnFlipPicTimeline(object sender, EventArgs e)
{
var clock = (Clock) sender;
if (clock.CurrentState == ClockState.Active) // Begun case
{
return;
}
if (clock.CurrentState != ClockState.Active) // Ended case
{
....
}
}V3022 Expression 'clock.CurrentState != ClockState.Active' is always true. MainWindow.cs 103
In general, it's quite fine, but when later we have an if statement nested in another if statement, and another... If only we could get rid of meaningless checks for better understanding of the code, which is read more often than it is written...
Let's take a short break and have a look at one function that I have recently come across. This is the body of the function:
private void UpdateSavings()
{
Savings = TotalIncome - (Rent + Misc + Food);
if (Savings < 0)
{
}
else if (Savings >= 0)
{
}
}V3022 Expression 'Savings >= 0' is always true. NetIncome.cs 98
Also we have found a lot (more than 60) comparisons of real numbers (double) with a precise 0.
if (leftConst != null && leftConst.Value == 0)
{
// 0 + y; return y;
return newRight;
}For example:
All the lines won't fit in one article. This warning is third level for us, because, its relevance strongly depends on the specifics of the program. In case there are mathematical evaluations (manipulations with the value), there is no guarantee that we will get a specific number: -1, 0, 1. But even slight deviation in 0.00000000001 will lead to incorrect result in comparisons. But if the program logic presupposes writing discrete values to the real numbers (double), then these checks aren't a mistake.
2. Errors in the initialization and assigning of variables
Functions are great things that help not only to remove duplicate code, but simplify the readability of the code where this function is used. It is especially important that this function will do exactly the task that is stated in its name, and the signature of the call. But this is not always the case, for example, consider the following code fragment. I'll write the whole function so you can understand the situation clearer.
public bool OpenDocument(string fileName)
{
Microsoft.Win32.OpenFileDialog dialog;
// If there is a document currently open, close it.
if (this.Document != null) CloseFile();
dialog = new Microsoft.Win32.OpenFileDialog();
dialog.CheckFileExists = true;
dialog.InitialDirectory = GetContentFolder();
dialog.Filter = this.OpenFileFilter;
bool result = (bool)dialog.ShowDialog(null);
if (result == false) return false;
fileName = dialog.FileName; // <=
return OpenFile(fileName);
}V3061 Parameter 'fileName' is always rewritten in method body before being used. ThumbViewer.xaml.cs 192
The name of the file that should be opened, is lost right before its first use fileName = dialog.FileName. Yes, a dialog window will be opened and the user file will be chosen, but why do we need a parameter that isn't really used?
Lack of time and copy-paste sometimes produces very strange constructions:
public MailSettingsDialog()
{
....
_timerClock = _timerClock = new DispatcherTimer();
....
}V3005 The '_timerClock' variable is assigned to itself. MailSettingsDialog.cs 56
This may not seem the most horrible typo, but it makes us think, "are we writing to the correct place for the second time?" Well, for example, like this:
private void DumpAllClipboardContentsInternal()
{
....
if (dataObject == null)
{
clipboardInfo.Text =
clipboardInfo.Text =
"Can't access clipboard now!
\n\nPlease click Dump All Clipboard
Contents button again.";
}
else
{
....
}V3005 The 'clipboardInfo.Text' variable is assigned to itself. MainWindow.cs 204
In general, the code abounds in strange assignments:
private void DoParse(string commandLine)
{
....
strLeft = strRight = string.Empty;
strLeft = strs[0];
strRight = strs[1];
....
}V3008 The 'strLeft' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 55, 54. CommandLine.cs 55
V3008 The 'strRight' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 56, 54. CommandLine.cs 56
strLeft and strRight - are just local variables of string type.
The following code is even more incorrect. For some reason the programmer did a lot of evaluations and reevaluations and then wrote it into the same variable.
private object InvokMethod(....)
{
arg = commandLine.Substring(
commandLine.IndexOf("(", StringComparison.Ordinal) + 1,
commandLine.IndexOf(")",
StringComparison.Ordinal) -
(commandLine.IndexOf("(",
StringComparison.Ordinal) + 1));
arg = commandLine.Substring(
commandLine.IndexOf("(",
StringComparison.Ordinal) + 1);
}V3008 The 'arg' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 176, 173. CommandLine.cs 176
And some more examples of meaningless primary assignments:
private void DrawFormattedText(DpiScale dpiInfo)
{
....
Geometry geometry = new PathGeometry();
geometry = formattedText.BuildGeometry(
new System.Windows.Point(0, 0));
....
}There is no point in writing each example, more interesting bugs are waiting ahead.
3. A couple of miscellaneous errors
Throwing the exception, it's important to save the stack call, so that we can later understand when looking at the logs, 'what exactly went wrong on the user's side', But not everybody knows how to do that.
public static object InvokePropertyOrMethod(....)
{
try
{
....
}
catch (MissingMethodException e)
{
....
throw e;
}
catch (AmbiguousMatchException e)
{
throw e;
}
return resultObject;
}V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost. ReflectionUtils.cs 797
V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost. ReflectionUtils.cs 806
According to the standard, if we pass the exception above in the function call stack by means of throw e;, we'll lose the call stack that was before the catch of the exception in the catch block. To keep the whole stack call, and its further continuation, we just need to write one throw word in the catch block and that's it.
Sometimes the checks are unnecessary, and sometimes they aren't enough as in the following code:
private static void ParseCssFontFamily(....)
{
....
if (fontFamilyList == null && fontFamily.Length > 0)
{
if (fontFamily[0] == '"' || fontFamily[0] == '\'')
{
// Unquote the font family name
fontFamily =
fontFamily.Substring(1, fontFamily.Length - 2);
....
}V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. HtmlCSSParser.cs 645
There is no check that fontFamily.Length is bigger than 1, thus, subtracting from fontFamily.Length number 2 we can get a value less than 0. And in such cases this function throws an exception ArgumentOutOfRangeException.
If would be safer to write a check:
if (fontFamilyList == null && fontFamily.Length > 1)4. WPF bug
The DependencyProperty is one of the most remarkable features of WPF. Creating properties that can notify the developer right from the box about the changes made is incredibly convenient. But the main thing is to avoid confusing the signature to describe them, it is particularly important to remember this when showing the examples, because that's what people judge by
public double Radius
{
get { return (double) GetValue(RadiusProperty); }
set { SetValue(RadiusProperty, value); }
}
public static readonly DependencyProperty
RadiusProperty = DependencyProperty.Register(
"RadiusBase",
typeof (double),
typeof (FireworkEffect),
new FrameworkPropertyMetadata(15.0));V3045 WPF: the names of the registered property 'RadiusBase', and of the property 'Radius', do not correspond with each other. FireworkEffect.cs 196
In this particular case, the name that is registered for a dependency property does not match the name of the wrapper property to access the DependencyProperty from the code. This option causes big problems when working from XAML markup. WPF allows from XAML access a simple property Radius and read the value from it, but the changes of this property won't get fetched from XAML.
Actually, in PVS-Studio, there are a number of diagnostics to detect errors in the signature when creating DependencyProperty [3044, 3045, 3046, 3047, 3048, 3049]. But most errors of this kind lead to the program crash as soon as the program starts using the class with these dependency properties. That's why these diagnostics are intended to save us from searching and analyzing long texts of signatures, especially after copying. Of course, the most efficient would be to check the code with PVS-Studio regularly, not just do the analysis of the final version of the program.
Let's look at another interesting warning. In this case it was our new diagnostic V3095. This diagnostic shows the places where we access the variable first, and then verify it against null.
private static XmlElement AddOrphanListItems(....)
{
Debug.Assert(htmlLiElement.LocalName.ToLower() == "li");
....
XmlNode htmlChildNode = htmlLiElement;
var htmlChildNodeName = htmlChildNode == null
? null
: htmlChildNode.LocalName.ToLower();
....
}V3095 The 'htmlLiElement' object was used before it was verified against null. Check lines: 916, 936. HtmlToXamlConverter.cs 916
In this case, in the condition of the ternary operator we check if the variable htmlChildNode can be null. At the same time the variable htmlChildNode, is nothing more than a reference to the variable htmlLiElement. But we accessed the variable htmlLiElement without the verification against null. As a result, we have code that will never be executed, or we'll get an exception NullReferenceException in the string htmlLiElement.LocalName.ToLower().
Besides the errors that we've described, a lot of attention is drawn to the diagnostic V3072, which is meant for detecting fields with the type that is implemented by the IDisposable interface, but the class where the fields aren't declared doesn't have this implementation.
internal class Email
{
private readonly SmtpClient _client;
....
}V3072 The 'Email' class containing IDisposable members does not itself implement IDisposable. Inspect: _client. Email.cs 15
IDisposable has always been troublesome. Sometimes Finalize can be of great help, at least in standard classes, to avoid critical errors related to its incorrect usage. Programmers often forget, miss, or just don't pay attention to the field with the type, implementing this interface. It's not that easy to justify such code, or admit having an error there while doing code review, but there are patterns that are worth paying attention to. In this solution there were also quite a lot of these warnings:
1. Errors when writing if statement conditions
It was quite a revelation for me to find C++ projects in this Solution, but nevertheless these are also bugs, so let's take a look.
As in C#, let's start with various comparisons. Let's look at that very C++ bug that I mentioned in the C# block.
STDMETHOD(CreateInstance)(....)
{
....
T *obj = new T();
if (NULL != obj)
{
....
}V668 There is no sense in testing the 'obj' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of a memory allocation error. classfactory.h 76
If the new operator was unable to allocate the memory, then according to the C++ standard, an exception std::bad_alloc() is thrown. Thus, the verification against null is meaningless, as the obj pointer will never be equal to NULL. If it is impossible to allocate the memory, then we have an exception which should be handled on a higher level, and the verification against null can just be deleted. In case it's not desirable to have exceptions in the application, we can use the new operator which doesn't generate exceptions (T *obj = new (std::nothrow) T()), , and thus, the return value can be verified against null. There were four more similar checks in the Solution:
Excessive conditions are common for both programming languages:
if (bitmapLock && bitmap)
{
if(bitmapLock)
{
bitmapLock->Release();
bitmapLock = NULL;
}
}V571 Recurring check. The 'bitmapLock' condition was already verified in line 104. aitdecoder.cpp 106
Some C# programmers aren't aware that the following two operations over the Nullable type are equivalent:
Thatt's why they write the following checks:
if (_isInDesignMode != null && _isInDesignMode.HasValue)At the same time, C++ people like to make pointless verifications against null, before freeing the memory that was allocated by the address that it points to.
static HRESULT OutputColorContext(....)
{
....
if (pixels)
delete[] pixels;
....
}V809 Verifying that a pointer value is not NULL is not required. The 'if (pixels)' check can be removed. aitencoder.cpp 189
static HRESULT OutputBitmapPalette(....)
{
....
if (colors)
delete[] colors;
....
}V809 Verifying that a pointer value is not NULL is not required. The 'if (colors)' check can be removed. aitencoder.cpp 241
static HRESULT OutputColorContext(....)
{
if (bytes)
delete[] bytes;
}V809 Verifying that a pointer value is not NULL is not required. The 'if (bytes)' check can be removed. aitencoder.cpp 292
2. Logic error
The following code shows quite an interesting situation of logical comparison, although you wouldn't say so.
STDMETHODIMP AitDecoder::QueryCapability(....)
{
....
// If this is our format, we can do everything
if (strcmp(bh.Name, "AIT") == 0)
{
*pCapability =
WICBitmapDecoderCapabilityCanDecodeAllImages ||
WICBitmapDecoderCapabilityCanDecodeThumbnail ||
WICBitmapDecoderCapabilityCanEnumerateMetadata ||
WICBitmapDecoderCapabilitySameEncoder;
}
....
}V560 A part of conditional expression is always true. aitdecoder.cpp 634
The diagnostic thought that a part of the condition is always true and it is really right, as the words WICBitmapDecoderCapabilityCanDecodeXXX are just enum values withe the name WICBitmapDecoderCapabilities:
enum WICBitmapDecoderCapabilities
{
WICBitmapDecoderCapabilitySameEncoder = 0x1,
WICBitmapDecoderCapabilityCanDecodeAllImages = 0x2,
WICBitmapDecoderCapabilityCanDecodeSomeImages = 0x4,
WICBitmapDecoderCapabilityCanEnumerateMetadata = 0x8,
WICBitmapDecoderCapabilityCanDecodeThumbnail = 0x10,
WICBITMAPDECODERCAPABILITIES_FORCE_DWORD = 0x7fffffff
};As a result, perhaps, someone confused the symbols, and instead of the bitwise OR "|" wrote logical OR "||". In contrast to the C# compiler, the C++ one didn't see a problem with it.
3. Error in initialization and assigning variables
Of course after refactoring we may have variables that were initialized twice in a row.
STDMETHODIMP BaseFrameEncode::WritePixels(....)
{
result = S_OK;
....
result = factory->CreateBitmapFromMemory(....);
}V519 The 'result' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 269, 279. baseencoder.cpp 279
When the variables are initialized further after several lines of code, we can easily understand, why the person made a mistake. Sometimes such strings are written successively:
STDMETHODIMP AitFrameEncode::Commit()
{
HRESULT result = E_UNEXPECTED;
result = BaseFrameEncode::Commit();
....
}V519 The 'result' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 320, 321. aitencoder.cpp 321
There is a point of view, that C# is less subject to errors than C++, and in some cases it is really so. But an interesting fact is that the majority of errors aren't in specific constructions, but in simple expressions. For example, in the condition of the if statement. Static code analyzer PVS-Studio for C, C++ and C#, will allow you to control the code quality, and will do its best to safeguard you from the fatal errors that can get to your users.