Аналізуємо помилки у відкритих компонентах Unity3D

Unity3D — один з найбільш перспективних і швидкозростаючих ігрових движків на поточний момент. Час від часу, в офіційному репозиторії з'являються нові бібліотеки і компоненти, багато з яких до недавнього часу були недоступні у вигляді джерел. На жаль, команда розробників Unity3D надала на розтерзання громадськості не весь вихідний код проекту, а лише деякі його компоненти, бібліотеки і демки. У цій статті ми спробуємо знайти всілякі помилки в них, використовуючи для цього статичний аналізатор PVS-Studio.



Введення
Ми вирішили перевірити всі компоненти, бібліотеки і демки, написані на мові C#, чий вихідний код надано офіційному репозиторії розробників Unity3D:
  1. UI System — система для реалізації графічного інтерфейсу.
  2. Networking — система для реалізації мультиплеєра.
  3. MemoryProfiler — система профілювання використовуваних ресурсів.
  4. XcodeAPI — компонент для взаємодії із середовищем розробки Xcode.
  5. PlayableGraphVisualizer — система візуалізації процесу виконання проекту.
  6. UnityTestTools — утиліти тестування Unity3D (без Unit тестів).
  7. AssetBundleDemo — проект, що містить вихідні коди AssetBundleServer'а й демонструє використання AssetBundle системи.
  8. AudioDemos — проекти, що демонструють використання аудіо системи.
  9. NativeAudioPlugins — аудіо плагіни (нас цікавить тільки код, що демонструє їх використання).
  10. GraphicsDemos — проекти, що демонструють використання графічної системи.
Було б дуже цікаво поглянути на исходники безпосередньо ядра движка, але крім розробників ні в кого такої можливості поки що немає. Тому сьогодні на нашому операційному столі лише мала частина вихідних кодів движка, які ми можемо перевірити. Найбільш цікавими проектами для нас є: нова UI система, призначена для реалізації більш гнучкого графічного інтерфейсу щодо старого сокирного GUI, і мережева бібліотека, яка вірою і правдою нам служила до появи UNet.

Також не меншого інтересу заслуговує MemoryProfiler, як потужний і гнучкий інструмент профілювання ресурсів і навантажень.

Знайдені помилки і підозрілі місця
Всі попередження, видані аналізатором, можна розділити на 3 рівня:
  1. Високий — найбільш ймовірна помилка.
  2. Середній — можлива помилка або помилка.
  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 format of 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.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.