The Empire Strikes Back

Recently there appeared an article “Hackathon 2: Time lapse analysis of Unreal Engine 4”, which describes how you can find a great deal of bugs in Unreal Engine 4 using Klocwork. I just can’t help commenting on this article. The thing is that once we fixed all the bugs that PVS-Studio analyzer found in this project. Of course, we haven’t worked on all bugs existing in the project, only on those that were detected by our analyzer. However, the article creates an impression that the PVS-Studio analyzer skipped too many bugs. Well, I guess now it’s my turn to say something. I have also rechecked Unreal Engine 4 and found plenty of another bugs. So I can claim that PVS-Studio can find new bugs in Unreal Engine 4. It’s a draw.

History Reference

It all started a year and a half ago, when I wrote an article “A Long-Awaited Check of Unreal Engine 4”, which led to our cooperation with Epic Games company, resulting in the removal of all warnings issued by PVS-Studio. During our work we have fixed a large number of errors and removed all false positives of the analyzer. Our team provided Epic Games Company with a project, free from PVS-Studio warnings. You can read this article “How the PVS-Studio Team Improved Unreal Engine’s Code” to see more details.

But not so long ago I came across another article: “Hackathon 2: Time lapse analysis of Unreal Engine 4”. And I should say that this article is of good quality and very informative. In general, Rogue Wave does a good job making such a powerful analyzer as Klocwork and organizing activities like open-source code checks. We also should give credit to Michail Greshishchev for checking the Unreal Engine code and taking time to write an article about it. It is very beneficial for the programmer community. But I am a little concerned by the fact that a person, who is not very familiar with static analyzers can come to wrong conclusions. Therefore, I have to comment on the article.

Unintentionally, but this article might show our team in a bad light in comparison with Klocwork. It may seem that PVS-Studio finds less bugs that Klocwork. But the truth is that this world is more complicated. Both analyzers have a great many of diagnostics and partially these diagnostics may overlap each other. But each analyzer has a unique set of diagnostics, that’s why having checked a project with one analyzer you will always find something else with the help of the other.

One more little detail. We haven’t checked third-party libraries (at least partially), while Michail Greshishchev has done it, as we can see it looking at one of the code snippets (see HeadMountedDisplayCommon function) In the ThirdParty PVS-Studio Analyzer can also easily find many interesting flaws. Moreover, the size of the ThirdParty source code is three times bigger than of the UE4 itself.

But this sounds like a pathetic attempt to excuse us :). So I don’t have anything else to do as to even the score. For this purpose we’ve downloaded Unreal Engine 4 source code and rechecked it with PVS-Studio.

And now I’ll show you that you can always easily find errors in large rapidly changing projects.

PVS-Studio check results

I’ve checked UE4 source code with the latest version of PVS-Studio. ThirdParty libraries weren’t included in the checking process. Otherwise, I’d get a whole reference book, not an article =)

So, I’ve got 1792 general analysis warnings of the 1st and 2nd level. But don’t be scared, I’ll explain where this number comes from.

The majority of these warnings (93%) are issued due to the implementation of a new diagnostic rule V730, intended to identify uninitialized class members. An uninitialized class member is not always an error, but nevertheless it is a spot in the program which is worth checking out. In general 1672 warnings of V730 diagnostic is a lot. I haven’t seen such a number of these warnings in other projects. Besides that the analyzer tries to foresee whether the uninitialized class member will cause any further difficulties or not. By the way, it’s not a very rewarding job — to look for uninitialized members; may be our readers would be interested to get to know why. You can have a look at this article “In search of uninitialized class members”.

But let’s get back to UE4. In this article I won’t be speaking about V730 warnings in details. There are too many of them and I can not say that I know UE4 project well enough to determine if some uninitialized variables will lead to an error or not. However, I am quite sure that there are some serious bugs hiding among those 1672 warnings. I suppose, it could be worth analyzing them. Even if the Epic Games developers would consider those warnings to be nothing more than false positives, they can easily turn this diagnostic off.

So, 1792–1672 = 120. In sum total PVS-Studio issued 120 warnings of general analysis (1 and 2 level) during the Unreal Engine check. A good number of these warnings have revealed real errors. Let’s have a closer look at the most interesting code snippets and corresponding warnings.

Interesting bugs found with the help of PVS-Studio

I should emphasize once more, that the list of errors I’m speaking about here is far from being a complete one. Firstly, I could skip something of interest, as I wasn’t able to spend a decent amount of time looking at all fragments properly. Secondly, I haven’t noted down those errors that aren’t very crucial or those that will require a lot of clarification (and code fragments for explanation).

Error N1

FORCEINLINE
bool operator==(const FShapedGlyphEntryKey& Other) const
{
return FontFace == Other.FontFace
&& GlyphIndex == Other.GlyphIndex
&& FontSize == Other.FontSize
&& FontScale == Other.FontScale
&& GlyphIndex == Other.GlyphIndex;
}

PVS-Studio warning V501 There are identical sub expressions ‘GlyphIndex == Other.GlyphIndex’ to the left and to the right of the ‘&&’ operator. fontcache. h 139

“GlyphIndex == Other.GlyphIndex” is checked twice. The Last Line Effect in action. Apparently, the last comparison should be: KeyHash == Other.KeyHash.

Error N2

Another last line effect, almost canonical.

bool
Compare(const FPooledRenderTargetDesc& rhs, bool bExact) const
{
....
return Extent == rhs.Extent
&& Depth == rhs.Depth
&& bIsArray == rhs.bIsArray
&& ArraySize == rhs.ArraySize
&& NumMips == rhs.NumMips
&& NumSamples == rhs.NumSamples
&& Format == rhs.Format
&& LhsFlags == RhsFlags
&& TargetableFlags == rhs.TargetableFlags
&& bForceSeparateTargetAndShaderResource ==
rhs.bForceSeparateTargetAndShaderResource
&& ClearValue == rhs.ClearValue
&& AutoWritable == AutoWritable;
}

PVS-Studio warning V501 There are identical sub-expressions to the left and to the right of the ‘==’ operator: AutoWritable == AutoWritable rendererinterface.h 180

At the very end a programmer forgot to add “rhs” and as a result the variable ‘AutoWritable’ is compared with itself.

Error N3

void AEQSTestingPawn::PostLoad() 
{
....
UWorld* World = GetWorld();
if (World && World->IsGameWorld() &&
bTickDuringGame == bTickDuringGame)
{
PrimaryActorTick.bCanEverTick = false;
}
}

PVS-Studio warning V501 There are identical sub-expressions to the left and to the right of the ‘==’ operator: bTickDuringGame == bTickDuringGame eqstestingpawn.cpp 157

Error N4

int32 SRetainerWidget::OnPaint(....) const
{
....
if ( RenderTargetResource->GetWidth() != 0 &&
RenderTargetResource->GetWidth() != 0 )
....
}

PVS-Studio warning V501 There are identical sub-expressions ‘RenderTargetResource->GetWidth() != 0’ to the left and to the right of the ‘&&’ operator. sretainerwidget.cpp 291

Error N5, N6

There are two similar errors, located close to each other. ZeroMemory macros which are mere memset() function calls, zero only a part of the allocated memory.

class FD3D12BufferedGPUTiming
{
....
FD3D12CLSyncPoint* StartTimestampListHandles;
FD3D12CLSyncPoint* EndTimestampListHandles;
....
};
void FD3D12BufferedGPUTiming::InitDynamicRHI()
{
....
StartTimestampListHandles = new FD3D12CLSyncPoint[BufferSize];
ZeroMemory(StartTimestampListHandles,
sizeof(StartTimestampListHandles));
  EndTimestampListHandles = new FD3D12CLSyncPoint[BufferSize];
ZeroMemory(EndTimestampListHandles,
sizeof(EndTimestampListHandles));
....
}

PVS-Studio warnings:

  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘StartTimestampListHandles’. d3d12query.cpp 493
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘EndTimestampListHandles’. d3d12query.cpp 495

The error is that the sizeof() operator evaluates the size of the pointer, not the array. One of the correct options will be:

ZeroMemory(StartTimestampListHandles,
sizeof(FD3D12CLSyncPoint) * BufferSize);
ZeroMemory(EndTimestampListHandles,
sizeof(FD3D12CLSyncPoint) * BufferSize);

Error N7

void FDeferredShadingSceneRenderer::RenderLight(....)
{
....
if (bClearCoatNeeded)
{
SetShaderTemplLighting<false, false, false, true>(
RHICmdList, View, *VertexShader, LightSceneInfo);
}
else
{
SetShaderTemplLighting<false, false, false, true>(
RHICmdList, View, *VertexShader, LightSceneInfo);
}
....
}

PVS-Studio warning V523 The ‘then’ statement is equivalent to the ‘else’ statement. lightrendering.cpp 864

Regardless of the conditions, two similar actions are carried out.

Error N8

bool FBuildDataCompactifier::Compactify(....) const
{
....
uint64 CurrentFileSize;
....
CurrentFileSize = IFileManager::Get().FileSize(*File);
if (CurrentFileSize >= 0)
{
....
}
else
{
GLog->Logf(TEXT("Warning. ......"), *File);
}
....
}

PVS-Studio warning V547 Expression ‘CurrentFileSize >= 0’ is always true. Unsigned type value is always >= 0. buildpatchcompactifier.cpp 135

“if (CurrentFileSize > = 0)” check makes no sense. The variable ‘CurrentFileSize’ is of the unsigned type, and thus its value is always > = 0.

Error N9

template<typename TParamRef>
void UnsetParameters(....)
{
....
int32 NumOutUAVs = 0;
FUnorderedAccessViewRHIParamRef OutUAVs[3];
OutUAVs[NumOutUAVs++] = ObjectBuffers......;
OutUAVs[NumOutUAVs++] = ObjectBuffers.Bounds.UAV;
OutUAVs[NumOutUAVs++] = ObjectBuffers.Data.UAV;
  if (CulledObjectBoxBounds.IsBound())
{
OutUAVs[NumOutUAVs++] = ObjectBuffers.BoxBounds.UAV;
}
....
}

V557 Array overrun is possible. The ‘NumOutUAVs ++’ index is pointing beyond array bound. distancefieldlightingshared.h 388

If the condition (CulledObjectBoxBounds.IsBound()) is executed, then the array index is out of bounds. Note that the ‘OutUAVs’ array consists of only 3 elements.

Error N10

class FSlateDrawElement
{
....
FORCEINLINE void SetPosition(const FVector2D& InPosition)
{ Position = Position; }
....
FVector2D Position;
....
};

PVS-Studio warning V570 The ‘Position’ variable is assigned to itself. drawelements.h 435

It’s not even worth the time looking at this bug, it’s just a typo. We should have:

{ Position = InPosition; }.

Error N11

bool FOculusRiftHMD::DoesSupportPositionalTracking() const
{
const FGameFrame* frame = GetFrame();
const FSettings* OculusSettings = frame->GetSettings();
return (frame && OculusSettings->Flags.bHmdPosTracking &&
(OculusSettings->SupportedTrackingCaps &
ovrTrackingCap_Position) != 0);
}

PVS-Studio warning V595 The ‘frame’ pointer was utilized before it was verified against nullptr. Check lines: 301, 302. oculusrifthmd.cpp 301

We see that first ‘frame’ variable is used and then it is checked if it is equal to nullity.

This error is very similar to the one described in the article of Klocwork:

bool FHeadMountedDisplay::IsInLowPersistenceMode() const
{
const auto frame = GetCurrentFrame();
const auto FrameSettings = frame->Settings;
return frame && FrameSettings->Flags.bLowPersistenceMode;
}

As you can see, both analyzers can identify this type of a flaw.

It’s worth mentioning that the code given in the article by Klocwork, refers to the ThirdParty repository, which we haven’t checked.

Error N12 — N21

FName UKismetNodeHelperLibrary::GetEnumeratorName(
const UEnum* Enum, uint8 EnumeratorValue)
{
int32 EnumeratorIndex = Enum->GetIndexByValue(EnumeratorValue);
return (NULL != Enum) ?
Enum->GetEnum(EnumeratorIndex) : NAME_None;
}

PVS-Studio warning V595 The ‘Enum’ pointer was utilized before it was verified against nullptr. Check lines: 146, 147. kismetnodehelperlibrary.cpp 146

Again we have a situation when a pointer gets dereferenced first and only then it is checked. It’s rather boring to look at such errors. I’ll just list fragments that worth looking at:

  • V595 The ‘Class’ pointer was utilized before it was verified against nullptr. Check lines: 278, 282. levelactor.cpp 278
  • V595 The ‘Template’ pointer was utilized before it was verified against nullptr. Check lines: 380, 386. levelactor.cpp 380
  • V595 The ‘UpdatedComponent’ pointer was utilized before it was verified against nullptr. Check lines: 100, 116. interptomovementcomponent.cpp 100
  • V595 The ‘SourceTexture’ pointer was utilized before it was verified against nullptr. Check lines: 150, 178. d3d12rendertarget.cpp 150
  • V595 The ‘NewRenderTarget’ pointer was utilized before it was verified against nullptr. Check lines: 922, 924. d3d11commands.cpp 922
  • V595 The ‘RenderTarget’ pointer was utilized before it was verified against nullptr. Check lines: 2173, 2175. d3d11commands.cpp 2173
  • V595 The ‘MyMemory’ pointer was utilized before it was verified against nullptr. Check lines: 210, 217. bttask_moveto.cpp 210
  • V595 The ‘SkelComp’ pointer was utilized before it was verified against nullptr. Check lines: 79, 100. animnode_animdynamics.cpp 79
  • V595 The ‘Result’ pointer was utilized before it was verified against nullptr. Check lines: 1000, 1004. uobjectglobals.cpp 1000

Error N22

class FD3D12Device
{
....
virtual void InitD3DDevice();
virtual void CleanupD3DDevice();
....
// Destructor is not declared
....
};

V599 The virtual destructor is not present, although the ‘FD3D12Device’ class contains virtual functions. d3d12device.cpp 448

In the FD3D12Device class there are virtual methods. Which means that this class will have derived classes.
In this class, however, there is no virtual destructor. It is very dangerous and most likely will lead to an error sooner or later.

Error N23 — N26

int SpawnTarget(WCHAR* CmdLine)
{
....
if(!CreateProcess(....))
{
DWORD ErrorCode = GetLastError();
    WCHAR* Buffer = new WCHAR[wcslen(CmdLine) + 50];
wsprintf(Buffer,
L"Couldn't start:\n%s\nCreateProcess() returned %x.",
CmdLine, ErrorCode);
MessageBoxW(NULL, Buffer, NULL, MB_OK);
delete Buffer;
    return 9005;
}
....
}

PVS-Studio warning 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 [] Buffer;’. bootstrappackagedgame.cpp 110

The allocated memory is deallocated in a wrong way. It should be like this:

delete [] Buffer;

Couple more similar errors:

  • 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 [] ChildCmdLine;’. bootstrappackagedgame.cpp 157
  • 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 [] ChildCmdLine;’. bootstrappackagedgame.cpp 165
  • 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 [] ChildCmdLine;’. bootstrappackagedgame.cpp 169

Error N27

void FSlateTexture2DRHIRef::InitDynamicRHI()
{
....
checkf(GPixelFormats[PixelFormat].BlockSizeX ==
GPixelFormats[PixelFormat].BlockSizeY ==
GPixelFormats[PixelFormat].BlockSizeZ == 1,
TEXT("Tried to use compressed format?"));
....
}

PVS-Studio warning V709 Suspicious comparison found: ‘a == b == c’. Remember that ‘a == b == c’ is not equal to ‘a == b && b == c’. slatetextures.cpp 67

The check doesn’t work in the way the programmer wanted it. Instead we should write:

GPixelFormats[PixelFormat].BlockSizeX == 1 &&
GPixelFormats[PixelFormat].BlockSizeY == 1 &&
GPixelFormats[PixelFormat].BlockSizeZ == 1

Error N28

void UWidgetComponent::UpdateRenderTarget()
{
....
FLinearColor ActualBackgroundColor = BackgroundColor;
switch ( BlendMode )
{
case EWidgetBlendMode::Opaque:
ActualBackgroundColor.A = 1.0f;
case EWidgetBlendMode::Masked:
ActualBackgroundColor.A = 0.0f;
}
....
}

V519 The ‘ActualBackgroundColor.A’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 938, 940. widgetcomponent.cpp 940

Here we see that the omitted ‘break’ operator is detected. ‘ActualBackgroundColor.A’ variable can be assigned with two different values two times successively. This is what makes the analyzer suspicious.

Error N29

void FProfilerManager::TrackDefaultStats()
{
// Find StatId for the game thread.
for( auto It = GetProfilerInstancesIterator(); It; ++It )
{
FProfilerSessionRef ProfilerSession = It.Value();
if( ProfilerSession->GetMetaData()->IsReady() )
{
....;
}
break;
}
}

PVS-Studio warning V612 An unconditional ‘break’ within a loop. profilermanager.cpp 717

This is a very suspicious code fragment. It seems that the ‘break’ operator is not on the right place. I’m not quite sure, but possibly it should be written like this:

for( auto It = GetProfilerInstancesIterator(); It; ++It )
{
FProfilerSessionRef ProfilerSession = It.Value();
if( ProfilerSession->GetMetaData()->IsReady() )
{
....;
break;
}
}

Overall results

At least 29 out of 120 warnings issued by PVS-Studio indicated real bugs (24%). Another 50% is the code that stinks. Remaining ones are false positives. The total amount of time spent checking the project and writing the article is about 10 hours.

Conclusions that can be drawn based on the check results of the PVS-Studio analyzer and Klocwork:

  • In a large and rapidly developing project you can always find more bugs :)
  • Sets of diagnostics in PVS-Studio and Klocwork are different, but there are some diagnostics that are similar.
  • Perhaps, Klocwork checked Unreal Engine 4, including third-party libraries (ThirdParty). We haven’t checked them at all.
  • Both analyzers did a great job. Using them can be really beneficial for the program development.

Thank you for your attention.

Article republished by the author’s permission.
One clap, two clap, three clap, forty?

By clapping more or less, you can signal to us which stories really stand out.