Geekly Articles each Day
📜 ⬆️ ⬇️

Check Umbraco source code again

Time is implacable. It would seem that only recently we announced the release of a static analyzer for C # code, checked the first projects and started writing articles about it. And now a whole year has passed from that moment. A year of painstaking and complex work to improve the analyzer performance, add new diagnostic rules, collect false positives statistics and eliminate their causes, interact with users and solve a host of other issues. A year of many small and big victories on the difficult but incredibly interesting path we have chosen. It's time to re-check the project, the first to come to us for research using the new C # analyzer a year ago - Umbraco.


Introduction


An article by my colleague Andrei Karpov was devoted to searching for errors in the Umbraco project. All this year, the project continued to develop, and to date contains about 3,340 files with the extension ".cs", which is approximately 425 KLOC (at the time of the check by Andrei, the project contained 3,200 files with the extension ".cs" and 400 KLOC, respectively).

During the first check in Umbraco, relatively few errors were found, which, nevertheless, were interesting enough to write an article about it and draw the first conclusions about the quality of the workings of the new C # analyzer. The more interesting now, when the analyzer has got dozens of new diagnostic rules and improved error finding mechanisms, retest the Umbraco project and compare the results with those obtained in November 2015. For verification, I used the latest version of the Umbraco source code, which, as before, is available for download on the GitHub portal, as well as the latest version of the PVS-Studio 6.11 analyzer .
')
As a result, 508 warnings were received. Of these, the first level is 71, the second is 358, the third is 79:


The total density of suspicious sites (the number of warnings per KLOC) is 1.12. This is a good indicator, corresponding to approximately one warning per thousand lines of code. But warnings do not mean real mistakes. For any static analyzer, a certain percentage of false positives is normal. Also, warnings can often be received that are very similar to real errors, but when studying by the author of the code it turns out that this is not the case. To save time, I will not consider warnings with a Low level, since there is usually a high percentage of false positives.

I analyzed the warnings issued by PVS-Studio and revealed about 56% of false positives at the High and Meduim levels. The remaining warnings contain very suspicious constructions that you should carefully look at, as well as real errors in the code.

What can be said about the quality of the analyzer, compared with the test of 2015? The first thing that catches your eye is that almost none of the warnings described in the previous article were detected. I want to believe that the authors of the project Umbraco drew attention to the article by Andrew and corrected the errors cited there. Although, of course, the project is in continuous development and errors could be corrected in the course of everyday work. One way or another, there are almost no old mistakes. But there are many new ones! I will give the most interesting of them.

Test results


Potential division by zero

PVS-Studio analyzer warning : V3064 Potential division by zero. Consider inspecting denominator 'maxWidthHeight'. ImageHelper.cs 154
PVS-Studio analyzer 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 above code snippet contains two possible errors at once, although the second will never happen. The if block condition allows the maxWidthHeight variable to be zero, which acts as a divider within the block. In general, such a code can work normally for quite a long time, and therein lies its danger. Based on the name of the maxWidthHeight variable, it can be concluded that its value is not likely to be zero. Well, if you still will? The corrected version of this construction is:

 private static ResizedImage GenerateThumbnail(....) { .... if (maxWidthHeight > 0) { var fx = (float)image.Size.Width / maxWidthHeight; var fy = (float)image.Size.Height / maxWidthHeight; .... } .... }

The case when the maxWidthHeight variable is zero must be processed separately.

Annoying typo

PVS-Studio analyzer warning : V3080 Possible null dereference. Consider inspecting the '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; } }

A typo was made: instead of the && operator, use &. The context.Request.Cookies! = Null condition will be checked regardless of the result of checking the previous context.Request condition ! = Null . This will inevitably lead to access via a null reference if the context.Request variable is null . The corrected version of this construction is:

 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; } }

Late equality test for null

PVS-Studio analyzer warning : V3027 The variable 'rootDoc' was used 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 checked for null equality already after using rootDoc.Text for access. The corrected version of this construction is:

 public bool Execute(....) { .... if (rootDoc != null && rootDoc.Text.Trim() == documentName.Trim() && rootDoc.ContentType != null) .... }

Negative character index in 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. 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; .... }

The route string is searched for the character '/', after which the index is assigned to the variable pos . The author took into account the possibility of finding the character at the beginning of the line ( pos == 0) , but did not take into account the probability of its absence: in this case, the variable pos will receive the value -1. This will throw an exception when the pos variable is later used to extract the substring route.Substring (0, pos) . The corrected version of this construction is:

 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 analyzer warning : V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the second argument. DateTimeExtensions.cs 24
PVS-Studio analyzer warning : V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the third argument. DateTimeExtensions.cs 24
PVS-Studio analyzer 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 code fragment contains 3 errors at once, which are also detected by the diagnostic rule V3057 . All errors are related to incorrect initialization of the object of the DateTime class, the constructor of which has the form: public DateTime (int year, int month, int day). In this case, the parameters year , month, and day cannot take values ​​<1. Otherwise, the exception will be thrown ArgumentOutOfRangeException. The corrected version of this construction is:

 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 analyzer 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)) // <= .... }

Due to the condition (ctId> 0 && ct == null) in this code fragment there is a probability of subsequent access via a null link. The exception HttpResponseException will be thrown only when both parts of the condition are met simultaneously. In the case, if the variable ctId is <= 0, regardless of the value of the variable ct , the work will continue. And the error must be corrected in the second condition, where ct is used . The corrected version of this construction is:

 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:


Formatting error

PVS-Studio analyzer warning : V3025 Incorrect format. A different number of formatted 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 noPreviewLinks format string does not contain the qualifier '{0}' for the first cssPath argument of the string.Format method. As a result of executing this code, an exception will be thrown. The corrected version of this construction is:

 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:


The late check for equality is null. Again

PVS-Studio analyzer warning : V3095 The 'dataset' object was used 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 the V3027 diagnostics, where a late check for null was found within one condition, here we are dealing with an attempt to access a null reference in another statement. The variable dataset is first converted to an array, and only after that it is checked for null equality. The corrected version of this construction is:

 internal static ImageCropData GetCrop(....) { var imageCropDatas = dataset?.ToArray(); if (imageCropDatas == null || !imageCropDatas.Any()) return null; .... }

Similar warnings:


Logical error

PVS-Studio analyzer 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 follows from the message in the exception, the name variable can take only one of the values ​​“Min” or “Max”. In this case, the condition for the release of this exception should be the simultaneous inequality of the name variable “Min” and “Max”. And in the above code snippet, an exception will be thrown for any name value. The corrected version of this construction is:

 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, another 32 similar potentially unsafe constructions were found (although they may turn out to be just superfluous checks). I will give some of them:


Strange cycle condition

PVS-Studio analyzer warning : V3022 Expression '! Stop' is always true. template.cs 229

 public Control parseStringBuilder(....) { .... bool stop = false; .... while (!stop) // <= { .... } .... }

Another suspicious design detected by the V3022 diagnostic. The variable stop is not used inside a while block. Inside the block there is a fairly large code snippet, about 140 lines, so I will not list it. Here are the search results for the stop variable:


Probably, the cycle is not infinite, as inside it contains a break , as well as exception handling blocks. However, the loop looks very strange and may contain a potential error.

Infinite recursion

PVS-Studio analyzer 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>"); }

Most likely, the specified code fragment contains an error associated with the creation of an infinite recursion. After calling the Render method of the base class, a recursive call to the overloaded Render method “by analogy” is made. Most likely, the this.Render method should contain some condition for exiting recursion. In this case, it is difficult to draw an unambiguous conclusion about the correct version of this design.

Conclusion


So, a re-check of the Umbraco project showed that PVS-Studio has made significant progress in detecting both potentially dangerous and erroneous constructions in C # code. The analyzer again proved its effectiveness. Of course, you should not check the projects at intervals once a year, because the maximum effect of static analysis is achieved with its regular use. This allows you to correct errors in a timely and efficient manner, preventing them from leaking into the assembly system or to users.

Use static analysis! And so that anyone could do this, we added the possibility of free use to our analyzer. Good luck to you in the fight against errors and the perfect code!


If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Re-analysis of Umbraco code

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/317900/

More articles:


All Articles