To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
A Spin-off: CryEngine 3 SDK Checked wit…

A Spin-off: CryEngine 3 SDK Checked with PVS-Studio

March 10, 2014
Author:

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.

0240_CryEngineSDK/image1.png

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 3-rd 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. And 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 you are 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<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 "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 I mentioned about: 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!

Popular related articles
The Ultimate Question of Programming, Refactoring, and Everything

Date: 04.14.2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
The way static analyzers fight against false positives, and why they do it

Date: 03.20.2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: 10.22.2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
PVS-Studio ROI

Date: 01.30.2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
The Evil within the Comparison Functions

Date: 05.19.2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
Free PVS-Studio for those who develops open source projects

Date: 12.22.2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
Static analysis as part of the development process in Unreal Engine

Date: 06.27.2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.31.2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: 11.21.2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept