Side effect: check CryEngine 3 SDK with PVS-Studio

We have finished comparing static code analyzers Cppcheck, PVS-Studio and the analyzer built in Visual Studio 2013. During this, more than 10 open source projects were tested. And about some of them you can write articles. Here is another such article about the results of the verification of the CryEngine 3 SDK project.

CryEngine 3 SDK

Wikipedia: CryEngine 3 SDK is a set of software tools for developing computer games on the CryEngine 3 game engine. The CryEngine 3 SDK, like CryEngine 3, is developed and maintained by the German company Crytek. CryEngine 3 SDK is a proprietary freeware for downloading, installing, and using a development tool with which anyone can develop computer games and distribute them for free. For commercial distribution (sales) of games developed on the CryEngine 3 SDK, it is necessary to make license deductions in Crytek.

PVS-Studio

Let's see what interesting can be found in this library with the help of the PVS-Studio analyzer.

Yes, PVS-Studio finds a bit more if you enable level 3 warnings. However, I cannot call it serious defects. 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 argument of the 'sprintf' function. The SIGNED integer type argument is expected. igame.h 66
')
Formally, to print the unsigned variable 'fileIdx', you must use "% u". However, it is doubtful that this variable will reach values ​​greater than INT_MAX. So actually the error does not manifest itself.

Test results

Brief result - you need to use static code analyzers. And then the errors in the program will be less and I will not write about what articles.

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' operator. vehiclemovementarcadewheeled.cpp 3300

The check “angVel.GetLengthSquared ()> 0.001f” is performed twice. One check is superfluous. Or is it a typo, for which another value is not checked.

The same action under different conditions.

Fragment N1.

 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 possibility of logical error presence. Check lines: 255, 261. Locationsdependentobjectmover.cpp 255

It seems to me that this code was written using Copy-Paste. And it seems to me that in this code, after copying, they forgot to fix something.

Fragment N2. The ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass () function is implemented in a very strange way. That's what I understand 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

Similar extremely suspicious places:

• environmentalweapon.cpp 964
• persistantstats.cpp 610
• persistantstats.cpp 714
• recordingsystem.cpp 8924
• movementtransitions.cpp 610
• gamerulescombicaptureobjective.cpp 1692
• vehiclemovementhelicopter.cpp 588

Uninitialized cell array

 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 array 'destructionEvents' consists of two elements. In the constructor, we wanted to initialize the array. But it did not work out.

Not there put the bracket

 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

A rare and interesting mistake. Not there is a closing bracket.

The fact is that the second argument of the function ShouldRecordEvent () is optional. It turns out that in the beginning the function ShouldRecordEvent () is called. Then, the comma operator ',' returns the value that is on the right. The condition depends only on the variable 'pOwnerRaw'.

In general, damn what happened.

Forgot to write the name of the function

 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 expression, the variable pMessage is assigned the value currentSeat-> GetSeatName (). No formatting happens. As a result, the string will be missing a colon ':'. Though a trifle, but an error.

It should have been:

 string pMessage = string().Format("%s:", currentSeat->GetSeatName()); 

Senseless and merciless checks

Fragment N1.

 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 the "stricmp" function is equal to the second argument. ishader.h 2089

If you have not noticed an error, then I prompt. The string m_Name.c_str () is compared to itself. It should be written:

 stricmp(m_Name.c_str(), m.m_Name.c_str()) 

Fragment N2. Now the logical error:

 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

Check in this code does not make sense. I will give an analogy:

 if (A != 1 || A != 2) 

The condition is always satisfied.

Fragment N3.

 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 N4.

 inline typename CryStringT<T>::size_type CryStringT<T>::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 assignment “pos = length ()” will never be executed.

Similar code here: cryfixedstring.h 1297

Pointers

Programmers love to check for a pointer to equality to zero. Now, if they still knew how often they did it wrong. Check when it's too late.

I will give one example, the rest will list.

 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 used against it. Check lines: 325, 326. scripthelpers.h 325

Promised list of 35 messages: CryEngineSDK-595.txt

Undefined behavior

 void AddSample( T x ) { m_index = ++m_index % N; .... } 

V567 Undefined behavior. The m_index variable inetwork.h 2303

One-time cycles

 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 body of the loop will perform only once. The reason is the unconditional 'break' operator. At the same time, there is no 'continue' operator in the loop.

There are several more such suspicious cycles :

• gunturret.cpp 1647
• vehiclemovementbase.cpp 2362
• vehiclemovementbase.cpp 2382

Strange assignments

Fragment N1.

 void CPlayerStateGround::OnPrePhysicsUpdate(....) { .... modifiedSlopeNormal.z = modifiedSlopeNormal.z; .... } 

V570 The 'modifiedSlopeNormal.z' variable is assigned to itself. playerstateground.cpp 227

Fragment N2.

 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

Member of the class 'objtypes' is assigned a value twice.

Fragment N3.

 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

And a few more such anomalous 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

Need to be careful with entity names

 void CGamePhysicsSettings::Debug(....) const { .... sprintf_s(buf, bufLen, pEntity->GetName()); .... } 

V618 is dangerous to the sprintf_s. The example of the safe code: printf ("% s", str); gamephysicssettings.cpp 174

This is probably not a mistake, but a dangerous code. If the symbol '%' is encountered in the entity name, the results can be quite unexpected.

Lone Wanderer

 CPersistantStats::SEnemyTeamMemberInfo *CPersistantStats::GetEnemyTeamMemberInfo(EntityId inEntityId) { .... insertResult.first->second.m_entityId; .... } 

V607 Ownerless expression 'insertResult.first-> second.m_entityId'. persistantstats.cpp 4814

A lonely expression that does nothing. Mistake? Unfinished code?

Another place in the code: recordingsystem.cpp 2671

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 against _ _ _ _ pointer against pointer against pointer pointer pointer pointer pointer _ _ _ _ The exception will be generated in the case of memory allocation error. crylobbypacket.h 88

Code is outdated. Now, with a memory allocation error, the 'new' operator throws an exception.

Other places waiting for 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

findings

I have no particular conclusions. But I imagine how much interesting things can be found not in the CryEngine 3 SDK, but in the CryEngine 3 engine itself.

I wish you all a reckless code!

PS

Someone constantly sends foreigners a link to a Russian article. Here is the translation: A Spin-off: CryEngine 3 SDK Checked with PVS-Studio .