Ищем и анализируем ошибки в коде Media Portal 2

• О проекте Media Portal 2
• Результаты проверки
  • Опечатки при Copy-Paste
  • Доступ по нулевой ссылке
  • Неправильный приоритет операторов
  • Опечатка после приведения типов
  • Двойное присвоение
  • Когда в минуте не всегда 60 секунд
  • Неправильный порядок аргументов
  • Условие всегда ложно
  • Избыточная проверка, или грубая опечатка?
  • Забытое условие или еще немного копипасты
• Заключение
• ВАЖНО

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

О проекте Media Portal 2

Описание проекта взято из Wikipedia:

MediaPortal позволяет компьютеру решать различные ориентированные на развлечения задачи, такие как запись, выставление на паузу и перемотка телевизионных трансляций, наподобие DVR систем (таких как TiVo). Прочая функциональность включает в себя просмотр видео, прослушивание музыки с построением динамических плей-листов на основе данных музыкальной социальной сети Last.fm, запуск игр, запись радиотрансляций и просмотр коллекций изображений. MediaPortal поддерживает систему плагинов и схем оформления, позволяющую расширять базовую функциональность.

Подавляющая часть проекта написана на C#. Присутствуют отдельные модули, написанные на C++. Так же, насколько я понимаю, разработчики Media Portal 2 используют в работе над проектом ReSharper. Я делаю этот вывод в связи с его упоминанием в файле .gitignore. Нам не нравится идея сравнивать ReSharper и PVS-Studio, так как это инструменты разного типа. Но, как видите, использование ReSharper не мешает нам находить в коде реальные ошибки.

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

В проверке участвовал 3321 файл. В общей сложности они содержали 512 435 строк кода. По итогам проверки в серверном проекте на первом (высоком) уровне было получено 72 предупреждения. Из них 57 явно или косвенно указывали на ошибки, опечатки, проблемные и странные места в коде. На втором уровне (среднем) было получено 79 предупреждений. По моему субъективному мнению, 53 предупреждения верно указывали на проблемные или странные места в коде. Третий (низкий) уровень предупреждений мы рассматривать не будем, так как это предупреждения с низким уровнем достоверности и, как правило, среди них очень много ложных срабатываний или присутствуют предупреждения, которые не актуальны для многих типов проектов.

Итого анализатор выявил 0.2 ошибки на 1000 строк кода. При этом процент ложных срабатываний всего 27%, что является очень хорошим результатом.

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

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

Опечатки при Copy-Paste

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

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'AllocinebId' variable should be used instead of 'CinePassionId' MovieRelationshipExtractor.cs 126

if (movie.CinePassionId > 0)
  ids.Add(ExternalIdentifierAspect.SOURCE_CINEPASSION,
    movie.CinePassionId.ToString());
if (movie.CinePassionId > 0)                            // <=
  ids.Add(ExternalIdentifierAspect.SOURCE_ALLOCINE,
    movie.AllocinebId.ToString());

Подобные ошибки очень тяжело находить при обычном Code Review. Поскольку код очень сильно слеплен, велика вероятность того, что программист попросту не заметит дефекта. Взглянув на строку, помеченную комментарием можно заметить, что во втором блоке if везде используется слово Allocine вместо CinePassion, но в условии проверки видимо забыли заменить переменную CinePassionId на AllocinebId.

Помимо этой ошибки, диагностика V3127 нашла еще несколько интересных опечаток, показывающих всю опасность Copy-Paste.

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'Y' variable should be used instead of 'X' PointAnimation.cs 125

double distx = (to.X - from.X) / duration;
distx *= timepassed;
distx += from.X;

double disty = (to.X - from.Y) / duration;      // <=
disty *= timepassed;
disty += from.Y;

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'X' variable should be used instead of 'Y' Point2DList.cs 935

double dx1 = this[upper].Y - this[middle].X;    // <=
double dy1 = this[upper].Y - this[middle].Y;

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'attrY' variable should be used instead of 'attrX' AbstractSortByComparableValueAttribute.cs 94

if (attrX != null)
{
  valX = (T?)aspectX.GetAttributeValue(attrX);
}
if (attrY != null)
{
  valX = (T?)aspectX.GetAttributeValue(attrX);   // <=
}

Во всех случаях, в первом блоке происходят вычисления с осью X, а во втором блоке - с осью Y. Если посмотреть на строки, помеченные комментариями, можно увидеть, что программист забыл заменить X на Y или наоборот при Copy-Paste одного из блоков.

Доступ по нулевой ссылке

Языки программирования эволюционируют, а основные способы выстрелить себе в ногу остаются. В приведенном ниже участке кода программист сначала проверяет не равна ли переменная BannerPath нулю. Если же она всё-таки равна нулю, то проверяет через метод Equals ее равенство к пустой строке, что собственно и может стать потенциальной причиной возникновения исключения NullReferenceException.

V3080 Possible null dereference. Consider inspecting 'BannerPath'. TvdbBannerWithThumb.cs 91

if (ThumbPath == null && 
   (BannerPath != null || BannerPath.Equals("")))    // <=
{
  ThumbPath = String.Concat("_cache/", BannerPath);
}

Данный участок кода является довольно странным, если учесть код под проверкой. На мой взгляд проверка должна срабатывать, если переменная BannerPath не является нулем и пустой строкой.

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

if (ThumbPath == null &&
    !string.IsNullOrEmpty(BannerPath))
{
  ThumbPath = String.Concat("_cache/", BannerPath);
}

Неправильный приоритет операторов

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

V3130 Priority of the '&&' operator is higher than that of the '||' operator. Possible missing parentheses. BinaryCacheProvider.cs 495

return config.EpisodesLoaded || !checkEpisodesLoaded &&
       config.BannersLoaded || !checkBannersLoaded &&
       config.ActorsLoaded || !checkActorsLoaded;

Программист, писавший этот код по всей видимости не учел, что оператор логического И (&&) имеет более высокий приоритет нежели оператор логического ИЛИ (||). Опять же в очередной раз посоветую явно указывать порядок операций и разделять их скобками.

Еще одна ошибка, связанная с неправильным приоритетом операторов. Программист не учел, что оператор + имеет более высокий приоритет, нежели оператор ??.

V3022 Expression '"Invalid header name: " + name' is always not null. The operator '??' is excessive. HttpRequest.cs 309

...("Invalid header name: " + name ?? "<null>");

В результате, если переменная name будет равна нулю, она будет добавлена к строке "Invalid header name: " как пустая строка, и не будет заменена на выражение "<null>". Само по себе - это не грубая ошибка и в данном контексте не приведет к падению.

Исправленный вариант выглядел бы следующим образом:

...("Invalid header name: " + (name ?? "<null>"));

Опечатка после приведения типов

Еще одна довольно распространенная опечатка, допущенная по невнимательности. Обратите внимание на переменные other и obj.

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'other'. EpisodeInfo.cs 560

EpisodeInfo other = obj as EpisodeInfo;
if (obj == null) return false;           // <=
if (TvdbId > 0 && other.TvdbId > 0)
  return TvdbId == other.TvdbId;
....

В данном участке кода сначала переменная obj явно приводится к типу EpisodeInfo, а результат приведения записывается в переменную other. Заметьте, что далее везде используется переменная other, но на null проверяется переменная obj. В случае если переменная obj будет иметь тип, отличный от того, к которому ее приводят, то дальнейшая работа с переменной other приведет к возникновению исключения.

Исправленный участок кода будет иметь следующий вид:

EpisodeInfo other = obj as EpisodeInfo;
if (other == null) return false;
if (TvdbId > 0 && other.TvdbId > 0)
  return TvdbId == other.TvdbId;
....

Двойное присвоение

Еще одна интересная ошибка, найденная статическим анализатором. Приведенный ниже участок кода не будет иметь смысла, так как переменная Released всегда будет обнулена.

V3008 The 'Released' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 56. OmDbSeasonEpisode.cs 57

DateTime releaseDate;
if (DateTime.TryParse(value, out releaseDate))
  Released = releaseDate;                       // <=
Released = null; // <=

Вероятнее всего выражение с обнулением должно находится в блоке else. Тогда исправленный вариант данного участка кода выглядел бы так:

DateTime releaseDate;
if (DateTime.TryParse(value, out releaseDate))
  Released = releaseDate;                    // <=
else
  Released = null;                           // <=

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

V3118 Milliseconds component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalMilliseconds' value was intended instead. Default.cs 60

private void WaitForNextFrame()
{
  double msToNextFrame = _msPerFrame - 
    (DateTime.Now - _frameRenderingStartTime).Milliseconds;
  if (msToNextFrame > 0)
    Thread.Sleep(TimeSpan.FromMilliseconds(msToNextFrame));
}

Еще одна довольно распространённая опечатка, возникающая из-за не совсем очевидной реализации типа TimeSpan. Программист, видимо, не знал, что свойство Milliseconds у объекта типа TimeSpan возвращает не суммарное количество миллисекунд на данном временном промежутке, а остаточное количество миллисекунд.

К примеру, если у нас есть временной промежуток длинной 1 секунда 150 миллисекунд, то вызов метода Milliseconds вернет нам всего 150 миллисекунд. Если необходимо вернуть суммарное количество - необходимо использовать свойство TotalMilliseconds. Для данного примера это будет 1150 миллисекунд.

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

double msToNextFrame = _msPerFrame - 
  (DateTime.Now - _frameRenderingStartTime).TotalMilliseconds;

Неправильный порядок аргументов

Еще одна ошибка из разряда опечаток по невнимательности. Метод TryCreateMultimediaCDDriveHandler принимает перечисления идентификаторов для видео, изображений и аудио в указанной последовательности.

V3066 Possible incorrect order of arguments passed to 'TryCreateMultimediaCDDriveHandler' method. RemovableMediaManager.cs 109

public static MultimediaDriveHandler
  TryCreateMultimediaCDDriveHandler(DriveInfo driveInfo,
    IEnumerable<Guid> videoMIATypeIds, 
    IEnumerable<Guid> imageMIATypeIds,           // <= 
    IEnumerable<Guid> audioMIATypeIds)           // <= 
  { .... }

Поскольку эти параметры имеют одинаковые типы - программист не обратил внимание на то, что при передаче аргументов в метод, перепутал местами изображение и аудио:

public static ....()
{
  MultimediaDriveHandler.TryCreateMultimediaCDDriveHandler(driveInfo,
    Consts.NECESSARY_VIDEO_MIAS, 
    Consts.NECESSARY_AUDIO_MIAS,          // <= 
    Consts.NECESSARY_IMAGE_MIAS)          // <=
}

Условие всегда ложно

Данный код довольно странный, поэтому я долго думал, стоит ли его включать в статью:

V3022 Expression 'IsVignetteLoaded' is always false. TvdbFanartBanner.cs 219

if (IsVignetteLoaded)         // <=
{
  Log.Warn(....);
  return false;
}
try
{
  if (IsVignetteLoaded)       // <=
  {
    LoadVignette(null);
  }
....

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

Избыточная проверка, или грубая опечатка?

V3001 There are identical sub-expressions 'screenWidth != _screenSize.Width' to the left and to the right of the '||' operator. MainForm.cs 922

if (bitDepth != _screenBpp ||
    screenWidth != _screenSize.Width ||
    screenWidth != _screenSize.Width)      // <=
{
  ....
}

Обратите внимание на последнюю проверку. Вероятнее всего программист хотел проверить ширину и высоту, но после Copy-Paste забыл заменить в последней проверке Width на Height.

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

V3001 There are identical sub-expressions 'p == null' to the left and to the right of the '||' operator. TriangulationConstraint.cs 141

public static uint CalculateContraintCode(
  TriangulationPoint p, TriangulationPoint q)
{
  if (p == null || p == null)                 // <=
  {
    throw new ArgumentNullException();
  }
  
  ....
}

При более детальном рассмотрении тела метода можно заметить, что параметр p дважды проверяется на null, при этом логика работы данного метода подразумевает так же использование параметра q. Вероятнее всего правая часть проверки должна содержать проверку переменной q вместо p.

Забытое условие или еще немного копипасты

Как вы могли заметить, большинство ошибок в этой статье - это опечатки при Copy-Paste, и следующая ошибка - не исключение.

V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 452, 462. Scanner.cs 452

if (style == NumberStyles.Integer)
{
  int ivalue;
  if (int.TryParse(num, out ivalue))
    return ivalue;
  ....
}
else if (style == NumberStyles.Integer) // <=
{
  return double.Parse(num);
}

В обоих проверках переменную style сравнивают с одним и тем же значением в перечислении. Как следствие вторая проверка никогда не будет выполнена. Если учитывать тот факт, что в первой проверке строку преобразуют к целому числу, а во второй проверке - к числу с плавающей запятой. Вероятнее всего условие второй проверки должно было бы выглядеть так:

....
}
else if (style == NumberStyles.Double) // <=
{
  return double.Parse(num);
}

Заключение

В данном проекте были найдены и другие ошибки, опечатки, недочёты. Но мне они не показались интересными настолько, чтобы описывать их в статье. В целом могу сказать, что кодовая база проекта плохо читаема и содержит множество странных мест. Большинство из них не были описаны в статье, но я их все же отношу к плохому стилю написания кода. Сюда можно отнести использование цикла foreach для получения первого элемента коллекции и выход из него при помощи break в конце первой итерации, многочисленные избыточные проверки, большие монолитно-слепленные блоки кода и т.д.

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

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

ВАЖНО

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

Первый стрим посвятили этой статье. В режиме online мы покажем, как проверять проект Media Portal 2 и находить ошибки. Это первый эксперимент, поэтому просим отнестись с пониманием. Заранее предупреждаем, что по неопытности можем столкнуться с техническими накладками. Тем не менее, просим всех читателей присоединиться к просмотру. Если формат понравится, то будем практиковать подобные мероприятия регулярно.

Стрим состоится сегодня (06.03.2017) в 15:00. Ждем всех pvs_studio_ru !

И, конечно, предлагаем всем подписаться на наш канал.