"Как будто у Unreal и Unity родился ребёнок" — такое трогательное описание дали этому движку в GameDev-сообществе. Эта фраза не только мило звучит, но и точно передаёт его суть, ведь движок действительно задумывался как нечто среднее между Unity Engine и Unreal Engine.
Привет вам, дорогие читатели! Я хочу познакомить вас с ещё одной интересной находкой с бескрайних просторов GitHub. Flax Engine — это полноценный мультиплатформенный коммерческий игровой движок от польских разработчиков.
В этой статье мы кратко рассмотрим основные особенности движка, а после разберём наиболее интересные проблемы, найденные в его исходном коде с помощью статического анализатора кода PVS-Studio. Это хороший способ немного прокачать своё умение замечать баги и опечатки в C# и C++ коде.
PVS-Studio — это статический анализатор кода, предназначенный для поиска потенциальных ошибок и уязвимостей в С#, C, C++ и Java коде без его фактического выполнения. С помощью PVS-Studio можно проанализировать как отдельные файлы, так и проект целиком.
Результатом такого анализа является отчёт с предупреждениями, с которым можно удобно работать через специальную панель в ваших любимых IDE:
При желании вы можете получить больше информации об анализаторе, посетив официальный сайт, а сейчас давайте вернемся к движку!
Итак, уважаемые читатели, знакомьтесь, это Flax Engine:
Что же выделяет Flax среди других движков?
Теперь, когда мы немного познакомились с движком, почему бы не узнать, что думает анализатор PVS-Studio о его исходном коде? Далее мы рассмотрим потенциальные проблемы в C# и C++ коде самой актуальной на момент написания статьи версии движка — 1.8.6512.2. Готовы? Тогда поехали!
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.
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.
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++ коде:
Статьи с разбором проблем в C# коде:
Надеюсь, вас также заинтересовал инструмент, который использовался для поиска описанных выше проблем в обширной кодовой базе движка.
Пользуясь моментом, хочу предложить познакомиться и с ним, запросив пробную лицензию на нашем сайте. После этого вы сможете бесплатно опробовать весь функционал анализатора PVS-Studio в течение пробного периода.
До встречи в следующих статьях!