By-product: we check CryEngine 3 SDK using PVS-Studio

We have finished comparing Cppcheck static code analyzers, PVS-Studio and the analyzer built into Visual Studio 2013. During this, more than 10 open 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 - a set of software tools for developing computer games on the CryEngine 3 game engine. CryEngine 3 SDK, like CryEngine 3, was developed and supported by the German company Crytek. CryEngine 3 SDK is a proprietary freeware for development, downloading, installation and use by means of which anyone can develop computer games and distribute them for free. In the commercial distribution (sales) of games developed on the CryEngine 3 SDK, you must make royalties to Crytek.

PVS-Studio

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

Yes, PVS-Studio finds a bit more if you enable level 3 alerts. However, I cannot call this a serious defect. 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

Formally, to print the unsigned variable 'fileIdx', use '% u'. However, it is doubtful that this variable will reach a value greater than INT_MAX. So in fact, the error will not manifest itself.

Validation Results

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

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 performed two times. One check is unnecessary. Or is it a typo, for which a different 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 probability of logical error presence. Check lines: 255, 261. vicinitydependentobjectmover.cpp 255

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

Fragment N2. In a very strange way, the ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass () function is implemented. This is 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 highly suspicious places:

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

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 array 'destructionEvents' consists of two elements. In the constructor, they wanted to initialize the array. But it didn’t work out.

Put the bracket in the 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

A rare and interesting bug. The closing bracket is not put there.

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

In general, the hell with what happened.

Forgot to write function name

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 set to currentSeat-> GetSeatName (). No formatting occurs. As a result, the colon will be missing the colon ':'. Though a trifle, but a mistake.

It should have been:

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

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

If you did not notice an error, then I suggest. 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 a 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

Testing 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::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 assignment "pos = length ()" will never be executed.

Similar code here: cryfixedstring.h 1297

Pointers

Programmers are very fond of checking the pointer to equality to zero. Now, if they still knew how often they do it wrong. Check when it's too late.

I will give you one example, I will list the rest with a 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 utilized before it was verified against nullptr. 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 is modified while being used twice between sequence points. inetwork.h 2303

"Once" 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 loop body will execute only once. The reason is the unconditional 'break' statement. At the same time, there are no 'continue' statements in the loop.

There are several more of these 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

A member of the 'objtypes' class 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

You need to be careful with 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

Perhaps this is not a mistake, but a dangerous code. If suddenly the symbol '%' appears in the name of the entity, then the results can be very 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

lone expression that does nothing. Error? Incomplete 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 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

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

Other places awaiting 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

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

I wish everyone a codeless code!

PS

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