We have finished a large comparison of the static code analyzers Cppcheck, PVS-Studio and Visual Studio 2013's built-in analyzer. In the course of this investigation, we checked over 10 open-source projects. Some of them do deserve to be discussed specially. In today's article, I'll tell you about the results of the check of the CryEngine 3 SDK project.
CryEngine 3 SDK
Wikipedia:
CryEngine 3 SDK is a toolset for developing computer games on the CryEngine 3 game engine. CryEngine 3 SDK is developed and maintained by German company Crytek, the developer of the original engine CyrEngine 3. CryEngine 3 SDK is a proprietary freeware development toolset anyone can use for non-commercial game development. For commercial game development exploiting CryEngine 3, developers have to pay royalties to Crytek.
PVS-Studio
Let's see if
PVS-Studio has found any interesting bugs in this library.
True, PVS-Studio catches a bit more bugs if you turn on the 3rd severity level diagnostics.
For example:
static void GetNameForFile(
const char* baseFileName,
const uint32 fileIdx,
char outputName[512] )
{
assert(baseFileName != NULL);
sprintf( outputName, "%s_%d", baseFileName, fileIdx );
}
V576 Incorrect format. Consider checking the fourth actual argument of the 'sprintf' function. The SIGNED integer type argument is expected. igame.h 66
From the formal viewpoint, the programmer should have used
%u to print the unsigned variable
fileIdx. But I'm very doubtful that this variable will ever reach a value larger than
INT_MAX. So this error will not cause any severe consequences.
Analysis results
My brief comment on the analysis results is, developers should use static analysis. There will be much fewer bugs in programs and I will drop writing articles like this one.
Double check
void CVehicleMovementArcadeWheeled::InternalPhysicsTick(float dt)
{
....
if (fabsf(m_movementAction.rotateYaw)>0.05f ||
vel.GetLengthSquared()>0.001f ||
m_chassis.vel.GetLengthSquared()>0.001f ||
angVel.GetLengthSquared()>0.001f ||
angVel.GetLengthSquared()>0.001f)
....
}
V501:
There are identical sub-expressions 'angVel.GetLengthSquared() > 0.001f' to the left and to the right of the '||' operator. vehiclemovementarcadewheeled.cpp 3300
The
angVel.GetLengthSquared()>0.001f check is executed twice. One of them is redundant, or otherwise there is a typo in it which prevents some other value from being checked.
Identical code blocks under different conditions
Fragment No. 1.
void CVicinityDependentObjectMover::HandleEvent(....)
{
....
else if ( strcmp(szEventName, "ForceToTargetPos") == 0 )
{
SetState(eObjectRangeMoverState_MovingTo);
SetState(eObjectRangeMoverState_Moved);
ActivateOutputPortBool( "OnForceToTargetPos" );
}
else if ( strcmp(szEventName, "ForceToTargetPos") == 0 )
{
SetState(eObjectRangeMoverState_MovingTo);
SetState(eObjectRangeMoverState_Moved);
ActivateOutputPortBool( "OnForceToTargetPos" );
}
....
}
V517:
The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 255, 261. vicinitydependentobjectmover.cpp 255
I suspect that this piece of code was written through the Copy-Paste technique. I also suspect that the programmer forgot to change some lines after the copying.
Fragment No. 2.
The
ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass() function is implemented in a very strange way. That's a
real name!
bool CGameRules::
ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass
(const IEntityClass* pEntityClass) const
{
assert(pEntityClass != NULL);
if(gEnv->bMultiplayer)
{
return
(pEntityClass == s_pSmartMineClass) ||
(pEntityClass == s_pTurretClass) ||
(pEntityClass == s_pC4Explosive);
}
else
{
return
(pEntityClass == s_pSmartMineClass) ||
(pEntityClass == s_pTurretClass) ||
(pEntityClass == s_pC4Explosive);
}
}
V523:
The 'then' statement is equivalent to the 'else' statement. gamerules.cpp 5401
Other similar defects: - environmentalweapon.cpp 964
- persistantstats.cpp 610
- persistantstats.cpp 714
- recordingsystem.cpp 8924
- movementtransitions.cpp 610
- gamerulescombicaptureobjective.cpp 1692
- vehiclemovementhelicopter.cpp 588
An uninitialized array cell
TDestructionEventId destructionEvents[2];
SDestructibleBodyPart()
: hashId(0)
, healthRatio(0.0f)
, minHealthToDestroyOnDeathRatio(0.0f)
{
destructionEvents[0] = -1;
destructionEvents[0] = -1;
}
V519:
The 'destructionEvents[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 75, 76. bodydestruction.h 76
The
destructionEvents array consists of two items. The programmer wanted to initialize the array in the constructor, but failed.
A parenthesis in a wrong place
bool ShouldRecordEvent(size_t eventID, IActor* pActor=NULL) const;
void CActorTelemetry::SubscribeToWeapon(EntityId weaponId)
{
....
else if(pMgr->ShouldRecordEvent(eSE_Weapon), pOwnerRaw)
....
}
V639:
Consider inspecting the expression for 'ShouldRecordEvent' function call. It is possible that one of the closing ')' brackets was positioned incorrectly. actortelemetry.cpp 288
It's a rare and interesting bug - a closing parenthesis is written in a wrong place.
The point is that the
ShouldRecordEvent() function's second argument is optional. It turns that the
ShouldRecordEvent() function is called first, and then the comma operator
, returns the value on the right. The condition depends on the
pOwnerRaw variable alone.
Long story short, the whole thing is darn messed up here.
A function name missing
virtual void ProcessEvent(....)
{
....
string pMessage = ("%s:", currentSeat->GetSeatName());
....
}
V521:
Such expressions using the ',' operator are dangerous. Make sure the expression '"%s:", currentSeat->GetSeatName()' is correct. flowvehiclenodes.cpp 662
In this fragment, the
pMessage variable is assigned the value
currentSeat->GetSeatName(). No formatting is done, and it leads to missing the colon ':' in this line. Though a trifle, it is still a bug.
The fixed code should look like this:
string pMessage =
string().Format("%s:", currentSeat->GetSeatName());
Senseless and pitiless checks
Fragment No. 1.
inline bool operator != (const SEfResTexture &m) const
{
if (stricmp(m_Name.c_str(), m_Name.c_str()) != 0 ||
m_TexFlags != m.m_TexFlags ||
m_bUTile != m.m_bUTile ||
m_bVTile != m.m_bVTile ||
m_Filter != m.m_Filter ||
m_Ext != m.m_Ext ||
m_Sampler != m.m_Sampler)
return true;
return false;
}
V549:
The first argument of 'stricmp' function is equal to the second argument. ishader.h 2089
If you haven't noticed the bug, I'll tell you. The
m_Name.c_str() string is compared to itself. The correct code should look like this:
stricmp(m_Name.c_str(), m.m_Name.c_str())
Fragment No. 2.
A logical error this time:
SearchSpotStatus GetStatus() const { return m_status; }
SearchSpot* SearchGroup::FindBestSearchSpot(....)
{
....
if(searchSpot.GetStatus() != Unreachable ||
searchSpot.GetStatus() != BeingSearchedRightAboutNow)
....
}
V547:
Expression is always true. Probably the '&&' operator should be used here. searchmodule.cpp 469
The check in this code does not make any sense. Here is an analogy:
if (A != 1 || A != 2)
The condition is always true.
Fragment No. 3.
const CCircularBufferTimeline *
CCircularBufferStatsContainer::GetTimeline(
size_t inTimelineId) const
{
....
if (inTimelineId >= 0 && (int)inTimelineId < m_numTimelines)
{
tl = &m_timelines[inTimelineId];
}
else
{
CryWarning(VALIDATOR_MODULE_GAME,VALIDATOR_ERROR,
"Statistics event %" PRISIZE_T
" is larger than the max registered of %"
PRISIZE_T ", event ignored",
inTimelineId,m_numTimelines);
}
....
}
V547:
Expression 'inTimelineId >= 0' is always true. Unsigned type value is always >= 0. circularstatsstorage.cpp 31
Fragment No. 4.
inline typename CryStringT::size_type
CryStringT::rfind( value_type ch, size_type pos ) const
{
const_str str;
if (pos == npos) {
....
} else {
if (pos == npos)
pos = length();
....
}
V571:
Recurring check. The 'if (pos == npos)' condition was already verified in line 1447. crystring.h 1453
The
pos = length() assignment will never be executed.
A similar defect: cryfixedstring.h 1297
Pointers
Programmers are very fond of checking pointers for being null. Wish they knew how often they do it wrong - check when it's too late.
I'll cite only one example and give you a link to a file with the list of all the other samples.
IScriptTable *p;
bool Create( IScriptSystem *pSS, bool bCreateEmpty=false )
{
if (p) p->Release();
p = pSS->CreateTable(bCreateEmpty);
p->AddRef();
return (p)?true:false;
}
V595:
The 'p' pointer was utilized before it was verified against nullptr. Check lines: 325, 326. scripthelpers.h 325
The list of other 35 messages: CryEngineSDK-595.txt
Undefined behavior
void AddSample( T x )
{
m_index = ++m_index % N;
....
}
V567:
Undefined behavior. The 'm_index' variable is modified while being used twice between sequence points. inetwork.h 2303
One-time loops
void CWeapon::AccessoriesChanged(bool initialLoadoutSetup)
{
....
for (int i = 0; i < numZoommodes; i++)
{
CIronSight* pZoomMode = ....
const SZoomModeParams* pCurrentParams = ....
const SZoomModeParams* pNewParams = ....
if(pNewParams != pCurrentParams)
{
pZoomMode->ResetSharedParams(pNewParams);
}
break;
}
....
}
V612:
An unconditional 'break' within a loop. weapon.cpp 2854
The loop body will be executed only once because of the unconditional statement
break, while there are no
continue operators around in this loop.
We found a few more suspicious loops like that:
- gunturret.cpp 1647
- vehiclemovementbase.cpp 2362
- vehiclemovementbase.cpp 2382
Strange assignments
Fragment No. 1.
void CPlayerStateGround::OnPrePhysicsUpdate(....)
{
....
modifiedSlopeNormal.z = modifiedSlopeNormal.z;
....
}
V570:
The 'modifiedSlopeNormal.z' variable is assigned to itself. playerstateground.cpp 227
Fragment No. 2.
const SRWIParams& Init(....)
{
....
objtypes=ent_all;
flags=rwi_stop_at_pierceable;
org=_org;
dir=_dir;
objtypes=_objtypes;
....
}
V519:
The 'objtypes' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2807, 2808. physinterface.h 2808
The
objtypes class member is assigned values twice.
Fragment No. 3.
void SPickAndThrowParams::SThrowParams::SetDefaultValues()
{
....
maxChargedThrowSpeed = 20.0f;
maxChargedThrowSpeed = 15.0f;
}
V519:
The 'maxChargedThrowSpeed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1284, 1285. weaponsharedparams.cpp 1285
A few more similar strange assignments:
- The bExecuteCommandLine variable. Check lines: 628, 630. isystem.h 630
- The flags variable. Check lines: 2807, 2808. physinterface.h 2808
- The entTypes Variable. Check lines: 2854, 2856. physinterface.h 2856
- The geomFlagsAny variable. Check lines: 2854, 2857. physinterface.h 2857
- The m_pLayerEffectParams variable. Check lines: 762, 771. ishader.h 771
Careless entity names
void CGamePhysicsSettings::Debug(....) const
{
....
sprintf_s(buf, bufLen, pEntity->GetName());
....
}
V618:
It's dangerous to call the 'sprintf_s' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); gamephysicssettings.cpp 174
It's not quite an error, but a dangerous code anyway. Should the
% character be used in an entity name, it may lead to absolutely unpredictable consequences.
Lone wanderer
CPersistantStats::SEnemyTeamMemberInfo
*CPersistantStats::GetEnemyTeamMemberInfo(EntityId inEntityId)
{
....
insertResult.first->second.m_entityId;
....
}
V607:
Ownerless expression 'insertResult.first->second.m_entityId'. persistantstats.cpp 4814
An alone standing expression doing nothing. What is it? A bug? Incomplete code?
Another similar fragment: recordingsystem.cpp 2671
The new operator
bool CreateWriteBuffer(uint32 bufferSize)
{
FreeWriteBuffer();
m_pWriteBuffer = new uint8[bufferSize];
if (m_pWriteBuffer)
{
m_bufferSize = bufferSize;
m_bufferPos = 0;
m_allocated = true;
return true;
}
return false;
}
V668:
There is no sense in testing the 'm_pWriteBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. crylobbypacket.h 88
The code is obsolete. Nowadays, the
new operator throws an exception when a memory allocation error occurs.
Other fragments in need of refactoring:
- cry_math.h 73
- datapatchdownloader.cpp 106
- datapatchdownloader.cpp 338
- game.cpp 1671
- game.cpp 4478
- persistantstats.cpp 1235
- sceneblurgameeffect.cpp 366
- killcamgameeffect.cpp 369
- downloadmgr.cpp 1090
- downloadmgr.cpp 1467
- matchmakingtelemetry.cpp 69
- matchmakingtelemetry.cpp 132
- matchmakingtelemetry.cpp 109
- telemetrycollector.cpp 1407
- telemetrycollector.cpp 1470
- telemetrycollector.cpp 1467
- telemetrycollector.cpp 1479
- statsrecordingmgr.cpp 1134
- statsrecordingmgr.cpp 1144
- statsrecordingmgr.cpp 1267
- statsrecordingmgr.cpp 1261
- featuretester.cpp 876
- menurender3dmodelmgr.cpp 1373
Conclusions
No special conclusions. But I wish I could check the CryEngine 3 engine itself, rather than CryEngine 3 SDK. Guess how many bugs I could find there?
May your code stay bugless!
Wow, that was impressively uninteresting.
Conclusion: Every project contains bugs, you just can't eliminate them all.
It is just a list of potential false/positive bugs you found with a tool.
Is it proven that those bugs are functional bugs or if those code fragments are used at all? We don't know even that.
Next article should be a list of the results when you search for "software bug" in google images.