# Ищем и анализируем ошибки в коде Media Portal 2 Media Portal 2 — это открытое программное обеспечение класса медиа центр, которое позволяет смотреть видео, фотографии, слушать музыку и многое другое\. Для нас, разработчиков статического анализатора PVS\-Studio, это еще одна возможность проверить интересный проект, рассказать людям \(и разработчикам в том числе\) о найденных ошибках и, в свою очередь, еще раз показать возможности нашего анализатора\. ![0481_Media_Portal_ru/image1.png](https://import.viva64.com/docx/blog/0481_Media_Portal_ru/image1.png) ## О проекте Media Portal 2 Описание проекта взято [из Wikipedia](https://ru.wikipedia.org/wiki/MediaPortal): _MediaPortal_ позволяет компьютеру решать различные ориентированные на развлечения задачи, такие как запись, выставление на паузу и перемотка телевизионных трансляций, наподобие _DVR_ систем \(таких как _TiVo_\)\. Прочая функциональность включает в себя просмотр видео, прослушивание музыки с построением динамических плей\-листов на основе данных музыкальной социальной сети Last\.fm, запуск игр, запись радиотрансляций и просмотр коллекций изображений\. _MediaPortal_ поддерживает систему плагинов и схем оформления, позволяющую расширять базовую функциональность\. Подавляющая часть проекта написана на C\#\. Присутствуют отдельные модули, написанные на C\+\+\. Так же, насколько я понимаю, разработчики Media Portal 2 используют в работе над проектом ReSharper\. Я делаю этот вывод в связи с его упоминанием в файле _\.gitignore_\. Нам не нравится идея сравнивать ReSharper и PVS\-Studio, так как это инструменты разного типа\. Но, как видите, использование ReSharper не мешает нам находить в коде реальные ошибки\. ## Результаты проверки В проверке участвовал 3321 файл\. В общей сложности они содержали 512 435 строк кода\. По итогам проверки в серверном проекте на первом \(высоком\) уровне было получено 72 предупреждения\. Из них 57 явно или косвенно указывали на ошибки, опечатки, проблемные и странные места в коде\. На втором уровне \(среднем\) было получено 79 предупреждений\. По моему субъективному мнению, 53 предупреждения верно указывали на проблемные или странные места в коде\. Третий \(низкий\) уровень предупреждений мы рассматривать не будем, так как это предупреждения с низким уровнем достоверности и, как правило, среди них очень много ложных срабатываний или присутствуют предупреждения, которые не актуальны для многих типов проектов\. ![0481_Media_Portal_ru/image2.png](https://import.viva64.com/docx/blog/0481_Media_Portal_ru/image2.png) Итого анализатор выявил 0\.2 ошибки на 1000 строк кода\. При этом процент ложных срабатываний всего 27%, что является очень хорошим результатом\. Сразу оговорюсь, что довольно тяжело определить, что именно программист имел в виду, когда писал тот или иной код, поэтому места, которые я посчитал ошибочными вполне могут иметь извращенную логику и в рамках конкретного алгоритма работают нормально, но если данный код будет переиспользован в другом месте человеком, не знающим всех нюансов реализации, то с большой долей вероятности это может привести к ошибкам\. Также хочу отметить, что в статье представлены далеко не все ошибки Итак, приступим к рассмотрению наиболее интересных из найденных ошибок\. Более подробно авторы проекта смогут изучить ошибки, запросив у нас временную лицензию и выполнив проверку самостоятельно\. Также, уважаемые читатели, если вы являетесь некоммерческим разработчиком, то предлагаю вам воспользоваться [бесплатной версией](https://pvs-studio.ru/ru/blog/posts/0457/) нашего статического анализатора\. Ее функциональные возможности абсолютно идентичны с платной лицензией, поэтому она идеально подойдет студентам, индивидуальным разработчикам и коллективам энтузиастов\. ### Опечатки при Copy\-Paste ![0481_Media_Portal_ru/image3.png](https://import.viva64.com/docx/blog/0481_Media_Portal_ru/image3.png) Начну, пожалуй, с ряда довольно распространенных ошибок, когда программист, скопировав один блок кода, по невнимательности забыл заменить одну или несколько переменных в нем\. [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'AllocinebId' variable should be used instead of 'CinePassionId' MovieRelationshipExtractor\.cs 126 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3127/) нашла еще несколько интересных опечаток, показывающих всю опасность Copy\-Paste\. [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'Y' variable should be used instead of 'X' PointAnimation\.cs 125 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'X' variable should be used instead of 'Y' Point2DList\.cs 935 ```cpp double dx1 = this[upper].Y - this[middle].X; // <= double dy1 = this[upper].Y - this[middle].Y; ``` [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'attrY' variable should be used instead of 'attrX' AbstractSortByComparableValueAttribute\.cs 94 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'BannerPath'\. TvdbBannerWithThumb\.cs 91 ```cpp if (ThumbPath == null && (BannerPath != null || BannerPath.Equals(""))) // <= { ThumbPath = String.Concat("_cache/", BannerPath); } ``` Данный участок кода является довольно странным, если учесть код под проверкой\. На мой взгляд проверка должна срабатывать, если переменная _BannerPath_ не является нулем и пустой строкой\. Тогда исправленный вариант мог бы выглядеть так: ```cpp if (ThumbPath == null && !string.IsNullOrEmpty(BannerPath)) { ThumbPath = String.Concat("_cache/", BannerPath); } ``` ### Неправильный приоритет операторов ![0481_Media_Portal_ru/image4.png](https://import.viva64.com/docx/blog/0481_Media_Portal_ru/image4.png) Предлагаю рассмотреть очередное не менее интересное срабатывание, связанное с неправильным приоритетом логических операторов\. [V3130](https://pvs-studio.ru/ru/docs/warnings/v3130/) Priority of the '&&' operator is higher than that of the '\|\|' operator\. Possible missing parentheses\. BinaryCacheProvider\.cs 495 ```cpp return config.EpisodesLoaded || !checkEpisodesLoaded && config.BannersLoaded || !checkBannersLoaded && config.ActorsLoaded || !checkActorsLoaded; ``` Программист, писавший этот код по всей видимости не учел, что оператор _логического И \(&&\)_ имеет более высокий приоритет нежели оператор _логического ИЛИ \(\|\|\)_\. Опять же в очередной раз посоветую явно указывать порядок операций и разделять их скобками\. Еще одна ошибка, связанная с неправильным приоритетом операторов\. Программист не учел, что оператор _\+_ имеет более высокий приоритет, нежели оператор _??_\. [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression '"Invalid header name: " \+ name' is always not null\. The operator '??' is excessive\. HttpRequest\.cs 309 ```cpp ...("Invalid header name: " + name ?? ""); ``` В результате, если переменная name будет равна нулю, она будет добавлена к строке _"Invalid header name: " _как пустая строка, и не будет заменена на выражение _""_\. Само по себе \- это не грубая ошибка и в данном контексте не приведет к падению\. Исправленный вариант выглядел бы следующим образом: ```cpp ...("Invalid header name: " + (name ?? "")); ``` ### Опечатка после приведения типов Еще одна довольно распространенная опечатка, допущенная по невнимательности\. Обратите внимание на переменные _other_ и _obj_\. [V3019](https://pvs-studio.ru/ru/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'obj', 'other'\. EpisodeInfo\.cs 560 ```cpp 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_ приведет к возникновению исключения\. Исправленный участок кода будет иметь следующий вид: ```cpp EpisodeInfo other = obj as EpisodeInfo; if (other == null) return false; if (TvdbId > 0 && other.TvdbId > 0) return TvdbId == other.TvdbId; .... ``` ### Двойное присвоение Еще одна интересная ошибка, найденная статическим анализатором\. Приведенный ниже участок кода не будет иметь смысла, так как переменная _Released_ всегда будет обнулена\. [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'Released' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 57, 56\. OmDbSeasonEpisode\.cs 57 ```cpp DateTime releaseDate; if (DateTime.TryParse(value, out releaseDate)) Released = releaseDate; // <= Released = null; // <= ``` Вероятнее всего выражение с обнулением должно находится в блоке _else_\. Тогда исправленный вариант данного участка кода выглядел бы так: ```cpp DateTime releaseDate; if (DateTime.TryParse(value, out releaseDate)) Released = releaseDate; // <= else Released = null; // <= ``` ### Когда в минуте не всегда 60 секунд ![0481_Media_Portal_ru/image5.png](https://import.viva64.com/docx/blog/0481_Media_Portal_ru/image5.png) [V3118](https://pvs-studio.ru/ru/docs/warnings/v3118/) Milliseconds component of TimeSpan is used, which does not represent full time interval\. Possibly 'TotalMilliseconds' value was intended instead\. Default\.cs 60 ```cpp 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 миллисекунд\. Корректный вариант мог бы выглядеть так: ```cpp double msToNextFrame = _msPerFrame - (DateTime.Now - _frameRenderingStartTime).TotalMilliseconds; ``` ### Неправильный порядок аргументов Еще одна ошибка из разряда опечаток по невнимательности\. Метод _TryCreateMultimediaCDDriveHandler_ принимает перечисления идентификаторов для видео, изображений и аудио в указанной последовательности\. [V3066](https://pvs-studio.ru/ru/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'TryCreateMultimediaCDDriveHandler' method\. RemovableMediaManager\.cs 109 ```cpp public static MultimediaDriveHandler TryCreateMultimediaCDDriveHandler(DriveInfo driveInfo, IEnumerable videoMIATypeIds, IEnumerable imageMIATypeIds, // <= IEnumerable audioMIATypeIds) // <= { .... } ``` Поскольку эти параметры имеют одинаковые типы \- программист не обратил внимание на то, что при передаче аргументов в метод, перепутал местами изображение и аудио: ```cpp public static ....() { MultimediaDriveHandler.TryCreateMultimediaCDDriveHandler(driveInfo, Consts.NECESSARY_VIDEO_MIAS, Consts.NECESSARY_AUDIO_MIAS, // <= Consts.NECESSARY_IMAGE_MIAS) // <= } ``` ### Условие всегда ложно Данный код довольно странный, поэтому я долго думал, стоит ли его включать в статью: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'IsVignetteLoaded' is always false\. TvdbFanartBanner\.cs 219 ```cpp if (IsVignetteLoaded) // <= { Log.Warn(....); return false; } try { if (IsVignetteLoaded) // <= { LoadVignette(null); } .... ``` Могу предположить, что первая проверка была добавлена в целях отладки, и по всей видимости программист забыл ее удалить\. В результате она блокирует вторую проверку, что может привести к неправильной работе программы\. ### Избыточная проверка, или грубая опечатка? [V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'screenWidth \!\= \_screenSize\.Width' to the left and to the right of the '\|\|' operator\. MainForm\.cs 922 ```cpp if (bitDepth != _screenBpp || screenWidth != _screenSize.Width || screenWidth != _screenSize.Width) // <= { .... } ``` Обратите внимание на последнюю проверку\. Вероятнее всего программист хотел проверить ширину и высоту, но после Copy\-Paste забыл заменить в последней проверке _Width_ на _Height_\. Анализатор нашел так же еще одну аналогичную ошибку: [V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'p \=\= null' to the left and to the right of the '\|\|' operator\. TriangulationConstraint\.cs 141 ```cpp public static uint CalculateContraintCode( TriangulationPoint p, TriangulationPoint q) { if (p == null || p == null) // <= { throw new ArgumentNullException(); } .... } ``` При более детальном рассмотрении тела метода можно заметить, что параметр _p _дважды проверяется на _null_, при этом логика работы данного метода подразумевает так же использование параметра _q_\. Вероятнее всего правая часть проверки должна содержать проверку переменной _q_ вместо _p_\. ### Забытое условие или еще немного копипасты Как вы могли заметить, большинство ошибок в этой статье \- это опечатки при Copy\-Paste, и следующая ошибка \- не исключение\. [V3003](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp if (style == NumberStyles.Integer) { int ivalue; if (int.TryParse(num, out ivalue)) return ivalue; .... } else if (style == NumberStyles.Integer) // <= { return double.Parse(num); } ``` В обоих проверках переменную _style_ сравнивают с одним и тем же значением в перечислении\. Как следствие вторая проверка никогда не будет выполнена\. Если учитывать тот факт, что в первой проверке строку преобразуют к целому числу, а во второй проверке \- к числу с плавающей запятой\. Вероятнее всего условие второй проверки должно было бы выглядеть так: ```cpp .... } else if (style == NumberStyles.Double) // <= { return double.Parse(num); } ``` ## Заключение ![0481_Media_Portal_ru/image6.png](https://import.viva64.com/docx/blog/0481_Media_Portal_ru/image6.png) В данном проекте были найдены и другие ошибки, опечатки, недочёты\. Но мне они не показались интересными настолько, чтобы описывать их в статье\. В целом могу сказать, что кодовая база проекта плохо читаема и содержит множество странных мест\. Большинство из них не были описаны в статье, но я их все же отношу к плохому стилю написания кода\. Сюда можно отнести использование цикла _foreach _для получения первого элемента коллекции и выход из него при помощи _break_ в конце первой итерации, многочисленные избыточные проверки, большие монолитно\-слепленные блоки кода и т\.д\. Разработчики _Media Portal 2_ легко смогут найти все недочёты, воспользовавшись инструментом [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Вы так же можете поискать ошибки в своих проектах, воспользовавшись предложенным выше статическим анализатором\. Хочу напомнить, что основная польза от статического анализа заключается в регулярном его использовании\. Скачать и разово проверить код \- это несерьезно\. Например, предупреждения компилятора программисты смотрят регулярно, а не включают их раз в 3 года перед одним из релизов\. При регулярном использовании, статический анализатор сэкономит массу времени на поиск опечаток и ошибок\. ## ВАЖНО Мы решили попробовать новый формат популяризации статического анализа кода и инструмента PVS\-Studio\. Теперь мы будем записывать стримы и наглядно демонстрировать, как с помощью PVS\-Studio можно находить ошибки, бороться с багами и ложными срабатываниями\. Будем отвечать на вопросы и практиковать интерактивное общение с аудиторией, стараясь подстраиваться под ее интересы и показывать то, что наиболее соответствует пожеланиям зрителей\. Первый стрим посвятили этой статье\. В режиме online мы покажем, как проверять проект Media Portal 2 и находить ошибки\. Это первый эксперимент, поэтому просим отнестись с пониманием\. Заранее предупреждаем, что по неопытности можем столкнуться с техническими накладками\. Тем не менее, просим всех читателей присоединиться к просмотру\. Если формат понравится, то будем практиковать подобные мероприятия регулярно\. Стрим состоится сегодня \(06\.03\.2017\) в 15:00\. Ждем всех [pvs\_studio\_ru](https://www.twitch.tv/pvs_studio_ru) \! И, конечно, предлагаем всем подписаться на наш канал\.