Побочный результат: проверяем CryEngine 3 SDK с помощью PVS-Studio

• CryEngine 3 SDK
• PVS-Studio
• Результаты проверки
  • Двойная проверка
  • Одно и тоже действие при разных условиях
  • Неинициализированная ячейка массива
  • Не там поставили скобку
  • Забыли написать имя функции
  • Бессмысленные и беспощадные проверки
• Указатели
  • Неопределённое поведение
  • Циклы "на один раз"
  • Странные присваивания
  • Нужно быть аккуратным с именами сущностей
  • Одинокий странник
  • Оператор new
• Выводы

Мы закончили сравнивать статические анализаторы кода Cppcheck, PVS-Studio и анализатор встроенный в Visual Studio 2013. В ходе этого было проверено более 10 открытых проектов. И про некоторые из них можно написать статьи. Вот очередная такая статья о результатах проверки проекта CryEngine 3 SDK.

CryEngine 3 SDK

Wikipedia: CryEngine 3 SDK — набор программных инструментов для разработки компьютерных игр на игровом движке CryEngine 3. CryEngine 3 SDK, как и CryEngine 3, разработан и поддерживается немецкой компанией Crytek. CryEngine 3 SDK является проприетарным бесплатным (freeware) для скачивания, установки и использования средством разработки, с помощью которого любой желающий может разрабатывать компьютерные игры и бесплатно распространять их. При коммерческом распространении (продажах) игр, разработанных на CryEngine 3 SDK, необходимо делать лицензионные отчисления в Crytek.

PVS-Studio

Давайте посмотрим, что интересного можно найти в этой библиотеке с помощью анализатора PVS-Studio.

PVS-Studio находит немного больше, если включить 3 уровень предупреждений. Пример:

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

Формально, для распечатки беззнаковой переменной 'fileIdx' надо использовать "%u". Однако сомнительно, что эта переменная достигнет значения больше, чем INT_MAX. Так что на самом деле ошибка никак себя не проявит.

Результаты проверки

Краткий результат - надо пользоваться статическими анализаторами кода. И тогда ошибок в программе станет меньше и мне не о чем будет писать статьи.

Двойная проверка

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

Два раза выполняется проверка "angVel.GetLengthSquared()>0.001f". Одна проверка лишняя. Или это опечатка, за-за которой не проверяется другое значение.

Одно и тоже действие при разных условиях

Фрагмент 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

Мне кажется, что этот код написан с помощью Copy-Paste. И ещё мне кажется, что в этом коде после копирования забыли что-то исправить.

Фрагмент N2. Очень странным образом реализована функция ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass(). Вот это я понимаю ИМЯ!

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

Аналогичные крайне подозрительные места:

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

Неинициализированная ячейка массива

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

Массив 'destructionEvents' состоит из двух элементов. В конструкторе массив хотели инициализировать. Но не получилось.

Не там поставили скобку

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

Редкая и интересная ошибка. Не там поставлена закрывающаяся скобка.

Дело в том, что второй аргумент функции ShouldRecordEvent() является необязательным. Получается, что в начале вызывается функция ShouldRecordEvent(). Затем, оператор запятая ',' возвращает значение, стоящее с права. Условие зависит только от переменной 'pOwnerRaw'.

В общем, чёрте что получилось.

Забыли написать имя функции

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

В этом выражении переменной pMessage присваивается значение currentSeat->GetSeatName(). Никакого форматирования не происходит. Как результат в строке будет отсутствовать двоеточие ':'. Хоть и мелочь, но ошибка.

Должно было быть:

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

Бессмысленные и беспощадные проверки

Фрагмент 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

Если не заметили ошибку, то подсказываю. Строка m_Name.c_str() сравнивается сама с собой. Должно быть написано:

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

Фрагмент N2. Теперь логическая ошибка:

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

Проверка в этом коде не имеет смысла. Приведу аналогию:

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

Условие всегда выполняется.

Фрагмент 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

Фрагмент 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

Присваивание "pos = length()" никогда не будет выполнено.

Аналогичный код здесь: cryfixedstring.h 1297

Указатели

Программисты очень любят проверять указатель на равенство нулю. Вот если бы они ещё знали, как часто они делают это неправильно. Проверяют, когда уже поздно.

Приведу один пример, остальное перечислю списком.

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

Обещанный список из 35 сообщений: CryEngineSDK-595.txt

Неопределённое поведение

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

Циклы "на один раз"

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

Тело цикла будет выполнять только один раз. Причина - безусловный оператор 'break'. При этом, в цикле нет операторов 'continue'.

Есть ещё несколько таких подозрительных циклов:

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

Странные присваивания

Фрагмент N1.

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

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

Фрагмент 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

Члену класса 'objtypes' два раза присваивается значение.

Фрагмент 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

И ещё несколько таких аномальных присваиваний:

• 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

Нужно быть аккуратным с именами сущностей

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

Пожалуй, это не ошибка, но опасный код. Если вдруг в имени сущности встретится символ '%', то результаты могут быть весьма неожиданными.

Одинокий странник

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

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

Одинокое выражение, которое ничего не делает. Ошибка? Недописанный код?

Ещё одно место в коде: recordingsystem.cpp 2671

Оператор new

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

Код устарел. Теперь при ошибки выделения памяти оператор 'new' генерирует исключение.

Другие места, ждущие рефакторинга:

• 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

Выводы

Особенных выводов у меня нет. Зато представляю, сколько всего интересного можно найти не в CryEngine 3 SDK, а в самом движке CryEngine 3.

Желаю всем безбажного кода!