Снова проверяем исходный код Umbraco19.12.2016 11:18

Время неумолимо. Казалось бы, только недавно мы анонсировали выход статического анализатора для C# кода, проверили первые проекты и начали писать про это статьи. И вот уже прошел целый год с этого момента. Год кропотливой и сложной работы по улучшению характеристик анализатора, добавлению новых диагностических правил, сбору статистики ложных срабатываний и устранению их причин, взаимодействию с пользователями и решению массы других вопросов. Год множества маленьких и больших побед на том трудном, но невероятно интересном пути, который мы для себя выбрали. Пришло время повторной проверки проекта, первым попавшего к нам для исследования с помощью нового C# анализатора год назад — Umbraco.

Введение

Поиску ошибок в проекте Umbraco была посвящена статья моего коллеги Андрея Карпова. Весь этот год проект продолжал развиваться, и к настоящему времени содержит около 3340 файлов с расширением ».cs», что составляет приблизительно 425 KLOC (на момент проверки Андреем проект содержал 3200 файлов с расширением ».cs» и 400 KLOC соответственно).

При первой проверке в Umbraco было обнаружено относительно немного ошибок, которые, тем не менее, были достаточно интересны для того, чтобы написать об этом статью и сделать первые выводы о качестве работы нового C# анализатора. Тем интереснее сейчас, когда анализатор обзавелся десятками новых диагностических правил и усовершенствованными механизмами поиска ошибок, провести повторную проверку проекта Umbraco и сравнить результаты с теми, которые были получены в ноябре 2015 года. Для проверки я использовал последний вариант исходного кода Umbraco, который, как и прежде, доступен для скачивания на портале GitHub, а также последнюю версию анализатора PVS-Studio 6.11.

В результате проверки было получено 508 предупреждений. Из них первого уровня — 71, второго — 358, третьего — 79:

При этом общий коэффициент плотности подозрительных мест (число предупреждений на KLOC) составляет 1.12. Это неплохой показатель, соответствующий примерно одному предупреждению на тысячу строк кода. Но предупреждения — еще не значит реальные ошибки. Для любого статического анализатора нормальным является некоторый процент ложных срабатываний. Также часто могут быть получены предупреждения, которые очень похожи на реальные ошибки, но при изучении автором кода выясняется, что это не так. Для экономии времени я не буду рассматривать предупреждения с уровнем Low, так как там обычно высок процент ложных срабатываний.

Я проанализировал предупреждения, выданные PVS-Studio, и выявил наличие около 56% ложных срабатываний на уровнях High и Meduim. Оставшиеся предупреждения содержат весьма подозрительные конструкции, к которым стоит внимательно присмотреться, а также реальные ошибки в коде.

Что же можно сказать о качестве работы анализатора, по сравнению с проверкой 2015 года? Первое, что бросается в глаза — не было обнаружено почти ни одного предупреждения из описанных в предыдущей статье. Хочется верить, что авторы проекта Umbraco обратили внимание на статью Андрея и исправили приведенные там ошибки. Хотя, конечно, проект находится в непрерывном развитии и ошибки могли быть исправлены в процессе повседневной работы. Так или иначе — старых ошибок почти нет. Зато есть много новых! Приведу наиболее интересные из них.

Результаты проверки

Потенциальное деление на ноль

Предупреждение анализатора PVS-Studio: V3064 Potential division by zero. Consider inspecting denominator 'maxWidthHeight'. ImageHelper.cs 154
Предупреждение анализатора PVS-Studio: 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;  // <=
    ....
  }
  ....
}

Приведенный фрагмент кода содержит сразу две возможные ошибки, хотя вторая никогда не произойдет. Условие блока if допускает равенство нулю переменной maxWidthHeight, которая выступает в качестве делителя внутри блока. Вообще, такой код может достаточно долго работать нормально, и в этом его опасность. На основании имени переменной maxWidthHeight можно сделать вывод о том, что ее значение, скорее всего, не будет равным нулю. Ну, а если все же будет? Исправленный вариант этой конструкции имеет вид:

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

Случай, когда переменная maxWidthHeight будет равна нулю, необходимо обработать отдельно.

Досадная опечатка

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

Допущена опечатка: вместо оператора && использовали &. Условие context.Request.Cookies!= null будет проверено вне зависимости от результата проверки предыдущего условия context.Request!= null. Это неизбежно приведет к доступу по нулевой ссылке в случае равенства null переменной context.Request. Исправленный вариант этой конструкции имеет вид:

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

В строке route производится поиск символа '/', после чего индекс присваивается переменной pos. Автор учел возможность нахождения символа в начале строки (pos == 0), но не учел вероятность его отсутствия: в этом случае переменная pos получит значение -1. Это приведет к выбросу исключения при последующем использовании переменной pos для извлечения подстроки route.Substring (0, pos). Исправленный вариант этой конструкции имеет вид:

internal static CultureInfo GetCulture(....)
{
  ....
  var pos = route.IndexOf('/');
  domain = (pos <= 0)
    ? null
    : domainHelper.DomainForNode(
      int.Parse(route.Substring(0, pos)), current)
      .UmbracoDomain;
  ....
}

Аналогичные предупреждения:

• 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

Время — деньги

Предупреждение анализатора PVS-Studio: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the second argument. DateTimeExtensions.cs 24
Предупреждение анализатора PVS-Studio: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the third argument. DateTimeExtensions.cs 24
Предупреждение анализатора PVS-Studio: 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);  // <=
  ....
}

Этот небольшой фрагмент кода содержит сразу 3 ошибки, также обнаруженные диагностическим правилом V3057. Все ошибки связаны с неправильной инициализацией объекта класса DateTime, конструктор которого имеет вид: public DateTime (int year, int month, int day). При этом параметры year, month и day не могут принимать значения < 1. В противном случае будет выброшено исключение ArgumentOutOfRangeException. Исправленный вариант этой конструкции имеет вид:

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

Ошибочное условие

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

Из-за условия (ctId > 0 && ct == null) в данном фрагменте кода возникает вероятность последующего доступа по нулевой ссылке. Исключение HttpResponseException будет выброшено только при одновременном выполнении обеих частей условия. В случае же, если переменная ctId будет <= 0, независимо от значения переменной ct, работа будет продолжена. А ошибку необходимо исправить уже во втором условии, где происходит использование ct. Исправленный вариант этой конструкции имеет вид:

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)))
  ....
}

Аналогичные предупреждения:

• 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

Ошибка в сроке формата

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

Строка формата noPreviewLinks не содержит спецификатора '{0}' для первого аргумента cssPath метода string.Format. В результате выполнения данного кода будет выброшено исключение. Исправленный вариант этой конструкции имеет вид:

public static IHtmlString EnableCanvasDesigner(....)
{
  ....
  string noPreviewLinks = @"";
  ....
  if (....)
    result = string.Format(noPreviewLinks, cssPath) +
             Environment.NewLine;
  ....
}

Аналогичные предупреждения:

• 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

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

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

В отличие от диагностики V3027, где несвоевременная проверка на null была найдена в пределах одного условия, здесь мы имеем дело с попыткой доступа по нулевой ссылке в другом операторе. Переменная dataset сначала преобразуется в массив, а только после этого проверяется на равенство null. Исправленный вариант этой конструкции имеет вид:

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

Аналогичные предупреждения:

• 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

Логическая ошибка

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

Как следует из сообщения в исключении, переменная name может принимать только одно из значений «Min» или «Max». При этом, условием выброса данного исключения должно являться одновременное неравенство переменной name «Min» и «Max». А в приведенном фрагменте кода выброс исключения будет происходить при любом значении name. Исправленный вариант этой конструкции имеет вид:

private object Aggregate(....)
{
  ....
  if (name != "Min" && name != "Max")
  {
    throw new ArgumentException(
      "Can only use aggregate min or max methods on properties
       which are datetime");
  }
  ....
}

В коде Umbraco было найдено еще 32 аналогичные потенциально небезопасные конструкции (хотя, они могут оказаться и просто лишними проверками). Приведу некоторые из них:

• 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

Странное условие цикла

Предупреждение анализатора PVS-Studio: V3022 Expression '! stop' is always true. template.cs 229

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

Еще одна подозрительная конструкция, обнаруженная диагностикой V3022. Переменная stop не используется внутри блока while. Внутри блока содержится достаточно объемный фрагмент кода, порядка 140 строк, поэтому я не буду приводить его. Вот результаты поиска переменной stop:

Вероятно, цикл не является бесконечным, так как внутри него содержится break, а также блоки обработки исключений. Тем не менее, цикл выглядит весьма странно и может содержать потенциальную ошибку.

Бесконечная рекурсия

Предупреждение анализатора PVS-Studio: V3110 Possible infinite recursion inside 'Render' method. MenuSplitButton.cs 30

protected override void Render(System.Web.UI.HtmlTextWriter writer)
{
  writer.Write("