Checking the Umbraco Source Code Again

    Time is inexorable. 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 this moment. A year of painstaking and complex work to improve the characteristics of the analyzer, add new diagnostic rules, collect statistics of false positives and eliminate their causes, interact with users and solve a host of other issues. A year of many small and big victories on that difficult but incredibly interesting path that we have chosen for ourselves. It's time to re-check the project, the first one that came to us for research using the new C # analyzer a year ago - Umbraco.


    Introduction


    The search for errors in the Umbraco project was dedicated to an article by my colleague Andrei Karpov. The project continued to develop all this year, and currently contains about 3340 files with the extension “.cs”, which is approximately 425 KLOC (at the time of the verification by Andrey the project contained 3200 files with the extension “.cs” and 400 KLOC, respectively).

    The first test in Umbraco revealed relatively few errors, which, nevertheless, were interesting enough to write an article about this and draw first conclusions about the quality of work of the new C # analyzer. It’s all the more interesting now that the analyzer has gotten dozens of new diagnostic rules and improved error search mechanisms, to re-examine 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 of the audit, 508 warnings were received. Of these, the first level - 71, the second - 358, the third - 79:


    At the same time, the total density coefficient of suspicious sites (the number of warnings per KLOC) is 1.12. This is a good indicator, corresponding to about 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 the author studies the code, it turns out that this is not so. To save time, I will not consider warnings with a Low level, since the percentage of false positives is usually high there.

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

    What can be said about the quality of the analyzer, compared with the 2015 test? The first thing that catches your eye - almost no warning was found from the ones described in the previous article. I want to believe that the authors of the Umbraco project drew attention to Andrei's article and corrected the errors cited there. Although, of course, the project is in continuous development and errors could be corrected in the process of daily 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.

    Validation 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 fragment contains two possible errors at once, although the second will never happen. The condition of the if block allows the variable maxWidthHeight to vanish , which acts as a divisor inside the block. In general, such a code can work fine for quite a long time, and this is its danger. Based on the variable name maxWidthHeight, we can conclude that its value is most likely not to be zero. But what if it does? The corrected version of this design 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 variable maxWidthHeight is equal to zero must be handled separately.

    Annoying typo

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

    Typo allowed: instead of the && operator , & was used . The context.Request.Cookies! = Null condition will be checked regardless of the result of checking the previous context.Request! = Null condition . This will inevitably result in null-reference access if the context.Request variable is null . The corrected version of this design 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;
      }
    }

    Несвоевременная проверка на равенство null

    Предупреждение анализатора PVS-Studio: 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)
      ....
    }

    Переменную rootDoc проверяют на равенство null уже после использования для доступа rootDoc.Text. Исправленный вариант этой конструкции имеет вид:

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

    Отрицательный индекс символа в строке

    Предупреждение анализатора PVS-Studio: 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 line searches for the character '/', after which the index is assigned to the variable pos . The author took into account the possibility of finding a 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 get a value of -1. This will throw an exception the next time you use the pos variable to extract the substring route.Substring (0, pos) . The corrected version of this design 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:

    • 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 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, also detected by the diagnostic rule V3057 . All errors are associated with incorrect initialization of an 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, an ArgumentOutOfRangeException exception will be thrown . The corrected version of this design 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 possibility of subsequent access via a null reference. An HttpResponseException will be thrown only if both parts of the condition are satisfied. If the variable ctId is <= 0, regardless of the value of the variable ct , the work will continue. And the error needs to be fixed already in the second condition, where ct is used . The corrected version of this design 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:

    • 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

    Error in the format term.

    PVS-Studio analyzer 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 = @"";
      ....
      if (....)
        result = string.Format(noPreviewLinks, cssPath) +  // <=
                 Environment.NewLine;
      ....
    }

    The format string noPreviewLinks not specifier comprises '{0}' for the first argument cssPath method string.Format . As a result of executing this code, an exception will be thrown. The corrected version of this design is:

    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 checking for null equality.

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

    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

    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 variable name can take only one of the values ​​“Min” or “Max”. At the same time, the condition for throwing this exception should be the simultaneous inequality of the variable name “Min” and “Max”. And in the given code fragment, an exception will be thrown for any value of name . The corrected version of this design 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 Umbraco code 32 more similar potentially unsafe constructions were found (although, they may turn out to be just superfluous checks). I will give 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

    Strange condition of the loop

    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 stop variable is not used inside the while block . The block contains a rather voluminous piece of code, of the order of 140 lines, so I will not give it. Here are the search results for the stop variable :


    The loop is probably not infinite, since 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("
    "); base.Render(writer); this.Render(writer); // <= writer.Write("

    Also popular now: