“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.
Intro
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? What is it?
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!
About Flax Engine
Dear readers, meet Flax Engine:
What makes Flax stand out among other engines?
Support for scripting in both C++ and C#. Developers can also leverage visual programming.
Support for .NET 8 and C# 12, so the latest language features are available, resulting in performance boost through vast runtime enhancements. If you’d like to revise these .NET features, you can check out our article. By the way, at the time of my writing, Unity still doesn’t support new versions.
If you may think that the engine is more C#-oriented than C++, but that’s not the case. The engine core is written in C++, which allows C++ developers to use its API directly. This enhances the performance and lets devs use some other APIs.
Considering it’s an open-source engine, it offers a very robust toolkit. You can find the list of tools on a separate page.
You can fully use the engine for free as long as the project revenue doesn’t exceed $250,000 per three months. If the revenue is higher, the engine cost will be equal to 4% of the profit you make from your Flax-driven projects.
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!
Errors in Flax Engine C# code
Redundant condition checks
Case 1
{
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:
Here’s another similar case found in the project:
Case 2
{
….
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.
Error due to careless copy-paste
{
….
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.
Array overrun
{
….
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.
Unreachable code
Case 1
{
….
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:
Case 2
{
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:
{
// 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.
Issue with thread synchronization due to compiler optimizations
….
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.
Errors in Flax Engine C++ code
Possible nullptr dereference
Case 1
{
….
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:
{
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
{
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:
{
….
if (AsAsset)
{
AsAsset ->OnUnloaded.Unbind<Variant, // <=
&Variant::OnAssetUnloaded>(this);
AsAsset ->RemoveReference(); // <=
}
AsAsset = asset;
if (asset)
{
asset->AddReference();
asset->OnUnloaded.Bind<Variant, &Variant::OnAssetUnloaded>(this);
}
}
Possible bitwise shift error
{
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:
: 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:
Well, the analyzer was right here. Case closed!
Conclusion
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:
Off we go! Digging into the game engine of War Thunder and interviewing its devs
Let’s check the qdEngine game engine, part one: top 10 warnings issued by PVS-Studio
Let’s check the qdEngine game engine, part two: simplifying C++ code
Let’s check the qdEngine game engine, part three: 10 more bugs
Heroes of Code and Magic: VCMI game engine analysis
Here’s a list of articles analyzing the C#-based engines:
PVS-Studio static analyzer to recheck Unity
Stride Game Engine error review
Playing with null: Checking MonoGame with the PVS-Studio analyzer
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!
