Анализируем ошибки в открытых компонентах Unity3D
Введение
Мы решили проверить все компоненты, библиотеки и демки, написанные на языке C#, чей исходный код предоставлен в официальном репозитории разработчиков Unity3D:
- UI System — система для реализации графического интерфейса.
- Networking — система для реализации мультиплеера.
- MemoryProfiler — система профилирования используемых ресурсов.
- XcodeAPI — компонент для взаимодействия со средой разработки Xcode.
- PlayableGraphVisualizer — система визуализации процесса выполнения проекта.
- UnityTestTools — утилиты тестирования Unity3D (без Unit тестов).
- AssetBundleDemo — проект, содержащий исходники AssetBundleServer’а и демонстрирующий использование AssetBundle системы.
- AudioDemos — проекты, демонстрирующие использование аудио системы.
- NativeAudioPlugins — аудио плагины (нас интересует только код, демонстрирующий их использование).
- GraphicsDemos — проекты, демонстрирующие использование графической системы.
Было бы очень интересно взглянуть на исходники непосредственно ядра движка, но кроме разработчиков ни у кого такой возможности пока нет. Поэтому сегодня на нашем операционном столе лишь малая часть исходных кодов движка, которые мы можем проверить. Наиболее интересными проектами для нас являются: новая UI система, предназначенная для реализации более гибкого графического интерфейса относительно старого топорного GUI, и сетевая библиотека, которая верой и правдой нам служила до появления UNet.
Также не меньшего интереса заслуживает MemoryProfiler, как мощный и гибкий инструмент профилирования ресурсов и нагрузок.
Найденные ошибки и подозрительные места
Все предупреждения, выданные анализатором, можно разделить на 3 уровня:
- Высокий — наиболее вероятная ошибка.
- Средний — возможная ошибка или опечатка.
- Низкий — предупреждение о маловероятно возможной ошибке или опечатке.
Мы будем рассматривать только высокий и средний уровни.
В таблице ниже представлен список проверенных проектов и итоговый результат проверки по всем проектам. Столбцы «Название проекта» и «Количество строк кода», думаю, всем понятны и не должны вызывать вопросов, а вот назначение столбца «Срабатывания анализатора» стоит объяснить. Он содержит в себе информацию о количестве срабатываний анализатора. Позитивными срабатываниями считаются те, которые прямо или косвенно указывают на ошибки или опечатки в коде. Ложные срабатывания — ложные сообщения анализаторы, которые указывают на корректные участки кода, подозревая наличие в них ошибки или опечатки. Как уже и говорилось ранее — все срабатывания разделены на 3 уровня. Мы будем рассматривать только высокий и средний, так как низкий уровень, в основном, содержит информационные сообщения или маловероятные ошибки.
По итогам проверки 10 проектов было получено 16 предупреждений высокого уровня, 75% из которых верно указали на проблемные места в коде, и 18 срабатываний среднего уровня, 39% из которых верно указали на проблемные места. Качество кода следует признать высоким, так как анализатор находит в среднем только одну ошибку на 2000 строк кода. Это хороший результат.
Итак, со статистикой мы закончили. Теперь приступим к рассмотрению непосредственно обнаруженных ошибок и опечаток.
Ошибочное регулярное выражение
V3057 Invalid regular expression patern in constructor. Inspect the first argument. AssetBundleDemo ExecuteInternalMono.cs 48
private static readonly Regex UnsafeCharsWindows =
new Regex("[^A-Za-z0-9\\_\\-\\.\\:\\,\\/\\@\\\\]");
При попытке создания экземпляра класса Regex с данным паттерном мы получим исключение System.ArgumentException с сообщением:
parsing \»[^A-Za-z0–9\\_\\-\\.\\:\\,\\/\\@\\]\» — Unrecognized escape sequence '
Это говорит о том, что данный паттерн является некорректным, и объект Regex не может быть создан с данным паттерном. По всей видимости, программист допустил ошибку при его проектировании.
Возможно обращение к объекту с нулевой ссылкой
V3080 Possible null dereference. Consider inspecting 't.staticFieldBytes'. MemoryProfiller CrawledDataUnpacker.cs 20
.... = packedSnapshot.typeDescriptions.Where(t =>
t.staticFieldBytes != null & t.staticFieldBytes.Length > 0 // <=
)....
После проверки объекта на null происходит обращение к нему. При этом обращение происходит независимо от результата проверки. Это может привести к возникновению исключения NullReferenceException. Вероятнее всего программист планировал использовать оператор условного и &&, но вследствие опечатки используется оператор логического и &.
Обращение к объекту перед проверкой его на null
V3095 The 'uv2.gameObject' object was used before it was verified against null. Check lines: 1719, 1731. UnityEngine.Networking NetworkServer.cs 1719
if (uv2.gameObject.hideFlags == HideFlags.NotEditable ||
uv2.gameObject.hideFlags == HideFlags.HideAndDontSave)
continue;
....
if (uv2.gameObject == null)
continue;
Сначала происходит обращение к объекту, и только потом его проверка на null. Вероятнее всего, если ссылка на объект окажется равной null, то мы получим исключение NullReferenceException, так и не дойдя до проверки.
Помимо приведенной выше ошибки, анализатор обнаружил еще 2 аналогичных срабатывания, которые при определенных обстоятельствах могут вызвать исключения:
- V3095 The 'm_HorizontalScrollbarRect' object was used before it was verified against null. Check lines: 214, 220. UnityEngine.UI ScrollRect.cs 214
- 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 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
if (!baseMat.HasProperty("_StencilReadMask"))
{
Debug.LogWarning(".... _StencilReadMask property", baseMat);
return baseMat;
}
if (!baseMat.HasProperty("_StencilReadMask")) // <=
{
Debug.LogWarning(".... _StencilWriteMask property", baseMat);
return baseMat;
}
Вполне вероятно, программист скопировал часть кода и забыл изменить условия проверки после вставки.
Исходя из этой опечатки, можно сказать, что вторая проверка должна иметь вид:
if (!baseMat.HasProperty("_StencilWriteMask"))
Создание экземпляра класса исключения без дальнейшего использования
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
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 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: port. AssetBundleDemo AssetBundleServer.cs 59
Console.WriteLine("Starting up asset bundle server.", port); // <=
Console.WriteLine("Port: {0}", port);
Console.WriteLine("Directory: {0}", basePath);
В этом случае, судя по смыслу данного участка кода, программист, скорее всего, забыл убрать аргумент из первой строки. С технической точки зрения данная опечатка не является критической и не приведет появлению ошибки, но и в то же время не несет никакой смысловой нагрузки.
Цикл, который может превратиться в вечный при определенных условиях
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
Process masterProcess = Process.GetProcessById((int)processID);
while (masterProcess == null || !masterProcess.HasExited) // <=
{
Thread.Sleep(1000);
}
Вероятнее всего, программист хотел дождаться в цикле момента завершения внешнего процесса, но не учел, что переменная masterProcess может изначально принять значение null, если процесс не будет найден, что вызовет бесконечный цикл. Для корректной работы данного алгоритма необходимо запрашивать процесс по его идентификатору каждую итерацию цикла:
while (true) {
Process masterProcess = Process.GetProcessById((int)processID);
if (masterProcess == null || masterProcess.HasExited) // <=
break;
Thread.Sleep(1000);
}
Небезопасная инициация события
Анализатор обнаружил потенциально небезопасный вызов обработчика события (event). Возможно возникновение исключения NullReferenceException.
V3083 Unsafe invocation of event 'unload', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. AssetBundleDemo AssetBundleManager.cs 47
internal void OnUnload()
{
m_AssetBundle.Unload(false);
if (unload != null)
unload(); // <=
}
В данном участке кода происходит проверка на null поля unload, и затем происходит вызов данного события. Проверка на null позволит избежать исключения в случае, если на событие никто не подписан на момент его вызова.
Однако представим, что у события есть один подписчик. И в момент между проверкой на null и непосредственными вызовом обработчика события существует вероятность, что будет произведена отписка от события, например, в другом потоке. Чтобы обезопасить себя в данной ситуации можно сделать так:
internal void OnUnload()
{
m_AssetBundle.Unload(false);
unload?.Invoke(); // <=
}
Таким образом, проверка события на null и вызов его обработчика будут выполнены в виде одной команды, что обеспечит безопасный вызов события.
Часть логического выражения всегда истинна или ложна
V3063 A part of conditional expression is always false: connId < 0. UnityEngine.Networking ConnectionArray.cs 59
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, так как, используя это выражение в первой проверке, всегда производится выход из функции. Исходя из этого, во второй проверке это выражение не несет никакой смысловой и функциональной нагрузки.
Также была найдена еще одна похожая ошибка.
public bool isServer
{
get
{
if (!m_IsServer)
{
return false;
}
return NetworkServer.active && m_IsServer; // <=
}
}
Думаю, не стоит говорить о том, что данное свойство может быть легко упрощено до вида:
public bool isServer
{
get
{
return m_IsServer && NetworkServer.active;
}
}
Помимо представленных выше двух примеров, в проектах были найдены еще 6 аналогичных ошибок:
- V3022 Expression 'm_Peers == null' is always false. UnityEngine.Networking NetworkMigrationManager.cs 710
- V3022 Expression 'uv2.gameObject == null' is always false. UnityEngine.Networking NetworkServer.cs 1731
- V3022 Expression 'newEnterTarget!= null' is always true. UnityEngine.UI BaseInputModule.cs 147
- V3022 Expression 'pointerEvent.pointerDrag!= null' is always false. UnityEngine.UI TouchInputModule.cs 227
- V3063 A part of conditional expression is always true: currentTest!= null. UnityTestTools TestRunner.cs 237
- V3063 A part of conditional expression is always false: connId < 0. UnityEngine.Networking ConnectionArray.cs 86
Заключение
Как и в любых других проектах, здесь не обошлось без ошибок и опечаток. Как вы могли заметить, PVS-Studio наиболее преуспел в поиске опечаток.
В свою очередь, вы также можете проверить свой, или любой другой проект, написанный на языке C/C++/C#, с помощью данного статического анализатора.
Спасибо всем за внимание! Желаю вам безбажных программ.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Ivan Kishchenko. Discussing Errors in Unity3D’s Open-Source Components.
