# Re\-analysis of Umbraco code 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\. ![0461_Umbraco_Continue/image1.png](https://import.viva64.com/docx/blog/0461_Umbraco_Continue/image1.png) ## Introduction The first [article](https://pvs-studio.com/en/blog/posts/csharp/0357/) about the [Umbraco](https://umbraco.com/) 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](https://github.com/umbraco/Umbraco-CMS), and also the latest version of [PVS\-Studio 6\.11](https://pvs-studio.com/en/pvs-studio/download/)\. In the results of the check, we got 508 warnings\. 71 warnings were first level, 358 \- second level, 79 \- third level\. ![0461_Umbraco_Continue/image2.png](https://import.viva64.com/docx/blog/0461_Umbraco_Continue/image2.png) 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\. ## The analysis results **Potential division by zero** **PVS\-Studio warning:** [V3064](https://pvs-studio.com/en/docs/warnings/v3064/) Potential division by zero\. Consider inspecting denominator 'maxWidthHeight'\. ImageHelper\.cs 154 **PVS\-Studio warning:** [V3064](https://pvs-studio.com/en/docs/warnings/v3064/) Potential division by zero\. Consider inspecting denominator 'maxWidthHeight'\. ImageHelper\.cs 155 ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'context\.Request'\. StateHelper\.cs 369 ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3027/) The variable 'rootDoc' was utilized in the logical expression before it was verified against null in the same logical expression\. publishRootDocument\.cs 34 ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3057/) The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. ContentExtensions\.cs 82 ```cpp 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: ```cpp internal static CultureInfo GetCulture(....) { .... var pos = route.IndexOf('/'); domain = (pos <= 0) ? null : domainHelper.DomainForNode( int.Parse(route.Substring(0, pos)), current) .UmbracoDomain; .... } ``` Similar warnings: * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the first argument\. DefaultUrlProvider\.cs 81 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. DefaultUrlProvider\.cs 84 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the first argument\. DefaultUrlProvider\.cs 126 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. DefaultUrlProvider\.cs 127 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the first argument\. PublishedContentCache\.cs 147 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. PublishedContentCache\.cs 148 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. ContentFinderByNiceUrlAndTemplate\.cs 35 * V3057 The 'Substring' function could receive the '\-9' value while non\-negative value is expected\. Inspect the second argument\. requestModule\.cs 187 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. Action\.cs 134 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the first argument\. LegacyShortStringHelper\.cs 130 * V3057 The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. StringExtensions\.cs 573 **Time** **is** **money** **PVS\-Studio warning:** [V3057](https://pvs-studio.com/en/docs/warnings/v3057/) The 'DateTime' constructor receives the '0' value while positive value is expected\. Inspect the second argument\. DateTimeExtensions\.cs 24 **PVS\-Studio warning:** [V3057](https://pvs-studio.com/en/docs/warnings/v3057/) The 'DateTime' constructor receives the '0' value while positive value is expected\. Inspect the third argument\. DateTimeExtensions\.cs 24 **PVS\-Studio warning:** [V3057](https://pvs-studio.com/en/docs/warnings/v3057/) The 'DateTime' constructor receives the '0' value while positive value is expected\. Inspect the third argument\. DateTimeExtensions\.cs 26 ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3125/) The 'ct' object was used after it was verified against null\. Check lines: 171, 163\. ContentTypeControllerBase\.cs 171 ```cpp 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 ```cpp 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: * V3125 The '\_repo' object was used after it was verified against null\. Check lines: 104, 78\. Installer\.aspx\.cs 104 * V3125 The 'docRequest\.RoutingContext\.UmbracoContext' object was used after it was verified against null\. Check lines: 57, 39\. ContentFinderByIdPath\.cs 57 * V3125 The 'User' object was used after it was verified against null\. Check lines: 90, 80\. config\.cs 90 * V3125 The '\_repo' object was used after it was verified against null\. Check lines: 254, 247\. installedPackage\.aspx\.cs 254 * V3125 The 'node\.NiceUrl' object was used after it was verified against null\. Check lines: 917, 912\. NodeExtensions\.cs 917 * V3125 The 'dst' object was used after it was verified against null\. Check lines: 58, 55\. DataEditorSetting\.cs 58 * V3125 The 'result' object was used after it was verified against null\. Check lines: 199, 188\. DefaultPreValueEditor\.cs 199 * V3125 The 'result' object was used after it was verified against null\. Check lines: 241, 230\. usercontrolPrevalueEditor\.cs 241 **An error in the format string** **PVS\-Studio warning:** [V3025](https://pvs-studio.com/en/docs/warnings/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 ```cpp public static IHtmlString EnableCanvasDesigner(....) { .... string noPreviewLinks = @""; .... 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: ```cpp public static IHtmlString EnableCanvasDesigner(....) { .... string noPreviewLinks = @""; .... if (....) result = string.Format(noPreviewLinks, cssPath) + Environment.NewLine; .... } ``` Similar warnings: * 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 946 * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: path\. requestModule\.cs 204 * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: Alias\.Replace\(" ", ""\)\. Template\.cs 382 * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: Alias\.Replace\(" ", ""\)\. Template\.cs 387 * V3025 Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: this\.Value\.ClientID\. SliderPrevalueEditor\.cs 221 **Untimely** **verification** **against** **null\.** **Again** **PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'dataset' object was used before it was verified against null\. Check lines: 48, 49\. ImageCropperBaseExtensions\.cs 48 ```cpp internal static ImageCropData GetCrop(....) { var imageCropDatas = dataset.ToArray(); // <= if (dataset == null || imageCropDatas.Any() == false) return null; .... } ``` Unlike [V3027](https://pvs-studio.com/en/docs/warnings/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: ```cpp internal static ImageCropData GetCrop(....) { var imageCropDatas = dataset?.ToArray(); if (imageCropDatas == null || !imageCropDatas.Any()) return null; .... } ``` Similar warnings: * V3095 The 'display\.PropertyEditor' object was used before it was verified against null\. Check lines: 30, 43\. ContentPropertyDisplayConverter\.cs 30 * V3095 The 'typedSource' object was used before it was verified against null\. Check lines: 164, 198\. DynamicQueryable\.cs 164 * V3095 The 'attempt\.Result' object was used before it was verified against null\. Check lines: 90, 113\. DynamicPublishedContent\.cs 90 * V3095 The 'actionExecutedContext' object was used before it was verified against null\. Check lines: 47, 76\. FileUploadCleanupFilterAttribute\.cs 47 * V3095 The 'type' object was used before it was verified against null\. Check lines: 92, 96\. assemblyBrowser\.aspx\.cs 92 * V3095 The 'httpContext' object was used before it was verified against null\. Check lines: 235, 237\. UmbracoContext\.cs 235 * V3095 The 'dst' object was used before it was verified against null\. Check lines: 53, 55\. DataEditorSetting\.cs 53 * V3095 The '\_val' object was used before it was verified against null\. Check lines: 46, 55\. CheckBoxList\.cs 46 * V3095 The '\_val' object was used before it was verified against null\. Check lines: 47, 54\. ListBoxMultiple\.cs 47 * V3095 The 'databaseSettings\.ConnectionString' object was used before it was verified against null\. Check lines: 737, 749\. DatabaseContext\.cs 737 * V3095 The 'path' object was used before it was verified against null\. Check lines: 101, 112\. IOHelper\.cs 101 **A** **logic** **error** **PVS\-Studio warning:** [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'name \!\= "Min" \|\| name \!\= "Max"' is always true\. Probably the '&&' operator should be used here\. DynamicPublishedContentList\.cs 415 ```cpp 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: ```cpp 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: * V3022 Expression 'macro \=\= null' is always false\. MacroController\.cs 91 * V3022 Expression 'p\.Value \=\= null' is always false\. ImageCropperPropertyEditor\.cs 216 * V3022 Expression 'loginPageObj \!\= null' is always true\. ProtectPage\.aspx\.cs 93 * V3022 Expression 'dictionaryItem \!\= null' is always true\. TranslateTreeNames\.cs 19 * V3022 Expression '\!IsPostBack' is always true\. EditUser\.aspx\.cs 431 * V3022 Expression 'result\.View \!\= null' is always false\. ControllerExtensions\.cs 129 * V3022 Expression 'string\.IsNullOrEmpty\(UmbracoSettings\.TEMP\_FRIENDLY\_XML\_CHILD\_CONTAINER\_NODENAME\) \=\= false' is always false\. NotFoundHandlers\.cs 128 * V3022 Expression 'mem \!\= null' is always true\. ViewMembers\.aspx\.cs 96 * V3022 Expression 'dtd \!\= null' is always true\. installedPackage\.aspx\.cs 213 * V3022 Expression 'jsonReader\.TokenType \=\= JSONToken\.EndArray && jsonReader\.Value \=\= null' is always false\. JSON\.cs 263 **A** **strange** **loop** **condition** **PVS\-Studio warning:** [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression '\!stop' is always true\. template\.cs 229 ```cpp public Control parseStringBuilder(....) { .... bool stop = false; .... while (!stop) // <= { .... } .... } ``` Another suspicious construction, detected by the [V3022](https://pvs-studio.com/en/docs/warnings/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: ![0461_Umbraco_Continue/image3.png](https://import.viva64.com/docx/blog/0461_Umbraco_Continue/image3.png) 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](https://pvs-studio.com/en/docs/warnings/v3110/) Possible infinite recursion inside 'Render' method\. MenuSplitButton\.cs 30 ```cpp protected override void Render(System.Web.UI.HtmlTextWriter writer) { writer.Write(""); base.Render(writer); this.Render(writer); // <= writer.Write("