«Как будто у Unreal и Unity родился ребёнок» — такое трогательное описание дали этому движку в GameDev-сообществе. Эта фраза не только мило звучит, но и точно передаёт его суть, ведь движок действительно задумывался как нечто среднее между Unity Engine и Unreal Engine.
Введение
Привет вам, дорогие читатели! Я хочу познакомить вас с ещё одной интересной находкой с бескрайних просторов GitHub. Flax Engine — это полноценный мультиплатформенный коммерческий игровой движок от польских разработчиков.
В этой статье мы кратко рассмотрим основные особенности движка, а после разберём наиболее интересные проблемы, найденные в его исходном коде с помощью статического анализатора кода PVS-Studio. Это хороший способ немного прокачать своё умение замечать баги и опечатки в C# и C++ коде.
PVS-Studio? Что это?
PVS-Studio — это статический анализатор кода, предназначенный для поиска потенциальных ошибок и уязвимостей в С#, C, C++ и Java коде без его фактического выполнения. С помощью PVS-Studio можно проанализировать как отдельные файлы, так и проект целиком.
Результатом такого анализа является отчёт с предупреждениями, с которым можно удобно работать через специальную панель в ваших любимых IDE:
При желании вы можете получить больше информации об анализаторе, посетив официальный сайт, а сейчас давайте вернемся к движку!
О Flax Engine
Итак, уважаемые читатели, знакомьтесь, это Flax Engine:
Что же выделяет Flax среди других движков?
-
Поддержка скриптинга как на С++, так и на C#. Также имеется возможность визуального программирования;
-
Поддержка .NET 8 и C# 12, что означает возможность не только использовать новейшие функции языка, но и получить прирост в производительности за счёт значительных улучшений среды выполнения. Для более подробного ознакомления с этими нововведениями можете обратиться к нашей статье. Также хочется отметить, что на момент написания статьи, в Unity поддержка этих версий отсутствует;
-
Если вам показалось, что данный движок больше ориентирован на C# разработку, нежели на C++, то вы ошибаетесь. Напротив, ядро движка реализовано на С++, что позволяет соответствующим разработчикам использовать его API напрямую. Это даёт преимущество в производительности, а также возможность использовать некоторые дополнительные API;
-
Наличие весьма солидного для Open Source движка набора инструментов, со списком которых можно ознакомиться на отдельной странице движка;
-
Абсолютно весь функционал движка можно использовать бесплатно, если доход от ваших проектов не превышает $250000 за квартал. При преодолении этого порога стоимость движка будет равна 4% прибыли от ваших проектов, реализованных на его основе.
Теперь, когда мы немного познакомились с движком, почему бы не узнать, что думает анализатор PVS-Studio о его исходном коде? Далее мы рассмотрим потенциальные проблемы в C# и C++ коде самой актуальной на момент написания статьи версии движка — 1.8.6512.2. Готовы? Тогда поехали!
Разбор ошибок в C# коде Flax Engine
Избыточные проверки условий
Case 1
public override string TypeDescription { get { // Translate asset type name var typeName = TypeName; string[] typeNamespaces = typeName.Split('.'); if ( typeNamespaces.Length != 0 && typeNamespaces.Length != 0) // <= { typeName = Utilities.Utils .GetPropertyNameUI( typeNamespaces[typeNamespaces.Length - 1]); } return typeName; } } Предупреждение PVS-Studio:
V3001 There are identical sub-expressions ‘typeNamespaces.Length != 0’ to the left and to the right of the ‘&&’ operator. AssetItem.cs: 83.
Анализатор обнаружил два идентичных выражения, связанных оператором &&. Несомненно, одно из них было указано по ошибке. Но является ли это просто избыточным кодом или все же признаком более серьезной проблемы? Например, вместо второго повторяющегося неравенства могла предполагаться следующая проверка:
typeNamespaces[typeNamespaces.Length - 1].Length != 0 Вот ещё один аналогичный случай, найденный в проекте.
Case 2
public override bool OnMouseDown(Float2 location, MouseButton button) { .... if (_rightMouseDown || (_middleMouseDown && _middleMouseDown)) // <= { // Start navigating StartMouseCapture(); Focus(); return true; } .... } Предупреждение PVS-Studio:
V3001 There are identical sub-expressions ‘_middleMouseDown’ to the left and to the right of the ‘&&’ operator. VisjectSurface.Input.cs: 495.
Ошибка из-за невнимательного copy-paste
partial class Window { .... public void Show() .... public void Hide() .... } public class ContextMenuBase : ContainerControl { private Window _window; .... public void Show() // <= { _window.Show(); } public void Hide() // <= { _window.Show(); } public void Minimize() { _window.Minimize(); } } Предупреждение PVS-Studio:
V3013 It is odd that the body of ‘Show’ function is fully equivalent to the body of ‘Hide’ function (70, line 78). WindowRootControl.cs: 70, 78.
Типичная ошибка из-за невнимательности при использовании Copy-Paste. В методе Hide вызывается метод _window.Show вместо _window.Hide.
Выход за границы массива
public Matrix2x2(float[] values) { .... if (values.Length != 4) throw new ArgumentOutOfRangeException(....); M11 = values[0]; M12 = values[1]; M21 = values[3]; M22 = values[4]; // <= } Предупреждение PVS-Studio:
V3106 Possibly index is out of bound. The ‘4’ index is pointing beyond ‘values’ bound. Matrix2x2.cs: 98.
Из условия видно, что в массиве values может быть только строго четыре элемента. Так как индексация элементов начинается с 0, индекс самого последнего элемента массива будет равен 3. Однако в последнем присваивании выполняется обращение по индексу 4, что непременно приведёт к исключению.
Недостижимый код
Case 1
public void SetMemberValue(object instance, object value) { .... if (type.IsEnum) value = Convert.ChangeType(value, Enum.GetUnderlyingType(type.Type)); else if (type.Type == typeof(byte)) value = Convert.ToByte(value); else if (type.Type == typeof(sbyte)) value = Convert.ToSByte(value); else if (type.Type == typeof(short)) value = Convert.ToInt16(value); else if (type.Type == typeof(int)) // <= value = Convert.ToInt32(value); else if (type.Type == typeof(long)) value = Convert.ToInt64(value); else if (type.Type == typeof(int)) // <= value = Convert.ToUInt16(value); .... } Предупреждение PVS-Studio:
V3003. The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 78, 82. MemberComparison.cs: 78, 82.
Анализатор обнаружил два одинаковых условия внутри связанных else if. В случае, если type.Type будет иметь значение int, код выполнится только в теле первого из них, а код в теле второго будет недостижим. Так как в теле второго рассматриваемого else if выполняется конвертация к типу UInt16, логичным решением была бы замена соответствующего условия на следующее:
type.Type == typeof(ushort) Case 2
private void UpdateDragPositioning(....) { if (....) _dragOverMode = DragItemPositioning.Above; else if (....) _dragOverMode = DragItemPositioning.Below; else _dragOverMode = DragItemPositioning.At; // Update DraggedOverNode var tree = ParentTree; if (_dragOverMode == DragItemPositioning.None) // <= { if (tree != null && tree.DraggedOverNode == this) tree.DraggedOverNode = null; } else if (tree != null) tree.DraggedOverNode = this; } Предупреждение PVS-Studio:
V3022 Expression ‘_dragOverMode == DragItemPositioning.None’ is always false. TreeNode.cs: 566.
Анализатор обнаружил всегда ложное if-условие, из-за чего код внутри then-ветви никогда не будет выполнен.
Обратите внимание, что в результате выполнения первой условной конструкции поле _dragOverMode всегда получает новое значение, отличное от DragItemPositioning.None. Из-за этого следующий if становится бессмысленным.
Не исключено, что это было сделано намеренно, а лишний код просто забыли убрать. Но если это все-таки ошибка, одним из вариантов её исправления может быть перенос первой условной конструкции из начала метода в его конец. Таким образом, исправленный вариант мог бы выглядеть так:
private void UpdateDragPositioning(....) { // Update DraggedOverNode var tree = ParentTree; if (_dragOverMode == DragItemPositioning.None) .... if (....) _dragOverMode = DragItemPositioning.Above; else if (....) _dragOverMode = DragItemPositioning.Below; else _dragOverMode = DragItemPositioning.At; } Это решение не должно нарушить логику метода, но наверняка проверить это могут только разработчики движка.
Проблема с синхронизацией потоков из-за оптимизаций компилятора
protected Window _window; .... public DialogResult ShowDialog(Window parentWindow) { // Show window Show(parentWindow); // Set wait flag Interlocked.Increment(ref _isWaitingForDialog); // Wait for the closing do { Thread.Sleep(1); } while (_window); // <= // Clear wait flag Interlocked.Decrement(ref _isWaitingForDialog); return _result; } Предупреждение PVS-Studio:
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. Dialog.cs: 108.
Этот код содержит потенциальную проблему, которую невозможно отловить при работе с Debug конфигурацией. В чём же секрет неуловимости этого зверя? Дело в том, что возникнуть эта проблема может только при сборке Release версии вследствие оптимизации кода компилятором. Помимо этого, её возникновение зависит и от других факторов, таких как используемая версия .NET и количество процессоров в системе.
Суть проблемы заключается в бесконечном зацикливании while из-за возможного кеширования компилятором значения поля _window. Это может произойти из-за того, что значение поля _window никак не меняется внутри самого цикла, а изменение в других потоках не ожидается компилятором, так как поле было объявлено без ключевого слова volatile. Подробнее об этом можно прочитать в MSDN.
Разбор ошибок в C++ коде Flax Engine
Потенциальное разыменование nullptr
Case 1
void Append(const T* data, int32 length) { .... auto prev = Base::_data; .... Base::_data = (T*)Allocator::Allocate(Base::_length * sizeof(T)); Platform::MemoryCopy(Base::_data, prev, prevLength * sizeof(T)); // <= .... if (_isAllocated && prev) .... } Предупреждение PVS-Studio:
V595 The ‘prev’ pointer was utilized before it was verified against nullptr. Check lines: ‘PlatformBase.h: 178‘, ‘DataContainer.h: 339‘, ‘DataContainer.h: 342‘. DataContainer.h 339.
Анализатор указывает на использование prev в качестве второго аргумента метода MemoryCopy. Потенциальная проблема заключается в том, что prev может иметь значение nullptr, на что указывает соответствующая проверка. Но действительно ли передача nullptr в MemoryCopy опасна? Вдруг этот случай обрабатывается внутри самого метода? Чтобы ответить на эти вопросы, взглянем на реализацию MemoryCopy:
FORCE_INLINE static void MemoryCopy(void* dst, const void* src, uint64 size) { memcpy(dst, src, static_cast<size_t>(size)); } Теперь видно, что значение второго параметра напрямую передаётся в функцию memcpy без какой-либо предварительной проверки на неравенство nullptr, что создаёт риск возникновения неопределённого поведения.
Вдумчивый читатель может не согласиться с этим, возразив: «Здесь же дополнительно передаётся size (количество байт, которое необходимо скопировать), а значит, если этот параметр равен 0, то и копирование выполнено не будет».
Увы, но это не совсем так. Документация на функцию memcpy ясно даёт понять, что в неё нельзя передавать невалидные указатели. Даже если количество копируемых данных равно нулю:
«If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero»
Case 2
void Variant::SetAsset(Asset* asset) { if (Type.Type != VariantType::Asset) SetType(VariantType(VariantType::Asset)); if (AsAsset) { asset->OnUnloaded.Unbind<Variant, // <= &Variant::OnAssetUnloaded>(this); asset->RemoveReference(); // <= } AsAsset = asset; if (asset) { asset->AddReference(); asset->OnUnloaded.Bind<Variant, &Variant::OnAssetUnloaded>(this); } } Предупреждение PVS-Studio:
V595 The ‘asset’ pointer was utilized before it was verified against nullptr. Check lines: 2706, 2709. Variant.cpp: 2706, 2709.
Весьма странный код. Кажется, что в теле первого оператора if должно обрабатываться старое значение AsAsset перед его заменой на новое, на что указывает условие этого if.
Также на ошибку в этом месте указывает условие второго if, в котором выполняется проверка параметра asset на неравенство nullptr. Если ожидается, что asset может быть равен nullptr, значит его разыменование внутри первого if может привести к неопределённому поведению.
Наиболее логичным исправлением в данном случае является замена asset на *AsAsset *внутри первого if:
void Variant::SetAsset(Asset* asset) { .... if (AsAsset) { AsAsset ->OnUnloaded.Unbind<Variant, // <= &Variant::OnAssetUnloaded>(this); AsAsset ->RemoveReference(); // <= } AsAsset = asset; if (asset) { asset->AddReference(); asset->OnUnloaded.Bind<Variant, &Variant::OnAssetUnloaded>(this); } } Потенциальная ошибка при побитовом сдвиге
void StringUtils::ConvertUTF162UTF8(....) { Array<uint32> unicode; .... const uint32 uni = unicode[j]; const uint32 count = uni <= 0x7FU ? 1 : uni <= 0x7FFU ? 2 : uni <= 0xFFFFU ? 3 : uni <= 0x1FFFFFU ? 4 : uni <= 0x3FFFFFFU ? 5 : uni <= 0x7FFFFFFFU ? 6 : 7; // <= to[i++] = (char)(count <= 1 ? (byte)uni : ((byte(0xFFU) << (8 - count)) | byte(uni >> (6 * (count - 1))))); // <= .... } Предупреждение PVS-Studio:
V610 Undefined behavior. Check the shift operator ‘>>’. The right operand (‘(6 * (count — 1))’ = [6..36]) is greater than or equal to the length in bits of the promoted left operand. StringUtilsBase.cpp. 253.
Анализатор обращает внимание на возможность получить неожиданный результат при битовом смещении в выражении uni >> (6 * (count — 1)). Произойти это может из-за того, что uni имеет тип int32. Смещение этого значения на 32 бита и более вправо может привести к неопределённому поведению.
Анализатор вычислил, что наибольший возможный шаг при побитовом сдвиге переменной uni равен 36. Как он это определил? Обратите внимание на тернарный оператор, используемый для инициализации переменной count:
const uint32 count = uni <= 0x7FU ? 1 : uni <= 0x7FFU ? 2 : uni <= 0xFFFFU ? 3 : uni <= 0x1FFFFFU ? 4 : uni <= 0x3FFFFFFU ? 5 : uni <= 0x7FFFFFFFU ? 6 : 7; Видно, что максимальное значение, которое может быть присвоено переменной, равно 7. А теперь подставим это значение в выражение, представляющее собой шаг сдвига:
6 * (count - 1) = 6 * (7 – 1) = 36 Что же, в этот раз анализатор оказался прав. Дело закрыто!
Заключение
На этом статья завершается, надеюсь, она вам понравилась 🙂
А ведь это далеко не первый игровой движок, исходный код которого мы проверяли. На случай, если вам захочется ознакомиться с другими нашими похожими статьями, оставлю ссылки на них ниже.
Статьи с разбором проблем в C++ коде:
-
От винта! Смотрим движок War Thunder и говорим с его создателями
-
Проверка игрового движка qdEngine, часть первая: топ 10 предупреждений PVS-Studio
-
Проверка игрового движка qdEngine, часть вторая: упрощение C++ кода
-
Проверка игрового движка qdEngine, часть третья: дополнительная десятка багов
Статьи с разбором проблем в C# коде:
-
Повторная проверка Unity статическим анализатором PVS-Studio
-
Игра с null: проверка MonoGame статическим анализатором PVS-Studio
Надеюсь, вас также заинтересовал инструмент, который использовался для поиска описанных выше проблем в обширной кодовой базе движка.
Пользуясь моментом, хочу предложить познакомиться и с ним, запросив пробную лицензию на нашем сайте. После этого вы сможете бесплатно опробовать весь функционал анализатора PVS-Studio в течение пробного периода.
До встречи в следующих статьях!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Moskalev. Flax Engine. Exploring game engine & analyzing its source code.
ссылка на оригинал статьи https://habr.com/ru/articles/835720/
Добавить комментарий