# Анализируем ошибки в открытых компонентах Unity3D Unity3D \- один из самых перспективных и быстроразвивающихся игровых движков на текущий момент\. Время от времени, в официальном репозитории появляются новые библиотеки и компоненты, многие из которых до недавнего времени были недоступны в виде исходников\. К сожалению, команда разработчиков Unity3D предоставила на растерзание общественности не весь исходный код проекта, а только некоторые его компоненты, библиотеки и демки\. В этой статье мы попробуем найти всевозможные ошибки и опечатки в них, используя для этого статический анализатор PVS\-Studio\. ![0423_Unity3D_ru/image1.png](https://import.viva64.com/docx/blog/0423_Unity3D_ru/image1.png) ## Введение Мы решили проверить все компоненты, библиотеки и демки, написанные на языке C\#, чей исходный код предоставлен в [официальном репозитории разработчиков Unity3D](https://bitbucket.org/Unity-Technologies/): 1. UI System \- система для реализации графического интерфейса\. 1. Networking \- система для реализации мультиплеера\. 1. MemoryProfiler \- система профилирования используемых ресурсов\. 1. XcodeAPI \- компонент для взаимодействия со средой разработки Xcode\. 1. PlayableGraphVisualizer \- система визуализации процесса выполнения проекта\. 1. UnityTestTools \- утилиты тестирования Unity3D \(без Unit тестов\)\. 1. AssetBundleDemo \- проект, содержащий исходники AssetBundleServer'а и демонстрирующий использование AssetBundle системы\. 1. AudioDemos \- проекты, демонстрирующие использование аудио системы\. 1. NativeAudioPlugins \- аудио плагины \(нас интересует только код, демонстрирующий их использование\)\. 1. GraphicsDemos \- проекты, демонстрирующие использование графической системы\. Было бы очень интересно взглянуть на исходники непосредственно ядра движка, но кроме разработчиков ни у кого такой возможности пока нет\. Поэтому сегодня на нашем операционном столе лишь малая часть исходных кодов движка, которые мы можем проверить\. Наиболее интересными проектами для нас являются: новая UI система, предназначенная для реализации более гибкого графического интерфейса относительно старого топорного GUI, и сетевая библиотека, которая верой и правдой нам служила до появления UNet\. Также не меньшего интереса заслуживает MemoryProfiler, как мощный и гибкий инструмент профилирования ресурсов и нагрузок\. ## Найденные ошибки и подозрительные места Все предупреждения, выданные анализатором, можно разделить на 3 уровня: 1. Высокий \- наиболее вероятная ошибка\. 1. Средний \- возможная ошибка или опечатка\. 1. Низкий \- предупреждение о маловероятно возможной ошибке или опечатке\. Мы будем рассматривать только высокий и средний уровни\. В таблице ниже представлен список проверенных проектов и итоговый результат проверки по всем проектам\. Столбцы "Название проекта" и "Количество строк кода", думаю, всем понятны и не должны вызывать вопросов, а вот назначение столбца "Срабатывания анализатора" стоит объяснить\. Он содержит в себе информацию о количестве срабатываний анализатора\. Позитивными срабатываниями считаются те, которые прямо или косвенно указывают на ошибки или опечатки в коде\. Ложные срабатывания \- ложные сообщения анализаторы, которые указывают на корректные участки кода, подозревая наличие в них ошибки или опечатки\. Как уже и говорилось ранее \- все срабатывания разделены на 3 уровня\. Мы будем рассматривать только высокий и средний, так как низкий уровень, в основном, содержит информационные сообщения или маловероятные ошибки\. ![0423_Unity3D_ru/image3.png](https://import.viva64.com/docx/blog/0423_Unity3D_ru/image3.png) По итогам проверки 10 проектов было получено 16 предупреждений высокого уровня, 75% из которых верно указали на проблемные места в коде, и 18 срабатываний среднего уровня, 39% из которых верно указали на проблемные места\. Качество кода следует признать высоким, так как анализатор находит в среднем только одну ошибку на 2000 строк кода\. Это хороший результат\. Итак, со статистикой мы закончили\. Теперь приступим к рассмотрению непосредственно обнаруженных ошибок и опечаток\. **Ошибочное регулярное выражение** [V3057](https://pvs-studio.ru/ru/docs/warnings/v3057/) Invalid regular expression patern in constructor\. Inspect the first argument\. AssetBundleDemo ExecuteInternalMono\.cs 48 ```cpp private static readonly Regex UnsafeCharsWindows = new Regex("[^A-Za-z0-9\\_\\-\\.\\:\\,\\/\\@\\\\]"); // <= ``` При попытке создания экземпляра класса _Regex_ с данным паттерном мы получим исключение _System\.ArgumentException_ с сообщением: ```cpp parsing \"[^A-Za-z0-9\\_\\-\\.\\:\\,\\/\\@\\]\" - Unrecognized escape sequence '\\_'. ``` Это говорит о том, что данный паттерн является некорректным, и объект _Regex_ не может быть создан с данным паттерном\. По всей видимости, программист допустил ошибку при его проектировании\. **Возможно обращение к объекту с нулевой ссылкой** [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 't\.staticFieldBytes'\. MemoryProfiller CrawledDataUnpacker\.cs 20 ```cpp .... = packedSnapshot.typeDescriptions.Where(t => t.staticFieldBytes != null & t.staticFieldBytes.Length > 0 // <= ).... ``` После проверки объекта на _null_ происходит обращение к нему\. При этом обращение происходит независимо от результата проверки\. Это может привести к возникновению исключения _NullReferenceException_\. Вероятнее всего программист планировал использовать оператор условного и _&&_, но вследствие опечатки используется оператор логического и _&_\. **Обращение к объекту перед проверкой его на _null_** [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'uv2\.gameObject' object was used before it was verified against null\. Check lines: 1719, 1731\. UnityEngine\.Networking NetworkServer\.cs 1719 ```cpp if (uv2.gameObject.hideFlags == HideFlags.NotEditable || uv2.gameObject.hideFlags == HideFlags.HideAndDontSave) continue; .... if (uv2.gameObject == null) continue; ``` Сначала происходит обращение к объекту, и только потом его проверка на _null_\. Вероятнее всего, если ссылка на объект окажется равной _null_, то мы получим исключение _NullReferenceException,_ так и не дойдя до проверки\. Помимо приведенной выше ошибки, анализатор обнаружил еще 2 аналогичных срабатывания, которые при определенных обстоятельствах могут вызвать исключения: * [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'm\_HorizontalScrollbarRect' object was used before it was verified against null\. Check lines: 214, 220\. UnityEngine\.UI ScrollRect\.cs 214 * [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'm\_VerticalScrollbarRect' object was used before it was verified against null\. Check lines: 215, 221\. UnityEngine\.UI ScrollRect\.cs 215 **Ранее уже встречается оператор _if_ с таким же условием, содержащий в _then_ части безусловный оператор _return_** Очень интересное срабатывание, показывающее всю силу копипасты, классический пример опечатки\. [V3021](https://pvs-studio.ru/ru/docs/warnings/v3021/) There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless UnityEngine\.UI StencilMaterial\.cs 64 ```cpp if (!baseMat.HasProperty("_StencilReadMask")) { Debug.LogWarning(".... _StencilReadMask property", baseMat); return baseMat; } if (!baseMat.HasProperty("_StencilReadMask")) // <= { Debug.LogWarning(".... _StencilWriteMask property", baseMat); return baseMat; } ``` Вполне вероятно, программист скопировал часть кода и забыл изменить условия проверки после вставки\. Исходя из этой опечатки, можно сказать, что вторая проверка должна иметь вид: ```cpp if (!baseMat.HasProperty("_StencilWriteMask")) ``` **Создание экземпляра класса исключения без дальнейшего использования** [V3006](https://pvs-studio.ru/ru/docs/warnings/v3006/) The object was created but it is not being used\. The 'throw' keyword could be missing: throw new ApplicationException\(FOO\)\. AssetBundleDemo AssetBundleManager\.cs 446 ```cpp if (bundleBaseDownloadingURL.ToLower().StartsWith("odr://")) { #if ENABLE_IOS_ON_DEMAND_RESOURCES Log(LogType.Info, "Requesting bundle " + ....); m_InProgressOperations.Add( new AssetBundleDownloadFromODROperation(assetBundleName) ); #else new ApplicationException("Can't load bundle " + ....); // <= #endif } ``` Создается класс _ApplicationException_, но никак не используется\. Вероятнее всего, программист хотел выбросить исключение, но забыл добавить оператор _throw_ перед созданным исключением\. **Неиспользуемые аргументы при форматировании строки** Как известно, при форматировании строк количество выражений типа _\{N\}_ должно соответствовать количеству передаваемых аргументов\. [V3025](https://pvs-studio.ru/ru/docs/warnings/v3025/) Incorrect format\. A different number of format items is expected while calling 'WriteLine' function\. Arguments not used: port\. AssetBundleDemo AssetBundleServer\.cs 59 ```cpp Console.WriteLine("Starting up asset bundle server.", port); // <= Console.WriteLine("Port: {0}", port); Console.WriteLine("Directory: {0}", basePath); ``` В этом случае, судя по смыслу данного участка кода, программист, скорее всего, забыл убрать аргумент из первой строки\. С технической точки зрения данная опечатка не является критической и не приведет появлению ошибки, но и в то же время не несет никакой смысловой нагрузки\. **Цикл, который может превратиться в вечный при определенных условиях** [V3032](https://pvs-studio.ru/ru/docs/warnings/v3032/) Waiting on this expression is unreliable, as compiler may optimize some of the variables\. Use volatile variable\(s\) or synchronization primitives to avoid this\. AssetBundleDemo AssetBundleServer\.cs 16 ```cpp Process masterProcess = Process.GetProcessById((int)processID); while (masterProcess == null || !masterProcess.HasExited) // <= { Thread.Sleep(1000); } ``` Вероятнее всего, программист хотел дождаться в цикле момента завершения внешнего процесса, но не учел, что переменная _masterProcess_ может изначально принять значение null, если процесс не будет найден, что вызовет бесконечный цикл\. Для корректной работы данного алгоритма необходимо запрашивать процесс по его идентификатору каждую итерацию цикла: ```cpp while (true) { Process masterProcess = Process.GetProcessById((int)processID); if (masterProcess == null || masterProcess.HasExited) // <= break; Thread.Sleep(1000); } ``` **Небезопасная инициация события** Анализатор обнаружил потенциально небезопасный вызов обработчика события \(event\)\. Возможно возникновение исключения NullReferenceException\. [V3083](https://pvs-studio.ru/ru/docs/warnings/v3083/) Unsafe invocation of event 'unload', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AssetBundleDemo AssetBundleManager\.cs 47 ```cpp internal void OnUnload() { m_AssetBundle.Unload(false); if (unload != null) unload(); // <= } ``` В данном участке кода происходит проверка на _null_ поля _unload_, и затем происходит вызов данного события\. Проверка на _null_ позволит избежать исключения в случае, если на событие никто не подписан на момент его вызова\. Однако представим, что у события есть один подписчик\. И в момент между проверкой на _null_ и непосредственными вызовом обработчика события существует вероятность, что будет произведена отписка от события, например, в другом потоке\. Чтобы обезопасить себя в данной ситуации можно сделать так: ```cpp internal void OnUnload() { m_AssetBundle.Unload(false); unload?.Invoke(); // <= } ``` Таким образом, проверка события на _null_ и вызов его обработчика будут выполнены в виде одной команды, что обеспечит безопасный вызов события\. **Часть логического выражения всегда истинна или ложна** [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always false: connId < 0\. UnityEngine\.Networking ConnectionArray\.cs 59 ```cpp public NetworkConnection Get(int connId) { if (connId < 0) { return m_LocalConnections[Mathf.Abs(connId) - 1]; } if (connId < 0 || connId > m_Connections.Count) // <= { ... return null; } return m_Connections[connId]; } ``` Выражение _connId < 0_ во второй проверке функции _get_ всегда будет равно _false_, так как, используя это выражение в первой проверке, всегда производится выход из функции\. Исходя из этого, во второй проверке это выражение не несет никакой смысловой и функциональной нагрузки\. Также была найдена еще одна похожая ошибка\. ```cpp public bool isServer { get { if (!m_IsServer) { return false; } return NetworkServer.active && m_IsServer; // <= } } ``` Думаю, не стоит говорить о том, что данное свойство может быть легко упрощено до вида: ```cpp public bool isServer { get { return m_IsServer && NetworkServer.active; } } ``` Помимо представленных выше двух примеров, в проектах были найдены еще 6 аналогичных ошибок: * [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'm\_Peers \=\= null' is always false\. UnityEngine\.Networking NetworkMigrationManager\.cs 710 * [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'uv2\.gameObject \=\= null' is always false\. UnityEngine\.Networking NetworkServer\.cs 1731 * [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'newEnterTarget \!\= null' is always true\. UnityEngine\.UI BaseInputModule\.cs 147 * [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'pointerEvent\.pointerDrag \!\= null' is always false\. UnityEngine\.UI TouchInputModule\.cs 227 * [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always true: currentTest \!\= null\. UnityTestTools TestRunner\.cs 237 * [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always false: connId < 0\. UnityEngine\.Networking ConnectionArray\.cs 86 ## Заключение Как и в любых других проектах, здесь не обошлось без ошибок и опечаток\. Как вы могли заметить, [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) наиболее преуспел в поиске опечаток\. В свою очередь, вы также можете проверить свой, или любой другой проект, написанный на языке C/C\+\+/C\#, с помощью данного статического анализатора\. Спасибо всем за внимание\! Желаю вам безбажных программ\.