Recently there appeared an article "Hackathon 2: Time lapse analysis of Unreal Engine 4", which describes how you can find a great number 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, we haven't necessarily 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.
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, 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, this article might be interpreted as showing 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 diagnostics and these diagnostics may partially 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 didn't check the third-party libraries (at least partially), while Michail Greshishchev obviously did; as we can see by looking at one of the code snippets (see HeadMountedDisplayCommon function in the ThirdParty). Of course, PVS-Studio can also easily find many interesting flaws in the ThirdParty repository, especially as 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.
I've checked UE4 source code with the latest version of PVS-Studio. ThirdParty libraries weren't included in the re-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 might 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 cannot 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.
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 the code fragments in detail. 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:
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:
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 most likely 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:
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 == 1Error 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;
}
}At least 29 out of 120 warnings issued by PVS-Studio indicated real bugs (24%). Another 50% is the code that smells. The remaining ones are false positives. The total amount of time spent checking the project and writing the article was about 10 hours.
Conclusions that can be drawn based on the check results of the PVS-Studio analyzer and Klocwork:
Thank you for your attention.