Анализ ошибок в Orchard CMS

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

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

О проекте Orchard CMS

Описание проекта взято с сайта http://www.orchardproject.net/mission.

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

Открытый и бесплатный проект, основанный на запросах сообщества;
Быстрый движок с модульной архитектурой и всеми необходимыми средствами CMS;
Общедоступная онлайн-галерея модулей, тем и других компонентов для расширения сообществом;
Качественная типографика, внимание к верстке и разметке страниц.
Сделайте акцент на создании удобной и функциональной панели администрирования;
Быстрое развертывание системы на рабочем столе и простая публикация на сервере.

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

Четыре предварительных версии были выпущены в течение года, пока Orchard CMS не достигла версии 1.0. Все это время разработчики поддерживали связь с сообществом, принимая запросы, рассматривая комментарии и исправляя ошибки. Проект был запущен на портале codeplex.com для обратной связи с пользователями - http://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

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

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

V3110 Возможна бесконечная рекурсия внутри свойства ReturnTypeCustomAttributes. ContentItemAlteration.cs 121

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

Первая ошибка в нашем списке - довольно распространенная опечатка, которая приведет к бесконечной рекурсии и исключению StackOverflow, что также приведет к сбою программы при достижении значения ReturnTypeCustomAttributes получено. Скорее всего, программист хотел вернуть из свойства поле с тем же именем, что и объект _dynamicMethod; тогда правильный вариант должен быть таким:

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

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

V3110 Возможна бесконечная рекурсия внутри метода IsDefined. 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 в TimeSpan, который не представляет собой полный временной интервал. Возможно, вместо этого было предназначено значение TotalSeconds. 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) // <=
  {
    ....
  }
}

Отсутствует значение перечисления во время проверки переключателя

V3002 Оператор switch не охватывает все значения перечисления TypeCode: 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. Из-за этой ошибки обработка байтов будет проигнорирована. Было бы легче избежать этой ошибки, если бы программист добавил блок default, в котором возникает исключение в отношении необработанных элементов перечисления.

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

V3010 Необходимо использовать возвращаемое значение функции Except. 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 });

Метод подстроки не принимает отрицательных значений.

V3057 Функция Подстрока может получить значение -1, в то время как ожидается неотрицательное значение. Изучите второй аргумент. ContentIdentity.cs 139

.... GetIdentityKeyValue(....) {
  ....
  var indexOfEquals = identityEntry.IndexOf("=");
  if (indexOfEquals < 0) return null;
  var key = identityEntry.Substring(1, indexOfEquals - 1);
  ....
}

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

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

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

V3057 Функция «Подстрока» может получить значение «-1», в то время как ожидается неотрицательное значение. Изучите второй аргумент. CommandParametersParser.cs 18
V3057 Функция «Подстрока» может получить значение «-1», в то время как ожидается неотрицательное значение. Изучите второй аргумент. CommandStep.cs 73

Приоритет операций в заявлении

V3022 Выражение localPart.Name +«. + localField.Name + "." + storageName ’всегда не равно нулю. ContentFieldProperties.cs 56

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

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

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

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

V3022 Выражение "localPart.Name +". " + localField.Name + "." + storageName ’всегда не равно нулю. ContentFieldsSortCriteria.cs 53

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

V3022 Выражение i == 4 всегда ложно. 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 Выражение «результат == ноль» всегда ложно. ContentFieldDriver.cs 175
V3022 Выражение String.IsNullOrWhiteSpace (url) всегда истинно. GalleryController.cs 93
V3022 Выражение ‘_smtpSettings.Host! = Null’ всегда истинно. SmtpMessageChannel.cs 143
V3022 Выражение «петля» всегда ложно. IndexingTaskExecutor.cs 270
V3022 Выражение «Convert.ToString (Shape.Value)» всегда не равно нулю. EditorShapes.cs 372
V3022 Выражение ModelState.IsValid всегда истинно. RecycleBinController.cs 81
V3022 Выражение «previousXml! = Null» всегда истинно. ContentDefinitionEventHandler.cs 104
V3022 Выражение «previousXml! = Null» всегда истинно. ContentDefinitionEventHandler.cs 134
V3022 Выражение «layoutId! = Null» всегда истинно. ProjectionElementDriver.cs 120

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

V3027 Переменная x.Container использовалась в логическом выражении до того, как она была проверена на соответствие нулю в том же логическом выражении. ContainerService.cs 130

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

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

Следующий фрагмент:

V3095 Объект Делегат использовался до того, как он был проверен на соответствие нулю. Проверить строки: 37, 40. SerializableDelegate.cs 37

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

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

V3095 Объект «Делегат» использовался до того, как был проверен на соответствие нулю. Проверить строки: 37, 40. SerializableDelegate.cs 37
V3095 Объект «виджет» использовался до того, как был проверен на соответствие нулю. Проверить строки: 81, 93. TagsWidgetCommands.cs 81

Вывод

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

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

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

Иван Кищенко