Long-awaited check of Unreal Engine 4

March 19, 2014 Unreal Engine 4 became available to all comers. Subscription price is only $ 19 per month. Source codes are also posted on the github repository. From this moment we received a lot of messages to the mail, Twitter and so on, with a request to check this game engine. We satisfy the request of our readers. Let's see what interesting can be found in the source code using the PVS-Studio static code analyzer.


')

Unreal engine

Unreal Engine is a game engine developed and supported by Epic Games. Written in C ++. It allows you to create games for most operating systems and platforms: Microsoft Windows, Linux, Mac OS and Mac OS X, Xbox, Xbox 360, PlayStation 2, PlayStation Portable, PlayStation 3, Wii, Dreamcast and Nintendo GameCube consoles.

Official website: https://www.unrealengine.com/

Description on Wikipedia: Unreal Engine .

The way to check nmake-based project

With the verification of the project Unreal Engine is not so simple. To test this project, we had to take advantage of a new opportunity that we recently implemented in PVS-Studio Standalone. Because of this, we had to slightly delay the publication of the article. We decided to publish it after the release of PVS-Studio, where this new feature will already be. Perhaps many will want to try it. It greatly facilitates the verification of projects that use complex or non-standard assembly systems.

The classic working principle of PVS-Studio is:

• You open a project in Visual Studio.
• Press the "check" button.
• The plug-in integrated into Visual Studio collects all the necessary information: which files to check, which macros to use, where are the header files, and so on.
• The plugin launches the analyzer itself and displays the results obtained.

The caveat is that Unreal Engine 4 is a nmake-based project, so you cannot check it with the PVS-Studio plugin.

I will explain. There is a project for the Visual Studio environment. But the build is done using nmake. This means that the plugin does not know which files are compiled with which keys. Accordingly, analysis is not possible. Rather, analysis is possible, but the process is time consuming (see the documentation section: " Direct integration of the analyzer into assembly automation systems ").

And here PVS-Studio Standalone comes to the rescue! He can work in two versions:

1. You somehow generate preprocessed files, and he will check them already.
2. It keeps track of compiler launches and "spies" all the necessary information.

Now we are interested in exactly the second usage scenario. This is how the Unreal Engine check took place:

1. We launched PVS-Studio Standalone.
2. Clicked the “Compiler Monitoring” button.
3. Clicked on the "Start Monitoring" button. We saw that the monitoring mode for the compiler calls was turned on.
4. Opened the Unreal Engine project in Visual Studio. Launched the build project. At the same time, the monitoring window shows that the compiler calls are intercepted.
5. When the build in Visual Studio ended, we clicked the Stop Monitoring button. After that, the PVS-Studio analyzer started working.

As a result, diagnostic messages appear in the PVS-Studio Standalone utility window.

Hint. For convenience, it is better to use Visual Studio, rather than the editor built into PVS-Studio Standalone. It is enough to save the analysis results to a file, and then open it from the Visual Studio environment (Menu-> PVS-Studio-> Open / Save-> Open Analysis Report).

All this and much more is described in the article " PVS-Studio now supports any build system on Windows and any compiler. Easy and out of the box . Please read this article before you start experimenting with PVS-Studio Standalone!

Test results

The code of the Unreal Engine project seemed to me very high quality. For example, developers use static code analysis when developing. This is indicated by such fragments in the code as:

// Suppress static code analysis warning about a // potential comparison of two constants CA_SUPPRESS(6326); .... // Suppress static code analysis warnings about a // potentially ill-defined loop. BlendCount > 0 is valid. CA_SUPPRESS(6294) .... #if USING_CODE_ANALYSIS 

Judging by these code fragments, a static code analyzer built into Visual Studio is used. About this code analyzer: Visual Studio 2013 Static Code Analysis in depth: What? When and How?

Perhaps other analyzers are used, but I know nothing about it.

So, the code is written very well. When working using the tools of static code analysis. Therefore, having checked the project using PVS-Studio, we found few suspicious code fragments. However, as in any large project, there are errors in it. And some of them are able to find PVS-Studio. Let's see what we managed to notice suspicious.

Typos

 static bool PositionIsInside(....) { return Position.X >= Control.Center.X - BoxSize.X * 0.5f && Position.X <= Control.Center.X + BoxSize.X * 0.5f && Position.Y >= Control.Center.Y - BoxSize.Y * 0.5f && Position.Y >= Control.Center.Y - BoxSize.Y * 0.5f; } 

PVS-Studio warning: V501 There are identical sub-expressions 'Position.Y> = Control.Center.Y - BoxSize.Y * 0.5f' the operator. svirtualjoystick.cpp 97

Please note that the variable “Position.Y” is twice compared with the expression “Control.Center.Y - BoxSize.Y * 0.5f”. This is clearly a typo. In the last line, replace the '-' operator with the '+' operator.

Another similar typo in the condition:

 void FOculusRiftHMD::PreRenderView_RenderThread( FSceneView& View) { .... if (View.StereoPass == eSSP_LEFT_EYE || View.StereoPass == eSSP_LEFT_EYE) .... } 

PVS-Studio Warning: V501 There are identical sub-expressions 'View.StereoPass == eSSP_LEFT_EYE' to the left | operator. oculusrifthmd.cpp 1453

Apparently, the work with Oculus Rift is not very tested yet.

We continue.

 struct FMemoryAllocationStats_DEPRECATED { .... SIZE_T NotUsed5; SIZE_T NotUsed6; SIZE_T NotUsed7; SIZE_T NotUsed8; .... }; FMemoryAllocationStats_DEPRECATED() { .... NotUsed5 = 0; NotUsed6 = 0; NotUsed6 = 0; NotUsed8 = 0; .... } 

PVS-Studio warning: V519 The 'NotUsed6' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 86, 88. memorybase.h 88

The members of the structure are initialized. Due to a typo, the member 'NotUsed6' is initialized twice. And the member 'NotUsed7' remained uninitialized. However, the _DEPRECATED () suffix in the function name says that this is already an uninteresting code.

Another couple of places where one variable is assigned a value twice:

• V519 The 'HighlightText' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 204, 206. srichtextblock.cpp 206
• V519 The 'TrackError.MaxErrorInScaleDueToScale' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1715, 1716. animationutils.cpp 1716

Null pointers

Very often I find dereferencing the null pointer in error handlers. No wonder. Such fragments are difficult and not interesting to test. You can meet the dereferencing of the null pointer in the error handler in the Unreal Engine project:

 bool UEngine::CommitMapChange( FWorldContext &Context ) { .... LevelStreamingObject = Context.World()->StreamingLevels[j]; if (LevelStreamingObject != NULL) { .... } else { check(LevelStreamingObject); UE_LOG(LogStreaming, Log, TEXT("Unable to handle streaming object %s"), *LevelStreamingObject->GetName()); } .... } 

PVS-Studio warning: V522 Dereferencing of the null pointer 'LevelStreamingObject' might take place. unrealengine.cpp 10768

If an error occurred, then I want to print the name of the object. That just does not exist this object.

Consider another fragment where the null pointer will be dereferenced. Everything is more interesting here. Perhaps the error is present due to improper merge. In any case, the comment shows that the code is not completed:

 void FStreamingPause::Init() { .... if( GStreamingPauseBackground == NULL && GUseStreamingPause ) { // @todo UE4 merge andrew // GStreamingPauseBackground = new FFrontBufferTexture(....); GStreamingPauseBackground->InitRHI(); } } 

PVS-Studio warning: V522 Dereferencing of the null pointer 'GStreamingPauseBackground' might take place. streamingpauserendering.cpp 197

More about null pointers

In almost any program, I meet a lot of warnings with code V595 ( examples ). These warnings indicate the following situation:

At first, the pointer is dereferenced. Below this pointer is checked for equality to zero. This is not always a mistake. But this is a very suspicious code, and it must be checked!

Diagnostics V595 allows you to detect such bloopers:

 /** * Global engine pointer. * Can be 0 so don't use without checking. */ ENGINE_API UEngine* GEngine = NULL; bool UEngine::LoadMap( FWorldContext& WorldContext, FURL URL, class UPendingNetGame* Pending, FString& Error ) { .... if (GEngine->GameViewport != NULL) { ClearDebugDisplayProperties(); } if( GEngine ) { GEngine->WorldDestroyed( WorldContext.World() ); } .... } 

PVS-Studio warning: V595 The 'GEngine' pointer was used before it was verified against nullptr. Check lines: 9714, 9719. unrealengine.cpp 9714

Pay attention to the comment. The global variable GEngine may be zero. Before you use it, you need to check it.

In the LoadMap () function, there really is such a check:

 if( GEngine ) 

But that's bad luck. The check is located after the pointer has been used:

 if (GEngine->GameViewport != NULL) 

Warnings V595 quite a lot (82 pieces). I think many of them will be false. Therefore, I will not litter the article with messages and give them a separate list: ue-v595.txt .

Unnecessary variable declaration

Consider a beautiful bug. It is connected with the fact that a new variable is created by chance, and the old one is not used.

 void FStreamableManager::AsyncLoadCallback(....) { .... FStreamable* Existing = StreamableItems.FindRef(TargetName); .... if (!Existing) { // hmm, maybe it was redirected by a consolidate TargetName = ResolveRedirects(TargetName); FStreamable* Existing = StreamableItems.FindRef(TargetName); } if (Existing && Existing->bAsyncLoadRequestOutstanding) .... } 

PVS-Studio Warning: V561 It's not better to assign value to 'Existing' variable than to declare it anew. Previous declaration: streamablemanager.cpp, line 325. streamablemanager.cpp 332

As I understand it, in fact, should be written like this:

 // hmm, maybe it was redirected by a consolidate TargetName = ResolveRedirects(TargetName); Existing = StreamableItems.FindRef(TargetName); 

Errors when calling functions

 bool FRecastQueryFilter::IsEqual( const INavigationQueryFilterInterface* Other) const { // @NOTE: not type safe, should be changed when // another filter type is introduced return FMemory::Memcmp(this, Other, sizeof(this)) == 0; } 

PVS-Studio warning: V579. It is possibly a mistake. Inspect the third argument. pimplrecastnavmesh.cpp 172

A comment warns that using Memcmp () is dangerous. But, in fact, it is still worse than the programmer thinks. The point is that the function compares only part of the object.

The sizeof (this) operator returns the size of the pointer. That is, in a 32-bit program, the function will compare the first 4 bytes. In a 64-bit program, 8 bytes will be compared.

The correct code is:

 return FMemory::Memcmp(this, Other, sizeof(*this)) == 0; 

The misadventures with the Memcmp () function do not end there. Having reviewed the following code snippet:

 D3D11_STATE_CACHE_INLINE void GetBlendState( ID3D11BlendState** BlendState, float BlendFactor[4], uint32* SampleMask) { .... FMemory::Memcmp(BlendFactor, CurrentBlendFactor, sizeof(CurrentBlendFactor)); .... } 

PVS-Studio warning: V530 The return value of the function 'Memcmp' is required to be utilized. d3d11statecachepriprivate.h 547

The analyzer was surprised to see that the result of the Memcmp () function is not used at all. And indeed, this is a mistake. As I understand it, they wanted to copy here, not compare the data. Then you need to use the Memcpy () function:

 FMemory::Memcpy(BlendFactor, CurrentBlendFactor, sizeof(CurrentBlendFactor)); 

The variable is assigned to itself.

 enum ECubeFace; ECubeFace CubeFace; friend FArchive& operator<<( FArchive& Ar,FResolveParams& ResolveParams) { .... if(Ar.IsLoading()) { ResolveParams.CubeFace = (ECubeFace)ResolveParams.CubeFace; } .... } 

PVS-Studio warning: V570 The 'ResolveParams.CubeFace' variable is assigned to itself. rhi.h 1279

The variable 'ResolveParams.CubeFace' is of type ECubeFace. Used to explicitly cast the variable to the ECubeFace type. That is, nothing happens. Then the variable is assigned to itself. There is something wrong with this code.

The most beautiful error found

Most of all, I liked this error:

 bool VertInfluencedByActiveBone( FParticleEmitterInstance* Owner, USkeletalMeshComponent* InSkelMeshComponent, int32 InVertexIndex, int32* OutBoneIndex = NULL); void UParticleModuleLocationSkelVertSurface::Spawn(....) { .... int32 BoneIndex1, BoneIndex2, BoneIndex3; BoneIndex1 = BoneIndex2 = BoneIndex3 = INDEX_NONE; if(!VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[0], &BoneIndex1) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[1], &BoneIndex2) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[2]) &BoneIndex3) { .... } 

PVS-Studio warning: V564 The '&' operator is applied to bool type value. You've probably forgotten to include the operator. particlemodules_location.cpp 2120

Notice the error is not easy. I am sure that you looked through the code fluently and did not see anything suspicious in it. The analyzer warning, unfortunately, is also strange and seems like a false positive. Meanwhile, we are dealing with a very interesting error.

Let's figure it out. Note that the last argument to the VertInfluencedByActiveBone () function is optional.

In the code, the VertInfluencedByActiveBone () function is called 3 times. Two times she is given 4 arguments. In the latter case, there are only 3 arguments. This is the error.

Thanks to luck, the code is compiled, and the error is unnoticeable. This is what happens here:

1. A function is called with 3 arguments: “VertInfluencedByActiveBone (Owner, SourceComponent, VertIndex [2])”;
2. The result of the function applies the operator '!';
3. The result of the expression "! VertInfluencedByActiveBone (...)" is of type bool;
4. The result is the operator '&' (bitwise AND);
5. Everything compiles successfully, since to the left of the '&' operator is an expression of the bool type. To the right of '&' is the integer variable BoneIndex3.

The analyzer suspected something was wrong when he saw that one of the arguments of the '&' operator is of the 'bool' type. He reported about it. And for good reason.

To correct the error, add a comma and place the closing bracket in a different place:

 if(!VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[0], &BoneIndex1) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[1], &BoneIndex2) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[2], &BoneIndex3)) 

Break operator forgotten

 static void VerifyUniformLayout(....) { .... switch(Member.GetBaseType()) { case UBMT_STRUCT: BaseTypeName = TEXT("struct"); case UBMT_BOOL: BaseTypeName = TEXT("bool"); break; case UBMT_INT32: BaseTypeName = TEXT("int"); break; case UBMT_UINT32: BaseTypeName = TEXT("uint"); break; case UBMT_FLOAT32: BaseTypeName = TEXT("float"); break; default: UE_LOG(LogShaders, Fatal, TEXT("Unrecognized uniform ......")); }; .... } 

PVS-Studio warning: V519 The 'BaseTypeName' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 862, 863. openglshaders.cpp 863

At the very beginning, the “break;” operator is forgotten. I think comments and explanations are unnecessary.

Micro-optimizations

The PVS-Studio analyzer has a small set of diagnostic rules that help to make small optimizations in the code. However, sometimes they are very useful. Consider as an example one of the assignment operators:

 FVariant& operator=( const TArray<uint8> InArray ) { Type = EVariantTypes::ByteArray; Value = InArray; return *this; } 

PVS-Studio warning: V801 Decreased performance. It is better to redefine it. Consider replacing 'const ... InArray' with 'const ... & InArray'. variant.h 198

Not a good idea to pass an array by value. The array 'InArray' can and should be transmitted using a constant link.

The analyzer generates a lot of warnings related to micro-optimizations. Not all of them will be useful, but just in case I will give them a list: ue-v801-V803.txt .

Suspicious amount

 uint32 GetAllocatedSize() const { return UniformVectorExpressions.GetAllocatedSize() + UniformScalarExpressions.GetAllocatedSize() + Uniform2DTextureExpressions.GetAllocatedSize() + UniformCubeTextureExpressions.GetAllocatedSize() + ParameterCollections.GetAllocatedSize() + UniformBufferStruct ? (sizeof(FUniformBufferStruct) + UniformBufferStruct->GetMembers().GetAllocatedSize()) : 0; } 

PVS-Studio warning: V502 Perhaps the '?:' Operator works in a different way. The '?:' Operator has a lower limit than the '+' operator. materialshared.h 224

The code is very complicated. To clarify what is wrong, I will create a simplified artificial example:

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

Calculated a certain size. Depending on the value of the 'uniform' variable, add 'UniformSize ()' or 0. In fact, this code does not work that way. The priority of the operators of addition '+' is higher than the priority of the operator '?:'.

This is what happens:

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

We see a similar situation in the Unreal Engine code. It seems to me that it’s not what the programmer wanted.

Confusion with enum

At first I didn’t want to describe this case, since I need to give a fairly large piece of code. Then I still overcame laziness. Please be patient.

 namespace EOnlineSharingReadCategory { enum Type { None = 0x00, Posts = 0x01, Friends = 0x02, Mailbox = 0x04, OnlineStatus = 0x08, ProfileInfo = 0x10, LocationInfo = 0x20, Default = ProfileInfo|LocationInfo, }; } namespace EOnlineSharingPublishingCategory { enum Type { None = 0x00, Posts = 0x01, Friends = 0x02, AccountAdmin = 0x04, Events = 0x08, Default = None, }; inline const TCHAR* ToString (EOnlineSharingReadCategory::Type CategoryType) { switch (CategoryType) { case None: { return TEXT("Category undefined"); } case Posts: { return TEXT("Posts"); } case Friends: { return TEXT("Friends"); } case AccountAdmin: { return TEXT("Account Admin"); } .... } } 

The analyzer issues several V556 warnings here . The point is that the argument of the 'switch ()' operator is a variable of type EOnlineSharingReadCategory :: Type. In this case, the 'case' operators use values ​​from another type EOnlineSharingPublishingCategory :: Type.

Logical error

 const TCHAR* UStructProperty::ImportText_Internal(....) const { .... if (*Buffer == TCHAR('\"')) { while (*Buffer && *Buffer != TCHAR('\"') && *Buffer != TCHAR('\n') && *Buffer != TCHAR('\r')) { Buffer++; } if (*Buffer != TCHAR('\"')) .... } 

PVS-Studio Warning: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 310, 312. propertystruct.cpp 310

Here they wanted to miss everything that is contained in double quotes. The algorithm should have been like this:

• If we find a double quote, then we start the cycle.
• In the loop, skip the characters until we meet the next double quote.

The error is that after finding the first double quote, we do not move the pointer to the next character. As a result, the second quote is immediately located. The cycle does not start.

Let me explain this in simplified code:

 if (*p == '\"') { while (*p && *p != '\"') p++; } 

To correct the error, you must make the following changes:

 if (*p == '\"') { p++; while (*p && *p != '\"') p++; } 

Suspicious shift

 class FMallocBinned : public FMalloc { .... /* Used to mask off the bits that have been used to lookup the indirect table */ uint64 PoolMask; .... FMallocBinned(uint32 InPageSize, uint64 AddressLimit) { .... PoolMask = ( ( 1 << ( HashKeyShift - PoolBitShift ) ) - 1 ); .... } } 

PVS-Studio warning: V629 Consider inspecting the '1 << (HashKeyShift - PoolBitShift)' expression. Bit shifting of the 32-bit type. mallocbinned.h 800

Whether there is an error or not depends on whether you need to shift 1 by more than 31 bits. Since the result is placed in a 64-bit variable PoolMask, apparently there is such a need.

If I guessed, then the library has an error in the memory allocation subsystem.

The number 1 is of type int. This means that it is impossible to shift 1, say 35 digits. Formally, there will be an undefined behavior ( details ). In practice, an overflow will occur and an incorrect value will be calculated.

The correct code is:

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

Outdated checks

 void FOculusRiftHMD::Startup() { .... pSensorFusion = new SensorFusion(); if (!pSensorFusion) { UE_LOG(LogHMD, Warning, TEXT("Error creating Oculus sensor fusion.")); return; } .... } 

PVS-Studio warning: V668 has been set using the 'new' operator. The exception will be generated in the case of memory allocation error. oculusrifthmd.cpp 1594

For a long time, in the event of a memory allocation error, the 'new' operator throws an exception. The “if (! PSensorFusion)” check is not needed.

In large projects, I usually meet a lot of such places. Surprisingly, in Unreal Engine there are few such places. List: ue-V668.txt .

Copy paste

The following code snippets are most likely to be due to the use of the Copy-Paste technique. Regardless of the condition, the same action is performed:

 FString FPaths::CreateTempFilename(....) { .... const int32 PathLen = FCString::Strlen( Path ); if( PathLen > 0 && Path[ PathLen - 1 ] != TEXT('/') ) { UniqueFilename = FString::Printf( TEXT("%s/%s%s%s"), Path, Prefix, *FGuid::NewGuid().ToString(), Extension ); } else { UniqueFilename = FString::Printf( TEXT("%s/%s%s%s"), Path, Prefix, *FGuid::NewGuid().ToString(), Extension ); } .... } 

PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. paths.cpp 703

Another such code snippet:

 template< typename DefinitionType > FORCENOINLINE void Set(....) { .... if ( DefinitionPtr == NULL ) { WidgetStyleValues.Add( PropertyName, MakeShareable( new DefinitionType( InStyleDefintion ) ) ); } else { WidgetStyleValues.Add( PropertyName, MakeShareable( new DefinitionType( InStyleDefintion ) ) ); } } 

PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. slatestyle.h 289

miscellanea

There was a different trifle. Describing it is not interesting. Just give you code snippets and diagnostic messages.

 void FNativeClassHeaderGenerator::ExportProperties(....) { .... int32 NumByteProperties = 0; .... if (bIsByteProperty) { NumByteProperties; } .... } 

PVS-Studio warning: V607 Ownerless expression 'NumByteProperties'. codegenerator.cpp 633

 static void GetModuleVersion( .... ) { .... char* VersionInfo = new char[InfoSize]; .... delete VersionInfo; .... } 

PVS-Studio warning: V611, it was allocated using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] VersionInfo;'. windowsplatformexceptionhandling.cpp 107

 const FSlateBrush* FSlateGameResources::GetBrush( const FName PropertyName, ....) { .... ensureMsgf(BrushAsset, TEXT("Could not find resource '%s'"), PropertyName); .... } 

PVS-Studio warning: V510 The 'EnsureNotFalseFormatted' function is not expected to receive class-type variable as sixth actual argument. slategameresources.cpp 49

findings

, Visual Studio, , . , PVS-Studio. , PVS-Studio VS2013, PVS-Studio 6 . — « » . .

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. A Long-Awaited Check of Unreal Engine 4 .