Our website uses cookies to enhance your browsing experience.
"It's like Unreal and Unity had a baby," as the GameDev community has affectionately described the engine. Not only is that a cute way to describe the engine, but it's also quite spot-on. It's designed to be a "golden mean" between Unity and Unreal Engine.
Hello, dear readers! I'd like to introduce you to another interesting find from the endless GitHub expanses. Flax Engine is a full-fledged, multiplatform commercial game engine by Polish developers.
In this article, we'll briefly review the key features of the engine. Then, we'll look at some of the most interesting issues we found in its source code using PVS-Studio static code analyzer. It's a good approach to boost our skills in spotting bugs and typos in C# and C++ code.
PVS-Studio is a static code analyzer that assists developers in searching for potential errors and vulnerabilities in C#, C, C++, and Java code without executing the program. PVS-Studio can analyze both separate files and the whole project.
The analyzer will generate a report with warnings that can be easily handled via a special panel in your favorite IDEs:
Want to know more about the analyzer? Check out our website. Now let's get back to the engine!
Dear readers, meet Flax Engine:
What makes Flax stand out among other engines?
Now that we've got to learn about the engine a bit, let's see what PVS-Studio will find in its source code. We'll discuss potential issues in the C# and C++ code of the latest engine version, which is 1.8.6512.2 as of this writing. Are you ready? Let's go!
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;
}
}The PVS-Studio warning:
V3001 There are identical sub-expressions 'typeNamespaces.Length != 0' to the left and to the right of the '&&' operator. AssetItem.cs: 83.
The analyzer has detected two identical expressions bounded by the && operator. Most likely, one of them was written just accidentally. Is it just redundant code, or a sign of a more serious problem? For example, instead of the second repeated inequality, the developers might have wanted to write such a check:
typeNamespaces[typeNamespaces.Length - 1].Length != 0Here's another similar case found in the project:
Case 2
public override bool OnMouseDown(Float2 location, MouseButton button)
{
....
if (_rightMouseDown || (_middleMouseDown && _middleMouseDown)) // <=
{
// Start navigating
StartMouseCapture();
Focus();
return true;
}
....
}The PVS-Studio warning:
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();
}
}The PVS-Studio warning:
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.
This is a typical error that occurs when developers aren't careful enough when copy-pasting. The Hide method calls the _window.Show method instead of _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]; // <=
}The PVS-Studio warning:
V3106 Possibly index is out of bound. The '4' index is pointing beyond 'values' bound. Matrix2x2.cs: 98.
The condition shows that the values array has only 4 items. Since the item indexing starts from 0, the array index of the latest item will be 3. However, in the last assignment, the index 4 is accessed, which will certainly lead to an exception being thrown.
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);
....
}The PVS-Studio warning:
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.
The analyzer has detected two identical conditions inside bound else if's. If type.Type has the int value, the code will be executed only in the body of the first one. The code in the second else if body will be unreachable. Since conversion to UInt16 type is in the body of the second else if, it'd be better to replace its condition with the following:
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;
}The PVS-Studio warning:
V3022 Expression '_dragOverMode == DragItemPositioning.None' is always false. TreeNode.cs: 566.
The analyzer has detected an always false if statement, so the then branch code will never be executed.
Note that as the result of the conditional statement, _dragOverMode gets a new value other than DragItemPositioning.None. The next if will be meaningless.
Developers might have written it intentionally, and just forgotten to delete redundant code. However, if it's an error, one of the approaches to fix it is to move the first condition from the beginning of the method to its end. The fixed code version could look like this:
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;
}I don't think this fix may break the method logic, but only the engine developers can say it for sure.
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;
}The PVS-Studio warning:
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.
The code has a potential vulnerability that can't be caught in the Debug configuration. What's the secret to this code beast's stealth? Such an issue may occur only when devs build the Release version because of the code optimization by a compiler. It can also happen for other reasons, like the version of .NET we're using and the number of processors in the system.
It's all the fault of endless looping of while that may occur because of the compiler caching the _window field value. Such an issue arises because the _window field value doesn't change inside the loop, and the compiler doesn't expect changes in other threads. The thing is that field is declared without the volatile keyword. You can read more about this on 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)
....
}The PVS-Studio warning:
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.
The analyzer indicates the use of prev as the second argument of the MemoryCopy method. A potential problem is that prev may be nullptr, as indicated by the check. But is passing nullptr to MemoryCopy that dangerous? What if it is handled inside the function? To answer these questions, let's take a look at the MemoryCopy implementation:
FORCE_INLINE static void MemoryCopy(void* dst, const void* src, uint64 size)
{
memcpy(dst, src, static_cast<size_t>(size));
}We can now see that the second parameter value is directly passed to the memcpy function without any prior check for nullptr, which can lead to undefined behavior.
One may argue that this parameter also passes size—it's the number of bytes to be copied—so, if this parameter is 0, there will be no copying.
Unfortunately, that's not quite the case. If we read the memcry documentation, we'll understand that we can't pass invalid pointers to it, even if the number of the copied data is equal to zero:
"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);
}
}The PVS-Studio warning:
V595 The 'asset' pointer was utilized before it was verified against nullptr. Check lines: 2706, 2709. Variant.cpp: 2706, 2709.
The code is rather strange. It seems that the first if statement should handle the old AsAsset value before it is replaced with a new one, as indicated by the if statement.
The second if statement, which checks the asset parameter for nullptr, also indicates an error in this place. If we expect that asset can be equal to nullptr, then dereferencing inside the first if can lead to undefined behavior.
The most logical fix here is to replace asset with AsAsset inside the first 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))))); // <=
....
}The PVS-Studio warning:
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.
The analyzer has detected a potential issue with the bitwise shift in the uni >> (6 * (count - 1)) expression, which can lead to unexpected behavior. This may happen because uni is of type int32. The right value shift to 32 bit and more may cause undefined behavior.
The analyzer has detected that the largest possible shift during bitwise shifting the uni variable is 36. How did it detect that? Note the ternary operator used to initialize the count variable:
const uint32 count = uni <= 0x7FU ? 1
: uni <= 0x7FFU ? 2
: uni <= 0xFFFFU ? 3
: uni <= 0x1FFFFFU ? 4
: uni <= 0x3FFFFFFU ? 5
: uni <= 0x7FFFFFFFU ? 6
: 7;We can see that the maximum value that can be assigned to a variable is 7. Now let's substitute this value into the expression representing the shift:
6 * (count - 1) = 6 * (7 – 1) = 36Well, the analyzer was right here. Case closed!
This article wraps up here, hope you enjoyed it :)
That's not the first game engine we've checked. If you're interested in learning more, I've included links to some related articles below.
Here's a list of articles analyzing the C++-based engines:
Here's a list of articles analyzing the C#-based engines:
I hope you found the tool piqued your interest—the one that helped us throughout this article to search for issues in extensive code base of the engine.
I invite you to give it a try! Just request a trial license on our website. Evaluate all the PVS-Studio features for free during the trial period.
See you in future articles!
0
0
0
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
30
0
0