Ищем и анализируем ошибки в коде Orchard CMS

Orchard — это бесплатная система управления контентом с открытым исходным кодом, являющаяся частью галереи ASP.NET-проектов с открытым исходным кодом некоммерческого фонда Outercurve Foundation.

Для нас, разработчиков статического анализатора PVS-Studio, это еще одна возможность проверить интересный проект, рассказать людям (и разработчикам в том числе) о найденных ошибках и, в свою очередь, еще раз протестировать наш анализатор. Сегодня речь пойдёт об ошибках, найденных в проекте Orchard CMS.

О проекте Orchard CMS

Описание проекта взято с сайта.

Проект Orchard CMS был анонсирован в марте 2010 года с выпуском первой бета-версии проекта. Создатели Orchard CMS поставили перед собой цель построить систему управления контентом на новом успешном фреймворке ASP.NET MVC, которая соответствовала бы следующим требованиям:

1. Открытый бесплатный и свободный проект, зависящий от запросов сообщества;
2. Быстрый движок с модульной архитектурой и всеми необходимыми средствами CMS;
3. Общедоступная онлайн-галерея модулей, тем и других компонентов расширения от сообщества;
4. Высокое качество типографики, внимание к компоновке и разметке страниц;
5. Упор на создание удобной и функциональной панели администрирования;
6. Быстрое развертывание системы на рабочем месте и легкая публикация на сервер.

Первоначально Orchard и его исходные коды лицензировались на основе свободной лицензии MS-PL, но, с выходом первой публичной версии, проект сменил лицензию на более простую и распространенную New BSD License.

Четыре предварительные версии были выпущены в течение года, пока Orchard CMS не достигла версии 1.0. Все это время разработчики держали связь с сообществом, принимая пожелания, учитывая комментарии и исправляя найденные ошибки. Для публикации исходных кодов и сбора отзывов пользователей проект был запущен на портале проектов с открытым исходным кодом codeplex.com по адресу orchard.codeplex.com.

Сегодня можно найти объемную документацию по всем аспектам применения Orchard CMS, можно поучаствовать в обсуждении проекта на форумах, можно отправить отчет об обнаруженной ошибке на багтрекер, можно загрузить последние исходные коды проекта и бинарные сборки.

Кроме страницы для разработчиков был запущен и официальный сайт проекта по адресу http://www.orchardproject.net/, который сегодня содержит всю необходимую для работы с Orchard CMS сопроводительную документацию. Кроме того, на официальном сайте размещена галерея модулей и других компонентов, созданных сообществом для расширения функционала Orchard CMS.

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

В проверке участвовали все исходные файлы (3739 штук) с расширением .cs. В общей сложности они содержали 214 564 строк кода.

По итогам проверки было получено 137 предупреждений. Если рассматривать более детально, то на первом (высоком) уровне было получено 39 предупреждений. 28 из них явно указывали на проблемные места или ошибки. На втором уровне (среднем) было получено 60 предупреждений. По моему субъективному мнению, 31 предупреждение верно указывало на места с ошибками или опечатками. Третий (низкий) уровень предупреждений мы рассматривать не будем, так как это предупреждения с низким уровнем достоверности и, как правило, среди них очень много ложных срабатываний или присутствуют предупреждения, которые не актуальны для многих типов проектов.

Итак, приступим к рассмотрению наиболее интересных из найденных ошибок. Более подробно авторы проекта смогут изучить ошибки, запросив у нас временную лицензию и выполнив проверку самостоятельно.

Так же, насколько я понимаю, разработчики Orchard CMS уже используют в работе над проектом ReSharper. Я делаю этот вывод на основе:

» https://github.com/OrchardCMS/Orchard-Harvest-Website/blob/master/src/Orchard.4.5.resharper

Нам не нравится идея сравнивать ReSharper и PVS-Studio, так как это инструменты разного типа. Но, как видите, использование ReSharper не мешает нам находить в коде реальные ошибки.

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

V3110 Possible infinite recursion inside 'ReturnTypeCustomAttributes' property. ContentItemAlteration.cs 121

public override ICustomAttributeProvider 
  ReturnTypeCustomAttributes 
{
  get { return ReturnTypeCustomAttributes; }
}

Открывает список ошибок у нас довольно распространённая опечатка, которая, при получении значения у свойства ReturnTypeCustomAttributes, приведет к возникновению бесконечной рекурсии и, как следствие, к выбросу исключения StackOverflow, что в свою очередь приведет к «падению» программы. Вероятнее всего программист хотел вернуть из свойства одноименное поле у объекта _dynamicMethod, тогда корректный вариант выглядел бы так:

public override ICustomAttributeProvider 
  ReturnTypeCustomAttributes
{
  get { return _dynamicMethod.ReturnTypeCustomAttributes; }
}

А вот еще одна опечатка, которая приведет к возникновению бесконечной рекурсии, и, как следствие, выбросу исключения StackOverflow.

V3110 Possible infinite recursion inside 'IsDefined' method. ContentItemAlteration.cs 101

public override bool IsDefined(Type attributeType, bool inherit) {
  return IsDefined(attributeType, inherit);
}

В данном случае метод IsDefined вызывает сам себя. Думаю, тут программист также хотел вызвать одноименный метод у объекта _dynamicMethod. Тогда корректный вариант выглядел бы так:

public override bool IsDefined(Type attributeType, bool inherit) {
  return _dynamicMethod.IsDefined(attributeType, inherit);
}

Когда в минуте не всегда 60 секунд

V3118 Seconds component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalSeconds' value was intended instead. AssetUploader.cs 182

void IBackgroundTask.Sweep() 
{
  ....
  // Don't flood the database with progress updates; 
  // Limit it to every 5 seconds.
  if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5) 
  {
    ....
  }
}

Еще одна довольно распространённая опечатка, возникающая из-за не совсем очевидной реализации типа TimeSpan. Из комментария видно, что данный метод должен блокировать запрос к базе данных, если с момента предыдущего запроса прошло менее 5 секунд. Но программист, видимо, не знал, что свойство Seconds у объекта типа TimeSpan возвращает не суммарное количество секунд на данном временном промежутке, а остаточное количество секунд.

К примеру, если у нас есть временной промежуток длинной 1 минута 4 секунды, то вызов метода Seconds вернет нам всего 4 секунды. Если необходимо вернуть суммарное количество секунд — необходимо использовать свойство TotalSeconds. Для данного примера это будет 64 секунды.

Корректный вариант мог бы выглядеть так:

void IBackgroundTask.Sweep() 
{
  ....
  // Don't flood the database with progress updates; 
  // Limit it to every 5 seconds.
  if ((_clock.UtcNow - lastUpdateUtc).TotalSeconds >= 5) // <=
  {
    ....
  }
}

Пропущено значение перечисления при проверке через switch

V3002 The switch statement does not cover all values of the 'TypeCode' enum: Byte. InfosetFieldIndexingHandler.cs 65

public enum TypeCode
{
  Empty = 0,
  Object = 1,
  DBNull = 2,
  Boolean = 3,
  Char = 4,
  SByte = 5,
  Byte = 6,
  Int16 = 7,
  UInt16 = 8,
  Int32 = 9,
  UInt32 = 10,
  Int64 = 11,
  UInt64 = 12,
  Single = 13,
  Double = 14,
  Decimal = 15,
  DateTime = 16,
  String = 18
}

public InfosetFieldIndexingHandler(....)
{
  ....
  var typeCode = Type.GetTypeCode(t);
  switch (typeCode) {
    case TypeCode.Empty:
    case TypeCode.Object:
    case TypeCode.DBNull:
    case TypeCode.String:
    case TypeCode.Char:
      ....
      break;
    case TypeCode.Boolean:
      ....
      break;
    case TypeCode.SByte:
    case TypeCode.Int16:
    case TypeCode.UInt16:
    case TypeCode.Int32:
    case TypeCode.UInt32:
    case TypeCode.Int64:
    case TypeCode.UInt64:
      ....
      break;
    case TypeCode.Single:
    case TypeCode.Double:
    case TypeCode.Decimal:
      ....
      break;
    case TypeCode.DateTime:
      ....
      break;
  }
}

Данный участок кода довольно громоздкий, поэтому тяжело сразу заметить ошибку. В данном случае имеется перечисление TypeCode и метод InfosetFieldIndexingHandler, в котором проверяется, к какому из элементов перечисления принадлежит переменная typeCode. В данном случае программист, писавший этот код, вероятнее всего забыл об одном небольшом элементе по имени Byte, но добавил его собрата SByte. Из-за этой ошибки обработка типа Byte будет проигнорирована. Избежать этой ошибки было бы проще, если бы программист добавил блок default, в котором выбрасывается исключение о необработанном элементе перечисления.

Отсутствие обработки возвращаемого значения из метода Except

V3010 The return value of function 'Except' is required to be utilized. AdminController.cs 140

public ActionResult Preview(string themeId, string returnUrl) {
  ....
  if (_extensionManager.AvailableExtensions()
    ....
  } else {
    var alreadyEnabledFeatures = GetEnabledFeatures();
    ....
    alreadyEnabledFeatures.Except(new[] { themeId }); // <=
    TempData[AlreadyEnabledFeatures] = alreadyEnabledFeatures;
  }
  ....
}

Как известно, метод Except исключает из коллекции, для которой он был вызван, элементы другой коллекции. Но программист, видимо, не знал, либо не обратил внимание на то, что результат работы данного метода возвращает новую коллекцию. Корректный вариант мог бы выглядеть так:

alreadyEnabledFeatures = 
  alreadyEnabledFeatures.Except(new[] { themeId });

Метод Substring не принимает отрицательное значение

V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. ContentIdentity.cs 139

.... GetIdentityKeyValue(....) {
  ....
  var indexOfEquals = identityEntry.IndexOf("=");
  if (indexOfEquals < 0) return null;

  var key = identityEntry.Substring(1, indexOfEquals - 1);

  ....
}

Обратите внимание на проверку переменной indexOfEquals. Проверка защищает от случая, если значение переменной отрицательное. Но нет защиты от нулевого значения.

Если символ '=' находится в самом начале, то возникнет исключение, так как второй аргумент метода Substring() будет иметь отрицательное значение -1.

Были найдены также похожие ошибки, отдельно рассматривать которые нет смысла:

• V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. CommandParametersParser.cs 18
• V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. CommandStep.cs 73

Приоритет операций в выражении

V3022 Expression 'localPart.Name + "." + localField.Name + "." + storageName' is always not null. ContentFieldProperties.cs 56

localPart.Name + "." + localField.Name + "." + storageName ?? ""

В данном случае программист, вероятнее всего, думал, что оператор ?? проверит на null только переменную storageName, и если это так, то вместо нее к выражению будет добавлена пустая строка. Но поскольку оператор + обладает более высоким приоритетом, то на null будет проверено всё выражение. Стоит так же отметить, что если к строке прибавить null, то мы получим ту же строку без изменений. Таким образом, в данном случае можно без опаски выбросить данную проверку как излишнюю и вводящую в заблуждение. Исправленный вариант мог бы выглядеть так:

localPart.Name + "." + localField.Name + "." + storageName

Анализатор также нашел еще одну аналогичную ошибку:

V3022 Expression 'localPart.Name + "." + localField.Name + "." + storageName' is always not null. ContentFieldsSortCriteria.cs 53

Проверка всегда ложная

V3022 Expression 'i == 4' is always false. WebHost.cs 162

public void Clean() {
  // Try to delete temporary files for up to ~1.2 seconds.
  for (int i = 0; i < 4; i++) {
    Log("Waiting 300msec before trying to delete ....");
    Thread.Sleep(300);

    if (TryDeleteTempFiles(i == 4)) {
      Log("Successfully deleted all ....");
      break;
    }
  }
}

Видно, что итератор цикла i всегда будет иметь значение в пределе от 0 до 3, но, несмотря на это, программист внутри тела цикла смело проверяет, имеет ли итератор значение 4. Данная проверка никогда не выполнится, но, не зная истинной цели данного участка кода, мне тяжело сказать насколько опасна эта ошибка в масштабах проекта.

Анализатором был найден так же ряд аналогичных ошибок. Все они говорят о том, что проверка будет всегда либо истинной, либо ложной.

• V3022 Expression 'result == null' is always false. ContentFieldDriver.cs 175
• V3022 Expression 'String.IsNullOrWhiteSpace(url)' is always true. GalleryController.cs 93
• V3022 Expression '_smtpSettings.Host != null' is always true. SmtpMessageChannel.cs 143
• V3022 Expression 'loop' is always false. IndexingTaskExecutor.cs 270
• V3022 Expression 'Convert.ToString(Shape.Value)' is always not null. EditorShapes.cs 372
• V3022 Expression 'ModelState.IsValid' is always true. RecycleBinController.cs 81
• V3022 Expression 'previousXml != null' is always true. ContentDefinitionEventHandler.cs 104
• V3022 Expression 'previousXml != null' is always true. ContentDefinitionEventHandler.cs 134
• V3022 Expression 'layoutId != null' is always true. ProjectionElementDriver.cs 120

Использование члена класса перед проверкой объекта на null

V3027 The variable 'x.Container' was utilized in the logical expression before it was verified against null in the same logical expression. ContainerService.cs 130

query.Where(x => 
           (x.Container.Id == containerId || 
            x.Container == null) && 
            ids.Contains(x.Id));

Как видно из кода выше (нас интересует 2 и 3 строки), программист сначала проверяет обращение к свойству Id у контейнера, и только потом проверяет контейнер на null. Правильным вариантом было бы сначала проверить на null, и только потом обращаться к элементам контейнера.

Следующий случай аналогичен предыдущему. Попробуйте найти здесь ошибку сами.

V3095 The 'Delegate' object was used before it was verified against null. Check lines: 37, 40. SerializableDelegate.cs 37

void ISerializable.GetObjectData(
  SerializationInfo info, 
  StreamingContext context)
{
  info.AddValue("delegateType", Delegate.GetType());
  ....
  if (.... && Delegate != null)
  {
    ....
  }                
}

Анализатором было найдено еще 2 аналогичных ошибки, которые похожи на описанную выше, поэтому приводить их я не буду.

• V3095 The 'Delegate' object was used before it was verified against null. Check lines: 37, 40. SerializableDelegate.cs 37
• V3095 The 'widget' object was used before it was verified against null. Check lines: 81, 93. TagsWidgetCommands.cs 81

Заключение

В данном проекте были найдены и другие ошибки, опечатки, недочёты. Но мне они не показались интересными настолько, чтобы описывать их в статье. Разработчики Orchard CMS легко смогут найти все недочёты, воспользовавшись инструментом PVS-Studio. Вы так же можете поискать ошибки в своих проектах воспользовавшись предложенным выше статическим анализатором.

Хочу напомнить, что основная польза от статического анализа заключается в регулярном его использовании. Скачать и разово проверить код — это несерьезно. Например, предупреждения компилятора программисты смотрят регулярно, а не включают их раз в 3 года перед одним из релизов. При регулярном использовании, статический анализатор сэкономит массу времени на поиск опечаток и ошибок.

P.S. Для тех, кто пропустил новость, хочу напомнить, что PVS-Studio теперь умеет интегрироваться с SonarQube. Статья на эту тему: "Контролируем качество кода с помощью платформы SonarQube".