Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
GBP
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Ищем и анализируем ошибки в коде Orchar…

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

29 Ноя 2016

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

0456_OrchardCMS_ru/image1.png

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

О проекте Orchard CMS

Описание проекта взято с сайта: http://orchardproject.ru/about-orchard.

Проект 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 строк кода.

0456_OrchardCMS_ru/image2.png

По итогам проверки было получено 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 не мешает нам находить в коде реальные ошибки.

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

0456_OrchardCMS_ru/image3.png

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 секунд

0456_OrchardCMS_ru/image4.png

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

0456_OrchardCMS_ru/image5.png

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

0456_OrchardCMS_ru/image6.png

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

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

0456_OrchardCMS_ru/image7.png

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

Заключение

0456_OrchardCMS_ru/image8.png

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

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

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

Популярные статьи по теме
Зло живёт в функциях сравнения

Дата: 19 Май 2017

Автор: Андрей Карпов

Возможно, читатели помнят мою статью под названием "Эффект последней строки". В ней идёт речь о замеченной мной закономерности: ошибка чаще всего допускается в последней строке однотипных блоков текс…
Любите статический анализ кода!

Дата: 16 Окт 2017

Автор: Андрей Карпов

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

Дата: 22 Окт 2018

Автор: Андрей Карпов

PVS-Studio, как и другие статические анализаторы кода, часто выдаёт ложные срабатывания. Но не стоит спешить считать странные срабатывания ложными. Это короткая история о том, как PVS-Studio вновь ок…
Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей

Дата: 21 Ноя 2018

Автор: Андрей Карпов

Краткое описание технологий, используемых в инструменте PVS-Studio, которые позволяют эффективно обнаруживать большое количество паттернов ошибок и потенциальных уязвимостей. Статья описывает реализа…
Бесплатный PVS-Studio для тех, кто развивает открытые проекты

Дата: 22 Дек 2018

Автор: Андрей Карпов

В канун празднования нового 2019 года команда PVS-Studio решила сделать приятный подарок всем контрибьюторам open-source проектов, хостящихся на GitHub, GitLab или Bitbucket. Им предоставляется возмо…
Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Дата: 31 Июл 2017

Автор: Андрей Карпов

После большой статьи про проверку операционной системы Tizen мне было задано много вопросов о проценте ложных срабатываний и о плотности ошибок (сколько ошибок PVS-Studio выявляет на 1000 строк кода)…
PVS-Studio для Java

Дата: 17 Янв 2019

Автор: Андрей Карпов

В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальн…
Эффект последней строки

Дата: 31 Май 2014

Автор: Андрей Карпов

Я изучил множество ошибок, возникающих в результате копирования кода. И утверждаю, что чаще всего ошибки допускают в последнем фрагменте однотипного кода. Ранее я не встречал в книгах описания этого …
Статический анализ как часть процесса разработки Unreal Engine

Дата: 27 Июн 2017

Автор: Андрей Карпов

Проект Unreal Engine развивается - добавляется новый код и изменятся уже написанный. Неизбежное следствие развития проекта - появление в коде новых ошибок, которые желательно выявлять как можно раньш…
Главный вопрос программирования, рефакторинга и всего такого

Дата: 14 Апр 2016

Автор: Андрей Карпов

Вы угадали, ответ - "42". Здесь приводится 42 рекомендации по программированию, которые помогут избежать множества ошибок, сэкономить время и нервы. Автором рекомендаций выступает Андрей Карпов - тех…

Комментарии (0)

Следующие комментарии
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять