Time passes inexorably. It feels that just recently we announced the release of the C# static code analyzer, checked the first projects, and started writing articles about it. But a whole year has passed since that moment. It was a year of painstaking and hard work on diagnostic improvements, adding new diagnostic rules, gathering statistics on false positives and eliminating their causes, communicating with users, and tackling a great deal of other issues. It was a year of both small and large successes on this hard, but incredibly interesting, path we have chosen. Now it's time for a re-analysis of the Umbraco project that we checked right after the release of our C# analyzer a year ago.
The first article about the Umbraco analysis was written by my colleague Andrey Karpov. This year the project continued development, and so far it contains about 3340 files with the extension ".cs", which is approximately 425 KLOC (at the time of the first check, the project had 3200 files with the extension ".cs", and 400 KLOC respectively).
On the first check, the analyzer found a relatively small number of errors, which were nevertheless quite interesting to write an article on, and to draw first conclusions about the work of the C# analyzer from. It is far more interesting doing the check now, when the analyzer has obtained dozens of new diagnostic rules, and improved it's mechanisms of searching for bugs; it's also quite amusing to compare the results of the present time check with the one we did a year ago. To do the analysis, I used the latest version of Umbraco source code, which is also available at GitHub, and also the latest version of PVS-Studio 6.11.
In the results of the check, we got 508 warnings. 71 warnings were first level, 358 - second level, 79 - third level.
The overall coefficient of issues density (the number of warnings per KLOC) was 1.12. This is a good indicator which corresponds to approximately one warning per a thousand lines of code. But warnings do not necessarily mean real errors. It's normal for any static analyzer to have a certain percentage of false positives. Quite often, the warnings look like real bugs, but later after inspection, it turns out that it's not so. Therefore, I won't be discussing the low level warnings, as the percentage of false positives is usually quite high there.
I have reviewed the warnings issued by PVS-Studio, and detected approximately 56% false positives at High and Meduim levels. The remaining warnings contain quite suspicious constructs that require additional review, as well as real errors in the code.
What can be said about the quality of the analyzer work, in comparison with 2015? The first thing that caught our eye is that there were none of the warnings present, which had been described in the previous article. It seems (or at least we want to believe) that the Umbraco developers paid attention to Andrey's article, and fixed the errors described in it. Although the project is of course in continuous development, and the bugs could be fixed anyway, during daily work. Anyway - there are almost no old mistakes. Still, there are a lot of new ones! I'll go through the most interesting bugs here.
Potential division by zero
PVS-Studio warning: V3064 Potential division by zero. Consider inspecting denominator 'maxWidthHeight'. ImageHelper.cs 154
PVS-Studio warning: V3064 Potential division by zero. Consider inspecting denominator 'maxWidthHeight'. ImageHelper.cs 155
private static ResizedImage GenerateThumbnail(....)
{
....
if (maxWidthHeight >= 0)
{
var fx = (float)image.Size.Width / maxWidthHeight; // <=
var fy = (float)image.Size.Height / maxWidthHeight; // <=
....
}
....
}The provided code fragment has two possible errors, although the second one will never be executed. The condition of the if block allows the maxWidthHeight variable to be equal to zero, which acts as a divisor inside the block. In general, this code can work normally for quite a long period of time, and this is the biggest danger in it. Looking at the name of the maxWidthHeight, we can conclude that it's value is most likely not equal to zero. Well, what if it is zero at some point of the execution? The correct version of this construction is as follows:
private static ResizedImage GenerateThumbnail(....)
{
....
if (maxWidthHeight > 0)
{
var fx = (float)image.Size.Width / maxWidthHeight;
var fy = (float)image.Size.Height / maxWidthHeight;
....
}
....
}The case where the variable maxWidthHeight is zero, should be inspected separately.
A vexatious typo
PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'context.Request'. StateHelper.cs 369
public static bool HasCookies
{
get
{
var context = HttpContext;
return context != null && context.Request != null & // <=
context.Request.Cookies != null &&
context.Response != null &&
context.Response.Cookies != null;
}
}There is a typo: the & operator is used instead of &&. The condition context.Request.Cookies != null will be checked regardless of the result from the check of the previous condition context.Request != null. This will inevitably lead to access by a zero reference in case the variable context.Request is null. The correct version of this construction is as follows:
public static bool HasCookies
{
get
{
var context = HttpContext;
return context != null && context.Request != null &&
context.Request.Cookies != null &&
context.Response != null &&
context.Response.Cookies != null;
}
}Untimely verification against null
PVS-Studio warning: V3027 The variable 'rootDoc' was utilized in the logical expression before it was verified against null in the same logical expression. publishRootDocument.cs 34
public bool Execute(....)
{
....
if (rootDoc.Text.Trim() == documentName.Trim() && // <=
rootDoc != null && rootDoc.ContentType != null)
....
}The variable rootDoc is verified against null after the accessing via rootDoc.Text. The correct version of this construction is as follows:
public bool Execute(....)
{
....
if (rootDoc != null &&
rootDoc.Text.Trim() == documentName.Trim() &&
rootDoc.ContentType != null)
....
}A negative character index in the string
PVS-Studio warning: V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. ContentExtensions.cs 82
internal static CultureInfo GetCulture(....)
{
....
var pos = route.IndexOf('/');
domain = pos == 0
? null
: domainHelper.DomainForNode(
int.Parse(route.Substring(0, pos)), current) // <=
.UmbracoDomain;
....
}In the route string the program searches for the '/' character, after which the the variable is assigned with the pos variable. The author took into account the possibility of a character in the beginning of the string (pos == 0), but didn't take into account the possibility of its absence: in this case the variable pos will get the value -1. This will cause an exception upon the subsequent usage of the pos variable to extract the substring route.Substring(0, pos). The correct version of this construction is as follows:
internal static CultureInfo GetCulture(....)
{
....
var pos = route.IndexOf('/');
domain = (pos <= 0)
? null
: domainHelper.DomainForNode(
int.Parse(route.Substring(0, pos)), current)
.UmbracoDomain;
....
}Similar warnings:
Time is money
PVS-Studio warning: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the second argument. DateTimeExtensions.cs 24
PVS-Studio warning: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the third argument. DateTimeExtensions.cs 24
PVS-Studio warning: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the third argument. DateTimeExtensions.cs 26
public static DateTime TruncateTo(this DateTime dt,
DateTruncate truncateTo)
{
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 0, 0); // <= x2
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 0); // <=
....
}This small snippet also contains error 3, immediately detected by diagnostic rule V3057. All the errors related to incorrect initialization of the object of the DateTime class, whose constructor is as follows: public DateTime(int year, int month, int day). At the same time, the parameters year, month, and day cannot take values <1. Otherwise, an ArgumentOutOfRangeException will be thrown. The correct version of this construction is as follows:
public static DateTime TruncateTo(this DateTime dt,
DateTruncate truncateTo)
{
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 1, 1);
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 1);
....
}Erroneous condition
PVS-Studio warning: V3125 The 'ct' object was used after it was verified against null. Check lines: 171, 163. ContentTypeControllerBase.cs 171
protected TContentType PerformPostSave<....>(....)
{
var ctId = Convert.ToInt32(....);
....
if (ctId > 0 && ct == null)
throw new HttpResponseException(HttpStatusCode.NotFound);
....
if ((....) &&
(ctId == 0 || ct.Alias != contentTypeSave.Alias)) // <=
....
}There is the possibility of access by the null reference because of the condition (ctId > 0 && ct == null) in this code fragment. The exception HttpResponseException will be thrown only if both parts of the condition are true at the same time. In the case that the ctld variable is <= 0, the work will be continued anyway regardless of the value of the ct variable. The error needs to fixed in the second condition, where the ct is used. The correct version of this construction is as follows
protected TContentType PerformPostSave<....>(....)
{
var ctId = Convert.ToInt32(....);
....
if (ctId > 0 && ct == null)
throw new HttpResponseException(HttpStatusCode.NotFound);
....
if ((....) &&
(ctId == 0 ||
(ct != null && ct.Alias != contentTypeSave.Alias)))
....
}Similar warnings:
An error in the format string
PVS-Studio warning: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Format items not used: {1}. Arguments not used: 1st. HtmlHelperRenderExtensions.cs 938
public static IHtmlString EnableCanvasDesigner(....)
{
....
string noPreviewLinks = @"<link href=""{1}"" type=
""text/css"" rel=""stylesheet"
" data-title=""canvasdesignerCss"" />";
....
if (....)
result = string.Format(noPreviewLinks, cssPath) + // <=
Environment.NewLine;
....
}The format string noPreviewLinks doesn't have a specifier '{0}' for the first argument cssPath of the string.Format method. The result of this code execution will be that we'll get an exception. The correct version of this construction is as follows:
public static IHtmlString EnableCanvasDesigner(....)
{
....
string noPreviewLinks = @"<link href=""{0}"" type=
""text/css"" rel=""stylesheet"
" data-title=""canvasdesignerCss"" />";
....
if (....)
result = string.Format(noPreviewLinks, cssPath) +
Environment.NewLine;
....
}Similar warnings:
Untimely verification against null. Again
PVS-Studio warning: V3095 The 'dataset' object was used before it was verified against null. Check lines: 48, 49. ImageCropperBaseExtensions.cs 48
internal static ImageCropData GetCrop(....)
{
var imageCropDatas = dataset.ToArray(); // <=
if (dataset == null || imageCropDatas.Any() == false)
return null;
....
}Unlike V3027 diagnostic - where the untimely verification against null was found within a single condition - here we are dealing with an attempt to access the null reference in a different statement. The variable dataset is converted to array first, and only then is verified against null. The correct version of this construction is as follows:
internal static ImageCropData GetCrop(....)
{
var imageCropDatas = dataset?.ToArray();
if (imageCropDatas == null || !imageCropDatas.Any())
return null;
....
}Similar warnings:
A logic error
PVS-Studio warning: V3022 Expression 'name != "Min" || name != "Max"' is always true. Probably the '&&' operator should be used here. DynamicPublishedContentList.cs 415
private object Aggregate(....)
{
....
if (name != "Min" || name != "Max") // <=
{
throw new ArgumentException(
"Can only use aggregate min or max methods on properties
which are datetime");
}
....
}As can be seen in the message of the exception, the name variable can only take one of the values "Min", or "Max". At the same time, the condition of the exception should be simultaneous unequal to the name variable "Min" and "Max". But in this fragment the exception will be thrown regardless of the value of name. The correct version of this construction is as follows:
private object Aggregate(....)
{
....
if (name != "Min" && name != "Max")
{
throw new ArgumentException(
"Can only use aggregate min or max methods on properties
which are datetime");
}
....
}In the Umbraco code, the analyzer found 32 more potentially dangerous constructions (although they may just be redundant checks). Here are some of them:
A strange loop condition
PVS-Studio warning: V3022 Expression '!stop' is always true. template.cs 229
public Control parseStringBuilder(....)
{
....
bool stop = false;
....
while (!stop) // <=
{
....
}
....
}Another suspicious construction, detected by the V3022 diagnostic. The variable stop isn't used inside the while block. The block has quite a large code fragment, about 140 lines of code, that's why I won't cite it here. Here is the result of searching for the stop variable:
Most likely, it's not an infinite loop, as we can see a break here, as well as the exception handling blocks. Nevertheless, the loop looks very strange, and may contain a potential error.
Infinite recursion
PVS-Studio warning: V3110 Possible infinite recursion inside 'Render' method. MenuSplitButton.cs 30
protected override void Render(System.Web.UI.HtmlTextWriter writer)
{
writer.Write("</div>");
base.Render(writer);
this.Render(writer); // <=
writer.Write("<div class='btn-group>");
}Apparently, this code fragment has a bug caused by an infinite recursion. After the call of the Render method of the base class, there is a recursive call of the overloaded Render method "by the analogy". Perhaps, the method this.Render should contain some condition of the exit from the recursion. However, it's hard to make a clear conclusion as to what the correct variant of this construction should be.
So, the recheck of the Umbraco project showed significant progress in PVS-Studio, in searching for potentially dangerous and erroneous constructs in C# code. The analyzer has proven its effectiveness once again. Of course, projects should not be checked once a year, because the maximum effect of static analysis is achieved only through regular use. This allows the fixing of bugs effectively, and in good time, without letting them get to the build system, and to the end-users.
Use static analysis! We added the possibility to use our analyzer for free so that everybody could do that. Good luck in the battle against errors and bugless code!