# Flax Engine\. Exploring game engine & analyzing its source code "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\. ![1153_FlaxEngine/image1.png](https://import.viva64.com/docx/blog/1153_FlaxEngine/image1.png) ## 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: ![1153_FlaxEngine/image2.png](https://import.viva64.com/docx/blog/1153_FlaxEngine/image2.png) Want to know more about the analyzer? Check out our [website](https://pvs-studio.com/en/)\. Now let's get back to the engine\! ## About Flax Engine Dear readers, meet Flax Engine: ![1153_FlaxEngine/image3.png](https://import.viva64.com/docx/blog/1153_FlaxEngine/image3.png) 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](https://pvs-studio.com/en/blog/posts/csharp/1080/)\. 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](https://flaxengine.com/features/)\. * 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](https://pvs-studio.com/en/) 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](https://github.com/FlaxEngine/FlaxEngine/releases/tag/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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'typeNamespaces\.Length \!\= 0' to the left and to the right of the '&&' operator\. AssetItem\.cs: [83](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Editor/Content/Items/AssetItem.cs#L83)\. 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: ```cpp typeNamespaces[typeNamespaces.Length - 1].Length != 0 ``` Here's another similar case found in the project: **Case 2** ```cpp public override bool OnMouseDown(Float2 location, MouseButton button) { .... if (_rightMouseDown || (_middleMouseDown && _middleMouseDown)) // <= { // Start navigating StartMouseCapture(); Focus(); return true; } .... } ``` The PVS\-Studio warning: [V3001](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions '\_middleMouseDown' to the left and to the right of the '&&' operator\. VisjectSurface\.Input\.cs: [495](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Editor/Surface/VisjectSurface.Input.cs#L495)\. ### Error due to careless copy\-paste ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/UI/GUI/WindowRootControl.cs#L70), [78](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/UI/GUI/WindowRootControl.cs#L78)\. 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 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3106/) Possibly index is out of bound\. The '4' index is pointing beyond 'values' bound\. Matrix2x2\.cs: [98](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Core/Math/Matrix2x2.cs#L98)\. 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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Editor/Utilities/MemberComparison.cs#L78), [82](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Editor/Utilities/MemberComparison.cs#L82)\. 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: ```cpp type.Type == typeof(ushort) ``` **Case 2** ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression '\_dragOverMode \=\= DragItemPositioning\.None' is always false\. TreeNode\.cs: [566](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Editor/GUI/Tree/TreeNode.cs#L566)\. 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: ```cpp 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\. ### Issue with thread synchronization due to compiler optimizations ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Editor/GUI/Dialogs/Dialog.cs#L108)\. 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](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile?redirectedfrom=MSDN)\. ## Errors in Flax Engine C\+\+ code ### Possible nullptr dereference **Case 1** ```cpp 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](https://pvs-studio.com/en/docs/warnings/v595/) The 'prev' pointer was utilized before it was verified against nullptr\. Check lines: 'PlatformBase\.h: [178](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Platform/Base/PlatformBase.h#L178)', 'DataContainer\.h: [339](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Core/Types/DataContainer.h#L339)', 'DataContainer\.h: [342](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Core/Types/DataContainer.h#L342)'\. DataContainer\.h [339](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Core/Types/DataContainer.h#L339)\. 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: ```cpp FORCE_INLINE static void MemoryCopy(void* dst, const void* src, uint64 size) { memcpy(dst, src, static_cast(size)); } ``` We can now see that the second parameter value is directly passed to the [_memcpy_](https://en.cppreference.com/w/cpp/string/byte/memcpy) function without any prior check for _nullptr_, which can lead to [undefined behavior](https://pvs-studio.com/en/blog/terms/0066/#:~:text=Неопределённое%20поведение%20(англ.%20undefined%20behaviour),результат%20операции%20Б%20не%20определён)\. 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** ```cpp void Variant::SetAsset(Asset* asset) { if (Type.Type != VariantType::Asset) SetType(VariantType(VariantType::Asset)); if (AsAsset) { asset->OnUnloaded.Unbind(this); asset->RemoveReference(); // <= } AsAsset = asset; if (asset) { asset->AddReference(); asset->OnUnloaded.Bind(this); } } ``` The PVS\-Studio warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'asset' pointer was utilized before it was verified against nullptr\. Check lines: 2706, 2709\. Variant\.cpp: [2706](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Core/Types/Variant.cpp#L2706), [2709](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Core/Types/Variant.cpp#L2709)\. 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](https://pvs-studio.com/en/blog/terms/0066/#:~:text=Неопределённое%20поведение%20(англ.%20undefined%20behaviour),результат%20операции%20Б%20не%20определён)\. The most logical fix here is to replace _asset_ with _AsAsset_ inside the first _if_: ```cpp void Variant::SetAsset(Asset* asset) { .... if (AsAsset) { AsAsset ->OnUnloaded.Unbind(this); AsAsset ->RemoveReference(); // <= } AsAsset = asset; if (asset) { asset->AddReference(); asset->OnUnloaded.Bind(this); } } ``` ### Possible bitwise shift error ```cpp void StringUtils::ConvertUTF162UTF8(....) { Array 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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/FlaxEngine/FlaxEngine/blob/009e1aaebba30f4ee7198a1e8212de91f59386bf/Source/Engine/Platform/Base/StringUtilsBase.cpp#L253)\. 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](https://pvs-studio.com/en/blog/terms/0066/#:~:text=Неопределённое%20поведение%20(англ.%20undefined%20behaviour),результат%20операции%20Б%20не%20определён)\. 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: ```cpp 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: ```cpp 6 * (count - 1) = 6 * (7 – 1) = 36 ``` 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](https://pvs-studio.com/en/blog/posts/cpp/1098/) * [Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS\-Studio](https://pvs-studio.com/en/blog/posts/cpp/1119/) * [Let's check the qdEngine game engine, part two: simplifying C\+\+ code](https://pvs-studio.com/en/blog/posts/cpp/1121/) * [Let's check the qdEngine game engine, part three: 10 more bugs](https://pvs-studio.com/en/blog/posts/cpp/1123/) * [Heroes of Code and Magic: VCMI game engine analysis](https://pvs-studio.com/en/blog/posts/cpp/1058/) Here's a list of articles analyzing the C\#\-based engines: * [PVS\-Studio static analyzer to recheck Unity](https://pvs-studio.com/en/blog/posts/csharp/0932/) * [Stride Game Engine error review](https://pvs-studio.com/en/blog/posts/csharp/0994/) * [Playing with null: Checking MonoGame with the PVS\-Studio analyzer](https://pvs-studio.com/en/blog/posts/csharp/0915/) 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](https://pvs-studio.com/en/pvs-studio/try-free/)\. Evaluate all the PVS\-Studio features for free during the trial period\. See you in future articles\!