On March 19, 2014, Unreal Engine 4 was made publicly available. Subscription costs only $19 per month. The source codes have also been published at the github repository. Since that moment, we have received quite a number of e-mails, twitter messages, etc., people asking to check this game engine. So we are fulfilling our readers' request in this article; let's see what interesting bugs the PVS-Studio static code analyzer has found in the project's source code.

Unreal Engine

The Unreal Engine is a game engine developed by Epic Games, first illustrated in the 1998 first-person shooter game Unreal. Although primarily developed for the first-person shooters, it has been successfully used in a variety of other genres, including stealth, MMORPGs, and other RPGs. With its code written in C++, Unreal Engine features a high degree of portability and is a tool used by many game developers today.

Analysis methodology for an nmake-based project

There exist certain difficulties regarding analysis of the Unreal Engine project. To check it, we had to use a new feature recently introduced in PVS-Studio Standalone. Because of that, we had to postpone the publication of this article a bit so that it would follow the release of the new PVS-Studio version with this feature. I guess many would like to try it: it allows programmers to easily check projects that make use of complex or non-standard build systems.

PVS-Studio's original working principle is as follows:

You open a project in Visual Studio.

Click the "Start" button.

The Visual Studio-integrated plugin collects all the necessary information: which files need to be analyzed, which macros are to be expanded, where the header files location, and so on.

The plugin launches the analyzer module itself and outputs the analysis results.

What's special about Unreal Engine 4 is that it is an nmake-based project, therefore it can't be checked by the PVS-Studio plugin.

Let me explain this point. Unreal Engine is implemented as a Visual Studio project, but the build is done with nmake. It means that the plugin cannot know which files are compiled with which switches. Therefore, analysis is impossible. To be exact, it is possible, but it will be somewhat of an effort (see the documentation section, "Direct integration of the analyzer into build automation systems").

And here's PVS-Studio Standalone coming to help! It can work in two modes:

You obtain preprocessed files in any way and let the tool check them.

Its monitoring compiler calls and get all the necessary information.

It is the second mode that we are interested in now. This is how the check of Unreal Engine was done:

We launched PVS-Studio Standalone.

Clicked "Compiler Monitoring".

Then we clicked "Start Monitoring" and made sure the compiler call monitoring mode was on.

We opened the Unreal Engine project in Visual Studio and started the project build. The monitoring window indicated that the compiler calls were being tapped.

When the build was finished, we clicked Stop Monitoring, and after that the PVS-Studio analyzer was launched.

The diagnostic messages were displayed in the PVS-Studio Standalone window.

Hint. It is more convenient to use Visual Studio instead of the PVS-Studio Standalone's editor to work with the analysis report. You only need to save the results into a log file and then open it in the Visual Studio environment (Menu->PVS-Studio->Open/Save->Open Analysis Report).

The project authors may also use some other analyzers, but I can't say for sure.

So their code is pretty good. Since they use static code analysis tools during the development, PVS-Studio has not found many suspicious fragments. However, just like any other large project, this one does have some bugs, and PVS-Studio can catch some of them. So let's find out what it has to show us.

Structure members are initialized here. A typo causes the NotUsed6 member to be initialized twice, while the NotUsed7 member remains uninitialized. However, the _DEPRECATED() suffix in the function name tells us this code is not of much interest anymore.

Here are two other fragments where one variable is assigned a value twice:

Null pointers

I pretty often come across null pointer dereferencing errors in error handlers. No wonder: these fragments are difficult and uninteresting to test. In Unreal Engine, you can find a null pointer dereferencing error in an error handler too:

We want to print the object name when an error occurs. But the object doesn't exist.

Here's another fragment with null pointer dereferencing. It's all much more interesting here. Perhaps the error appeared because of an incorrect merge. Anyway, the comment proves that the code is incomplete:

Notice the comment. The global variable GEngine may be equal to zero, so it must be checked before it can be used.

And there is such a check indeed in the function LoadMap():

if( GEngine )

Unfortunately, this check is executed only after the pointer has been already used:

if (GEngine->GameViewport != NULL)

There were quite a number of V595 warnings for the project (about 82). I guess many of them are false positives, so I won't litter the article with the samples and cite them in a separate list: ue-v595.txt.

Excess variable declaration

This error is pretty nice. It is about mistakenly declaring a new variable instead of using an already existing one.

The analyzer was surprised at finding the Memcmp() function's result not being used anywhere. And this is an error indeed. As far as I get it, the programmer wanted to copy the data, not compare them. If so, the Memcpy() function should be used:

The ResolveParams.CubeFace variable is of the ECubeFace type, and it is cast explicitly to the ECubeFace type, i.e. nothing happens. After that, the variable is assigned to itself. Something is wrong with this code.

PVS-Studio's diagnostic message: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. particlemodules_location.cpp 2120

It's not that easy to spot it. I'm sure you've just scanned through the code and haven't noticed anything strange. The analyzer warning, unfortunately, is also strange and suggests a false positive. But in fact, we are dealing with a real and very interesting bug.

Let's figure it all out. Notice that the last argument of the VertInfluencedByActiveBone() function is optional.

In this code fragment, the VertInfluencedByActiveBone() function is called 3 times. The first two times, it receives 4 arguments; with the last call, only 3 arguments. And here is where the error is lurking.

It is only from pure luck that the code compiles well, the error staying unnoticed. This is how it happens:

The function is called with 3 arguments: VertInfluencedByActiveBone(Owner, SourceComponent, VertIndex[2]);

The '!' operator is applied to the function result;

The !VertInfluencedByActiveBone(...) expression evaluates to a bool value;

The '&' (bitwise AND) operator is applied to it;

All this is compiled successfully because there is a bool expression to the left of the '&' operator and an integer variable BoneIndex3 to the right.

The analyzer suspected something was wrong on discovering one of the '&' operator's arguments to have the bool type. And that was what it warned us about - not in vain.

To fix the error, we need to add a comma and put a closing parenthesis in the right place:

The break; operator is missing in the very beginning. I guess no comments and explanations are needed.

Microoptimizations

The PVS-Studio analyzer offers a small set of diagnostic rules that help carry out microoptimizations of the code. Though small, they may prove pretty useful at times. Let's take one assignment operator as an example:

PVS-Studio's diagnostic message: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. materialshared.h 224

This code is pretty complicated. To make the explanation clearer, I have composed a simplified artificial sample:

return A() + B() + C() + uniform ? UniformSize() : 0;

A certain size is being calculated in this code. Depending on the value of the uniform variable, either UniformSize() or 0 should be added. But the code actually works in quite a different way. The priority of the addition operators '+' is higher than that of the '?:' operator.

So here's what we get:

return (A() + B() + C() + uniform) ? UniformSize() : 0;

A similar issue can be found in Unreal Engine's code. I suspect the program calculates something different than the programmer wanted it to.

Mess-up with enum

I didn't feel like describing this case at first as I would have to cite quite a large piece of code. But then I overcame my laziness, so please be patient too.

The analyzer generates a few V556 warnings at once on this code. The reason is that the switch operator has a variable of the EOnlineSharingReadCategory::Type type as its argument. At the same time, case operators work with values of a different type, EOnlineSharingPublishingCategory::Type.

The programmer intended to skip all text in double quotes. The algorithm was meant to be like this:

Once the program comes across a double quote, a loop is started.

The loop keeps skipping characters until stumbling across the next double quote.

The error is about the pointer failing to be referenced to the next character after the first double quote is found. As a result, the second double quote is found right away, too, and the loop doesn't start.

Whether or not this code contains an error depends on whether the value 1 needs to be shifted by more than 31 bits. Since the result is saved into a 64-bit variable PoolMask, it seems highly probable.

If I am right, the library contains an error in the memory allocation subsystem.

The number 1 is of the int type, which means that you cannot shift it by 35 bits, for example. Theoretically, it leads to undefined behavior (find out more). In practice, an overflow will occur and an incorrect value will be computed.

The fixed code looks as follows:

PoolMask = ( ( 1ull << ( HashKeyShift - PoolBitShift ) ) - 1 );

Obsolete checks

PVS-Studio's diagnostic message: V668 There is no sense in testing the 'pSensorFusion' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. oculusrifthmd.cpp 1594

For a long time now the new operator has been throwing an exception in case of a memory allocation error. The if (!pSensorFusion) check is not needed.

I usually find quite a lot of such fragments in large projects, but Unreal Engine's code contains surprisingly few of them: ue-V668.txt.

Copy-Paste

The code fragments below have most likely appeared through the Copy-Paste method. Regardless of the condition, one and the same code branch is executed:

PVS-Studio's diagnostic message: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] VersionInfo;'. windowsplatformexceptionhandling.cpp 107

Conclusions

Using the static analyzer integrated into Visual Studio does make sense but it is not enough. The authors should consider using specialized tools in addition to it, for example our analyzer PVS-Studio. If you compare PVS-Studio to VS2013's analyzer, the former detects 6 times more bugs. Here you have the proof:

I invite all those who want their code to be high-quality to try our code analyzer.

P.S. I should also mention that the errors described in this article (except for microoptimizations) could theoretically have been found by the lightweight analyzer CppCat as well. A one-year license for CppCat costs $250; annual renewal costs $200. But it wouldn't do in this particular case because it is lightweight and lacks the necessary functionality to monitoring compiler launches, which is a crucial requirement when checking Unreal Engine. However, the CppCat analyzer may well satisfy the authors of small projects.

The last line, as you mentioned, should use a + not a - operator. But also, it should use a <= not a >= operator. Changing one without fixing the other would make your PVS-Studio error message go away but would still result in incorrect results.

Perhaps the best thing here would be to add some test cases to test the actual results of the function (in addition to static code analysis).

Really, kudos that your static code analyzer caught that "The nicest of all the errors". I glanced over it multiple times, and only noticed the "missing" closing parenthesis. The fact that that typo happened to compile is very unfortunate - I would never catch that because it visually looks perfect and compiles fine.

I'm also impressed that you can detect the "Mess-up with enum" bug and some of the others.

Sorry for being a nitpick in case the answer is an obvious yes, but since it's not expressly mentioned in the article: do you have permission from Epic to reproduce the UE4 code snippets in public?

Of Course it's open source!

No it is not.

From Epic games website:

"

Can I share the Unreal Engine source code or tools with others?

You can share the source code or tools, along with any modifications you’ve made, with anyone who is an Unreal Engine licensee who is authorized to access the same version of the engine as yours, e.g. the 4.x.x version number of your installed build."

I think pointing out these errors is in somewhat poor taste. Add in that it was done to further their own goals makes it worse.

How so? Real world examples are the best kind of examples. Epic even get some of their bugs fixed for free, everyone wins. Still, it highlights as others have said to have SA as part of the development arsenal.

I am a big fan of static code analysis and routinely use such tools in my indie projects. This is not the first PVS-Studio advertisement for gamedev communities, and in general the takeaway message "there are bugs even in UE4 codebase that PVS-Studio static analysis can catch" is nice and convinces that the tool is good, but not quite enough to lead to a buying decision.

For people interested in C/C++ static analysis tools, there are several that are free:

VS2012&VS2013 /analyze option (that was mentioned in this article). Feels like the weakest static analyzer out there, but understandable, as it is the youngest analyzer project in development.

cppcheck - in my experience finds much more issues than /ANALYZE, but also some false positives.

Given that these tools are free, and PVS-Studio is a commercial one, I would like to see for an article like this to compare how PVS-Studio performs against these free tools on real-world codebases like UE4? If I am already running routinely with the three above tools, is there any worth in paying for PVS-Studio?

As a second question, there are also the following excellent free runtime analyzer tools:

Is PVS-Studio capable of surpassing the combined strength of these free static+dynamic analyzers?

And finally, I find odd is that the price of the tool is "please write us to ask for a quote", and is passed out in one-year annual licenses. This does not look like a pricing model specifically targeted towards indie developers like the GameDev.net community is, but more like a strategy towards larger business-to-business clients. Even after reading an excellent article like this at GameDev.net, the pricing model really puts me off, especially since I am not sure how well it would augment the above existing free tools in my toolbox. Nevertheless, this was an interesting read, thanks for the writeup!

I'm disappointed this article is not actually about unreal engine (as other have already pointed out it's really just marketing material for PVS-Studio)

The marketing material is interesting and reads nicely, but it would IMO have been more trustworthy, with a name/ description/ introduction that identified this article as a use case for PVS-Studio on large projects. (I guess what I'm trying to say is I would have preferred the article not to pass itself off for something it isn't)