Flax Engine. Знакомство с игровым движком и анализ его исходного кода

• Введение
• PVS-Studio? Что это?
• О Flax Engine
• Разбор ошибок в C# коде Flax Engine
  • Избыточные проверки условий
  • Ошибка из-за невнимательного copy-paste
  • Выход за границы массива
  • Недостижимый код
  • Проблема с синхронизацией потоков из-за оптимизаций компилятора
• Разбор ошибок в C++ коде Flax Engine
  • Потенциальное разыменование nullptr
  • Потенциальная ошибка при побитовом сдвиге
• Заключение

"Как будто у 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, часть третья: дополнительная десятка багов
• Герои Кода и Магии: анализ игрового движка VCMI

Статьи с разбором проблем в C# коде:

• Повторная проверка Unity статическим анализатором PVS-Studio
• Разбор ошибок в игровом движке Stride
• Игра с null: проверка MonoGame статическим анализатором PVS-Studio

Надеюсь, вас также заинтересовал инструмент, который использовался для поиска описанных выше проблем в обширной кодовой базе движка.

Пользуясь моментом, хочу предложить познакомиться и с ним, запросив пробную лицензию на нашем сайте. После этого вы сможете бесплатно опробовать весь функционал анализатора PVS-Studio в течение пробного периода.

До встречи в следующих статьях!