On March 19, 2014, Unreal Engine 4 was made public 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.
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.
The official website: https://www.unrealengine.com/
The Wikipedia article: Unreal Engine.
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:
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 effortful (see the documentation section, "Direct integration of the analyzer into build automation systems").
And here's PVS-Studio Standalone coming to help! It monitoring compiler calls and get all the necessary information.
This is how the check of Unreal Engine was done:
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).
All that and many other things are described in detail in the article "PVS-Studio Now Supports Any Build System under Windows and Any Compiler. Easy and Right Out of the Box". Do read this article please before you start experimenting with PVS-Studio Standalone!
I found the Unreal Engine project's code very high-quality. For example, developers employ static code analysis during the development, which is hinted at by the following code fragments:
// 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_ANALYSISThese code fragments prove that they use a static code analyzer integrated into Visual Studio. To find out more about this tool, see the article Visual Studio 2013 Static Code Analysis in depth: What? When and How?
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.
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's diagnostic message: V501 There are identical sub-expressions 'Position.Y >= Control.Center.Y - BoxSize.Y * 0.5f' to the left and to the right of the '&&' operator. svirtualjoystick.cpp 97
Notice that the "Position.Y" variable is compared to the "Control.Center.Y - BoxSize.Y * 0.5f" expression twice. This is obviously a typo; the '-' operator should be replaced with '+' in the last line. And the '>=' operator should be replaced with '<='.
Here's one more similar mistake in a condition:
void FOculusRiftHMD::PreRenderView_RenderThread(
FSceneView& View)
{
....
if (View.StereoPass == eSSP_LEFT_EYE ||
View.StereoPass == eSSP_LEFT_EYE)
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'View.StereoPass == eSSP_LEFT_EYE' to the left and to the right of the '||' operator. oculusrifthmd.cpp 1453
It seems that the work with Oculus Rift is not well tested yet.
Let's go on.
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's diagnostic message: V519 The 'NotUsed6' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 86, 88. memorybase.h 88
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:
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:
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's diagnostic message: V522 Dereferencing of the null pointer 'LevelStreamingObject' might take place. unrealengine.cpp 10768
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:
void FStreamingPause::Init()
{
....
if( GStreamingPauseBackground == NULL && GUseStreamingPause )
{
// @todo UE4 merge andrew
// GStreamingPauseBackground = new FFrontBufferTexture(....);
GStreamingPauseBackground->InitRHI();
}
}PVS-Studio's diagnostic message: V522 Dereferencing of the null pointer 'GStreamingPauseBackground' might take place. streamingpauserendering.cpp 197
Almost in every program I check, I get a pile of V595 warnings (examples). These warnings indicate the following trouble:
A pointer is dereferenced first and only then is checked for being null. That's not always an error, but this code is highly suspicious and needs to be checked anyway!
The V595 diagnostic helps us reveal slip-ups like this:
/**
* 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's diagnostic message: V595 The 'GEngine' pointer was utilized before it was verified against nullptr. Check lines: 9714, 9719. unrealengine.cpp 9714
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.
This error is pretty nice. It is about mistakenly declaring a new variable instead of using an already existing one.
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's diagnostic message: V561 It's probably better to assign value to 'Existing' variable than to declare it anew. Previous declaration: streamablemanager.cpp, line 325. streamablemanager.cpp 332
I suspect the code must look like this:
// hmm, maybe it was redirected by a consolidate
TargetName = ResolveRedirects(TargetName);
Existing = StreamableItems.FindRef(TargetName);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's diagnostic message: V579 The Memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pimplrecastnavmesh.cpp 172
The comment warns us that it is dangerous to use Memcmp(). But actually it all is even worse than the programmer expects. The point is that the function compares only a part of the object.
The sizeof(this) operator returns the pointer size; that is, the function will compare the first 4 bytes in a 32-bit program and 8 bytes in a 64-bit program.
The correct code should look as follows:
return FMemory::Memcmp(this, Other, sizeof(*this)) == 0;But that's not the only trouble with the Memcmp() function. Have a look at the following code fragment:
D3D11_STATE_CACHE_INLINE void GetBlendState(
ID3D11BlendState** BlendState, float BlendFactor[4],
uint32* SampleMask)
{
....
FMemory::Memcmp(BlendFactor, CurrentBlendFactor,
sizeof(CurrentBlendFactor));
....
}PVS-Studio's diagnostic message: V530 The return value of function 'Memcmp' is required to be utilized. d3d11statecacheprivate.h 547
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:
FMemory::Memcpy(BlendFactor, CurrentBlendFactor,
sizeof(CurrentBlendFactor));enum ECubeFace;
ECubeFace CubeFace;
friend FArchive& operator<<(
FArchive& Ar,FResolveParams& ResolveParams)
{
....
if(Ar.IsLoading())
{
ResolveParams.CubeFace = (ECubeFace)ResolveParams.CubeFace;
}
....
}PVS-Studio's diagnostic message: V570 The 'ResolveParams.CubeFace' variable is assigned to itself. rhi.h 1279
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.
I do like the following error most of all:
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'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 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:
if(!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[1], &BoneIndex2) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[2], &BoneIndex3))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's diagnostic message: V519 The 'BaseTypeName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 862, 863. openglshaders.cpp 863
The "break;" operator is missing in the very beginning. I guess no comments and explanations are needed.
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:
FVariant& operator=( const TArray<uint8> InArray )
{
Type = EVariantTypes::ByteArray;
Value = InArray;
return *this;
}PVS-Studio's diagnostic message: V801 Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const .. InArray' with 'const .. &InArray'. variant.h 198
It's not a very good idea to pass an array by value. The 'InArray' can and must be passed by a constant reference.
The analyzer generated quite a few warnings related to microoptimizations. I don't think many of them will be really useful, but here you are a list of these fragments just in case: ue-v801-V803.txt.
uint32 GetAllocatedSize() const
{
return UniformVectorExpressions.GetAllocatedSize()
+ UniformScalarExpressions.GetAllocatedSize()
+ Uniform2DTextureExpressions.GetAllocatedSize()
+ UniformCubeTextureExpressions.GetAllocatedSize()
+ ParameterCollections.GetAllocatedSize()
+ UniformBufferStruct
?
(sizeof(FUniformBufferStruct) +
UniformBufferStruct->GetMembers().GetAllocatedSize())
:
0;
}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.
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.
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 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.
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's diagnostic message: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 310, 312. propertystruct.cpp 310
The programmer intended to skip all text in double quotes. The algorithm was meant to be like this:
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.
Here is a simpler code to clarify the point:
if (*p == '\"')
{
while (*p && *p != '\"')
p++;
}To fix the error, you need to change the code in the following way:
if (*p == '\"')
{
p++;
while (*p && *p != '\"')
p++;
}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's diagnostic message: V629 Consider inspecting the '1 << (HashKeyShift - PoolBitShift)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. mallocbinned.h 800
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 );void FOculusRiftHMD::Startup()
{
....
pSensorFusion = new SensorFusion();
if (!pSensorFusion)
{
UE_LOG(LogHMD, Warning,
TEXT("Error creating Oculus sensor fusion."));
return;
}
....
}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.
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:
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's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. paths.cpp 703
One more example:
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's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. slatestyle.h 289
What's left is just diverse subtle issues which are not very interesting to discuss. So let me just cite a few code fragments and corresponding diagnostic messages.
void FNativeClassHeaderGenerator::ExportProperties(....)
{
....
int32 NumByteProperties = 0;
....
if (bIsByteProperty)
{
NumByteProperties;
}
....
}PVS-Studio's diagnostic message: V607 Ownerless expression 'NumByteProperties'. codegenerator.cpp 633
static void GetModuleVersion( .... )
{
....
char* VersionInfo = new char[InfoSize];
....
delete VersionInfo;
....
}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
const FSlateBrush* FSlateGameResources::GetBrush(
const FName PropertyName, ....)
{
....
ensureMsgf(BrushAsset, TEXT("Could not find resource '%s'"),
PropertyName);
....
}PVS-Studio's diagnostic message: V510 The 'EnsureNotFalseFormatted' function is not expected to receive class-type variable as sixth actual argument. slategameresources.cpp 49
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 are the proof: - "Comparison methodology".
I invite all those who want their code to be high-quality to try our code analyzer.