В мае 2016 года немецкая компания Crytek решила опубликовать на Github исходный код игрового движка CryEngine V. Игровой движок написан на языке C++ и сразу привлёк внимание как сообщества open-source разработчиков, так и команду разработчиков статического анализатора PVS-Studio, выполняющую проверку качества кода открытых проектов. На CryEngine разных версий сделано много отличных игр от разных игровых студий, и теперь движок стал доступен ещё большему числу разработчиков. Статья содержит обзор ошибок, выявленных с помощью статического анализатора кода.

Введение

CryEngine ^[1] — игровой движок, созданный немецкой компанией Crytek ^[2] в 2002 году и первоначально используемый в шутере от первого лица Far Cry. На CryEngine разных версий сделано много отличных игр от разных игровых студий, которые лицензировали движок: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve и многие другие ^[3]. В марте 2016 года компания Crytek анонсировала выход своего нового движка CryEngine V и вскоре опубликовала исходный код на Github ^[4].

Для проверки открытого исходного кода использовался статический анализатор PVS-Studio ^[5] версии 6.05. Это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Единственный верный способ использования методологии статического анализа — регулярная проверка кода на компьютерах разработчиков и build-сервере. Но для демонстрации возможностей анализатора PVS-Studio мы выполняем разовые проверки open-source проектов ^[6] и пишем обзорные статьи с описанием выявленных ошибок. Впрочем, если проект показался нам интересным, по пришествию одного-двух лет мы можем вновь проверить его. По сути такие повторные проверки ничем не отличаются от разовых, так как в коде накапливается много изменений.

Для проверки выбираются как просто известные и популярные проекты, так и предложенные читателями по электронной почте. Поэтому среди игровых движков CryEngine V далеко не первый. Были проверены следующие движки:

• Unreal Engine 4 (первая проверка ^[7], вторая проверка ^[8], третья проверка ^[9])
• Проверка Godot Engine ^[10]
• Проверка Serious Engine ^[11]
• Проверка X-Ray Engine ^[12]

Проверка Xenko Engine ^[13]Также однажды был проверен CryEngine 3 SDK ^[14].

Особое внимание хочу уделить проверке игрового движка Unreal Engine 4 ^[15]. На примере этого проекта мы смогли в подробностях рассказать о правильном применении методологии статического анализа на реальном проекте: от внедрения анализатора в проект до сведения количества предупреждений до нуля с последующим контролем за появлением ошибок на новом коде. Наша работа с проектом Unreal Engine 4 вылилась в сотрудничество с компанией Epic Games, в рамках которого команда PVS-Studio исправила все предупреждения анализатора в исходном коде движка, и была написана совместная статья о проделанной работе, которая опубликована в Unreal Engine Blog ^[16] (ссылка на русский перевод ^[8]). Также компания Epic Games приобрела лицензию PVS-Studio для самостоятельного контроля качества кода. К аналогичному сотрудничеству мы готовы и с компанией Crytek.

Структура отчёта анализатора

В этой статье я хочу ответить на несколько часто задаваемых вопросов, связанных с количеством предупреждений и ложных срабатываний. Например, — «Сколько процентов составляют ложные срабатывания?» — или — «Почему в таком большом проекте так мало ошибок?».

Для начала, все предупреждения анализатора PVS-Studio делятся на три уровня важности: High, Medium и Low. Так High уровень содержит высоко критичные предупреждения, с наибольшей вероятностью являющиеся реальными ошибками, а Low уровень — предупреждения с низкой критичностью, либо предупреждения с очень высокой вероятностью ложно-позитивного срабатывания. Стоит помнить, что конкретный код ошибки не обязательно привязывает её к определённому уровню важности, а распределение сообщений по группам сильно зависит от контекста, в котором они были сгенерированы.

В проекте CryEngine V предупреждения анализатора общего назначения (General Analysis) распределены по уровням важности следующим образом:

• High: 576 предупреждений;
• Medium: 814 предупреждений,
• Low: 2942 предупреждения.

На рисунке 1 изображено распределение предупреждений по уровням важности на круговой диаграмме.

Рисунок 1 — Распределение предупреждений по уровням важности в процентном отношении

В статью невозможно уместить описание всех предупреждений и показать соответствующие фрагменты кода. Обычно статья содержит 10-40 примеров ошибок с описанием. Некоторые подозрительные места кода приводятся просто списком. Большинство предупреждений остаются нерассмотренными. В лучшем случае, после уведомления разработчиков, они спрашивают полный отчёт анализатора для детального изучения. Горькая правда заключается в том, что в большинстве проверяемых нами проектов, материала для статьи более чем достаточно после просмотра только предупреждений уровня High. И игровой движок CryEngine V — не исключение. На рисунке 2 представлена структура предупреждений уровня High.

Рисунок 2 — Описание предупреждений High уровня

Теперь более подробно о секторах представленной круговой диаграммы:

• Described in the article (6%) — сообщения анализатора, приведённые в статье с фрагментами кода и описанием.
• Presented list (46%) — сообщения анализатора, приведённые в статье списком. Такие предупреждения очень похожи на какой-нибудь случай, который уже подробно описан в статье, поэтому просто указывается информация о предупреждении.
• False Positive (8%) — некоторый процент ложных срабатываний был выписан для доработки анализатора в будущем.
• Skipped (40%) — все остальные сообщения анализатора. В них входят предупреждения, которые не удалось уместить в статье, не являющиеся критичными, не очень интересные, либо встретившиеся на участках кода, в которых сложно разобраться человеку не из команды разработчиков проверяемого проекта. Как показала работа с Unreal Engine 4, на практике такой код «пахнет» и все предупреждения всё равно исправляются.

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

Обидный copy-paste

V501 ^[17] There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z — q2.v.z entitynode.cpp 93

bool
CompareRotation(const Quat& q1, const Quat& q2, float epsilon)
{
  return (fabs_tpl(q1.v.x - q2.v.x) <= epsilon)
      && (fabs_tpl(q1.v.y - q2.v.y) <= epsilon)
      && (fabs_tpl(q2.v.z - q2.v.z) <= epsilon) // <=
      && (fabs_tpl(q1.w - q2.w) <= epsilon);
}

Опечатка в одной цифре, пожалуй, одна из самых обидных опечаток, которую может допустить программист. В этой функции анализатор обнаружил подозрительное выражение (q2.v.z — q2.v.z), в котором с большой вероятностью перепутали переменные q1 и q2.

V501 There are identical sub-expressions '(m_eTFSrc == eTF_BC6UH)' to the left and to the right of the '||' operator. texturestreaming.cpp 919

//! Texture formats.
enum ETEX_Format : uint8
{
  ....
  eTF_BC4U,     //!< 3Dc+.
  eTF_BC4S,
  eTF_BC5U,     //!< 3Dc.
  eTF_BC5S,
  eTF_BC6UH,
  eTF_BC6SH,
  eTF_BC7,
  eTF_R9G9B9E5,
  ....
};

bool CTexture::StreamPrepare(CImageFile* pIM)
{
  ....
  if ((m_eTFSrc == eTF_R9G9B9E5) ||
      (m_eTFSrc == eTF_BC6UH) ||     // <=
      (m_eTFSrc == eTF_BC6UH))       // <=
  {
    m_cMinColor /= m_cMaxColor.a;
    m_cMaxColor /= m_cMaxColor.a;
  }
  ....
}

Другой тип опечаток связан с копированием констант. В данном случае переменная m_eTFSrc два раза сравнивается с константой eTF_BC6UH, на месте которой должна быть любая другая, например, отличающая всего одной буквой — константа eTF_BC6SH.

Ещё два похожих места в проекте:

• V501 There are identical sub-expressions '(td.m_eTF == eTF_BC6UH)' to the left and to the right of the '||' operator. texture.cpp 1214
• V501 There are identical sub-expressions 'geom_colltype_solid' to the left and to the right of the '|' operator. attachmentmanager.cpp 1004

V517 ^[18] The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 266, 268. d3dhwshader.cpp 266

int SD3DShader::Release(EHWShaderClass eSHClass, int nSize)
{
  ....
  if (eSHClass == eHWSC_Pixel)
    return ((ID3D11PixelShader*)pHandle)->Release();
  else if (eSHClass == eHWSC_Vertex)
    return ((ID3D11VertexShader*)pHandle)->Release();
  else if (eSHClass == eHWSC_Geometry)                   // <=
    return ((ID3D11GeometryShader*)pHandle)->Release();  // <=
  else if (eSHClass == eHWSC_Geometry)                   // <=
    return ((ID3D11GeometryShader*)pHandle)->Release();  // <=
  else if (eSHClass == eHWSC_Hull)
    return ((ID3D11HullShader*)pHandle)->Release();
  else if (eSHClass == eHWSC_Compute)
    return ((ID3D11ComputeShader*)pHandle)->Release();
  else if (eSHClass == eHWSC_Domain)
    return ((ID3D11DomainShader*)pHandle)->Release()
  ....
}

Вот пример ленивого копирования каскада условных операторов, в одном из случаев которого забыли внести изменения в код.

V517 ^[18] The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 970, 974. environmentalweapon.cpp 970

void CEnvironmentalWeapon::UpdateDebugOutput() const
{
  ....
  const char* attackStateName = "None";
  if(m_currentAttackState &                       // <=
     EAttackStateType_EnactingPrimaryAttack)      // <=
  {
    attackStateName = "Primary Attack";
  }
  else if(m_currentAttackState &                  // <=
          EAttackStateType_EnactingPrimaryAttack) // <=
  {
    attackStateName = "Charged Throw";
  }
  ....
}

В предыдущем коде была хоть какая-то вероятность того, что лишнее условие — это результат слишком большого количества копий кода и одна проверка осталась просто лишней. В этом фрагменте кода из-за идентичных условных выражений переменная attackStateName никогда не примет значение «Charged Throw».

V519 ^[19] The 'BlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266

void CCryDXGLDeviceContext::
OMGetBlendState(...., FLOAT BlendFactor[4], ....)
{
  CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);
  if ((*ppBlendState) != NULL)
    (*ppBlendState)->AddRef();
  BlendFactor[0] = m_auBlendFactor[0];
  BlendFactor[1] = m_auBlendFactor[1];
  BlendFactor[2] = m_auBlendFactor[2]; // <=
  BlendFactor[2] = m_auBlendFactor[3]; // <=
  *pSampleMask = m_uSampleMask;
}

В коде проекта обнаружилась такая вот функция, в которой из-за опечатки в индексе пропущено заполнение элемента с индексом три: BlendFactor[3]. Это место было бы просто одним из интересных мест с опечаткой, если бы анализатор не обнаружил ещё 2 фрагмента, куда скопировали код с допущенной опечаткой:

V519 ^[19] The 'm_auBlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 904, 905. ccrydxgldevicecontext.cpp 905

void CCryDXGLDeviceContext::
  OMSetBlendState(....const FLOAT BlendFactor[4], ....)
{
  ....
  m_uSampleMask = SampleMask;
  if (BlendFactor == NULL)
  {
    m_auBlendFactor[0] = 1.0f;
    m_auBlendFactor[1] = 1.0f;
    m_auBlendFactor[2] = 1.0f;                   // <=
    m_auBlendFactor[2] = 1.0f;                   // <=
  }
  else
  {
    m_auBlendFactor[0] = BlendFactor[0];
    m_auBlendFactor[1] = BlendFactor[1];
    m_auBlendFactor[2] = BlendFactor[2];         // <=
    m_auBlendFactor[2] = BlendFactor[3];         // <=
  }

  m_pContext->SetBlendColor(m_auBlendFactor[0],
                            m_auBlendFactor[1],
                            m_auBlendFactor[2],
                            m_auBlendFactor[3]);
  m_pContext->SetSampleMask(m_uSampleMask);
  ....
}

Вот то самое место, где продолжают пропускать заполнение элемента с индексом '3'. На мгновение я даже подумал, что в этом был какой-то смысл, но эта мысль меня быстро покинула, когда в конце функции я увидел обращение ко всем четырём элементам массива m_auBlendFactor. Похоже в файле ccrydxgldevicecontext.cpp просто сделали несколько копий кода с опечаткой.

V523 ^[20] The 'then' statement is equivalent to the 'else' statement. d3dshadows.cpp 1410

void CD3D9Renderer::ConfigShadowTexgen(....)
{
  ....
  if ((pFr->m_Flags & DLF_DIRECTIONAL) ||
    (!(pFr->bUseHWShadowMap) && !(pFr->bHWPCFCompare)))
  {
    //linearized shadows are used for any kind of directional
    //lights and for non-hw point lights
    m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist);
  }
  else
  {
    //hw point lights sources have non-linear depth for now
    m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist);
  }
  ....
}

В заключение раздела про copy-paste привожу описание ещё одной интересной ошибки. Независимо от результата условного выражения, значение m_cEF.m_TempVecs[2][Num] всегда вычисляется по одной формуле. Судя по со соседнему коду этого фрагмента, здесь нет опечатки в индексе, в этом условии хотят заполнить именно элемент с индексом '2'. А вот формулу расчёта, скорее всего, хотели использовать разную, но после копирования строки забыли изменить код.

Проблемы с инициализацией

V546 ^[21] Member of a class is initialized by itself: 'eConfigMax(eConfigMax)'. particleparams.h 1013

ParticleParams() :
  ....
  fSphericalApproximation(1.f),
  fVolumeThickness(1.0f),
  fSoundFXParam(1.f),
  eConfigMax(eConfigMax.VeryHigh), // <=
  fFadeAtViewCosAngle(0.f)
{}

Анализатор обнаружил возможную опечатку, когда поле класса инициализируется с использованием собственного значения.

V603 ^[22] The object was created but it is not being used. If you wish to call constructor, 'this->SRenderingPassInfo::SRenderingPassInfo(....)' should be used. i3dengine.h 2589

SRenderingPassInfo()
  : pShadowGenMask(NULL)
  , nShadowSide(0)
  , nShadowLod(0)
  , nShadowFrustumId(0)
  , m_bAuxWindow(0)
  , m_nRenderStackLevel(0)
  , m_eShadowMapRendering(static_cast<uint8>(SHADOW_MAP_NONE))
  , m_bCameraUnderWater(0)
  , m_nRenderingFlags(0)
  , m_fZoomFactor(0.0f)
  , m_pCamera(NULL)
  , m_nZoomInProgress(0)
  , m_nZoomMode(0)
  , m_pJobState(nullptr)
{
  threadID nThreadID = 0;
  gEnv->pRenderer->EF_Query(EFQ_MainThreadList, nThreadID);
  m_nThreadID = static_cast<uint8>(nThreadID);
  m_nRenderFrameID = gEnv->pRenderer->GetFrameID();
  m_nRenderMainFrameID = gEnv->pRenderer->GetFrameID(false);
}
  
SRenderingPassInfo(threadID id)
{
  SRenderingPassInfo(); // <=
  SetThreadID(id);
}

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

В этом коде произойдёт следующее: будет создан новый неименованный объект типа SRenderingPassInfo и тут же разрушен. В результате поля класса остаются неинициализированными. Одним из способов исправления ошибки будет написание отдельной функции инициализации и вызов её из разных конструкторов.

V688 ^[23] The 'm_cNewGeomMML' local variable possesses the same name as one of the class members, which can result in a confusion. terrain_node.cpp 344

void CTerrainNode::Init(....)
{
  ....
  m_nOriginX = m_nOriginY = 0; // sector origin
  m_nLastTimeUsed = 0;         // basically last time rendered

  uint8 m_cNewGeomMML = m_cCurrGeomMML = m_cNewGeomMML_Min ....

  m_pLeafData = 0;

  m_nTreeLevel = 0;
  ....
}

Анализатор обнаружил совпадения имени локальной переменной cNewGeomMML с полем класса. Часто такой код не является ошибкой, но здесь это выглядит очень подозрительно на фоне инициализации других полей класса.

V575 ^[24] The 'memset' function processes '0' elements. Inspect the third argument. crythreadutil_win32.h 294

void EnableFloatExceptions(....)
{
  ....
  CONTEXT ctx;
  memset(&ctx, sizeof(ctx), 0);  // <=
  ....
}

С помощью анализатора была найдена очень интересная ошибка. При вызове функции memset() перепутали переданные аргументы, в результате чего функцию зовут для заполнения 0 байт памяти. Вот как выглядит прототип функции:

void * memset ( void * ptr, int value, size_t num );

Третьим аргументом должен передаваться размер буфера, а вторым — значение, которым необходимо заполнить буфер.

Исправленный вариант:

void EnableFloatExceptions(....)
{
  ....
  CONTEXT ctx;
  memset(&ctx, 0, sizeof(ctx));
  ....
}

V630 ^[25] The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 62

void CBuffer::Execute()
{
  ....
  QuatT * pJointsTemp = static_cast<QuatT*>(
    alloca(m_state.m_jointCount * sizeof(QuatT)));
  ....
}

В коде проекта встречаются места, где с помощью функции alloca() выделяют память для массива объектов. В приведённом коде, при таком способе выделения памяти у объектов класса QuatT не будет вызывать ни конструктор, ни деструктор. Подобный код может привести к работе с неинициализированными переменными и другим ошибкам.

Весь список подозрительных мест:

• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 67
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. posematching.cpp 144
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 280
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 282
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. scriptbind_entity.cpp 6252
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. jobmanager.cpp 1016
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. driverd3d.cpp 5859

V583 ^[26] The '?:' operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 330

ILINE bool InitializePoseAlignerPinger(....)
{
  ....
  chainDesc.offsetMin = Vec3(0.0f, 0.0f, bIsMP ? -1.8f : -1.8f);
  chainDesc.offsetMax = Vec3(0.0f, 0.0f, bIsMP ? +0.75f : +1.f);
  ....
}

В коде проекта встречаются случаи, когда тернарный оператор ?: возвращает одно и то же значение. Возможно, в предыдущем случае так написали для красоты кода, но зачем так сделали в этом фрагменте?

float predictDelta = inputSpeed < 0.0f ? 0.1f : 0.1f; // <=
float dict = angle + predictDelta * ( angle - m_prevAngle) / dt ;

Все такие места, найденные в проекте:

• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 313
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -2.f. posealignerc3.cpp 347
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D11_RTV_DIMENSION_TEXTURE2DARRAY. d3dtexture.cpp 2277
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 255U. renderer.cpp 3389
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D12_RESOURCE_STATE_GENERIC_READ. dx12device.cpp 151
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0.1f. vehiclemovementstdboat.cpp 720

V570 ^[27] The 'runtimeData.entityId' variable is assigned to itself. behaviortreenodes_ai.cpp 1771

void ExecuteEnterScript(RuntimeData& runtimeData)
{
  ExecuteScript(m_enterScriptFunction, runtimeData.entityId);

  runtimeData.entityId = runtimeData.entityId; // <=
  runtimeData.executeExitScriptIfDestructed = true;
}

Подозрительное присваивание переменной самой себе. Разработчикам стоит проверить это место.

Приоритеты операций

V502 ^[28] Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. gpuparticlefeaturespawn.cpp 79

bool HasDuration() { return m_useDuration; }

void CFeatureSpawnRate::SpawnParticles(....)
{
  ....
  SSpawnData& spawn = pRuntime->GetSpawnData(i);
  const float amount = spawn.amount;
  const int spawnedBefore = int(spawn.spawned);
  const float endTime = spawn.delay +
                        HasDuration() ? spawn.duration : fHUGE;
  ....
}

Похоже, в этой функции неверно выполняется замер времени. Приоритет оператора сложения выше, чему тернарного оператора :?, поэтому к spawn.delay сначала прибавляется значение 0 или 1, а в переменную endTime всегда записывается значение spawn.duration или fHUGE. Это довольно распространённая ошибка. Об интересных паттернах ошибок в приоритетах операций, найденных в базе ошибок PVS-Studio, я рассказал в статье: Логические выражения в C/C++. Как ошибаются профессионалы ^[29].

V634 ^[30] The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. model.cpp 336

enum joint_flags
{
  angle0_locked = 1,
  ....
};

bool CDefaultSkeleton::SetupPhysicalProxies(....)
{
  ....
  for (int j = 0; .... ; j++)
  {
    // lock axes with 0 limits range
    m_arrModelJoints[i]....flags |= (....) * angle0_locked << j;
  }
  ....
}

Ещё одна очень интересная ошибка, связанная с приоритетом операций умножения и побитового сдвига. У последнего приоритет выполнения ниже, поэтому всё выражение на каждой итерации умножается на единицу (константа angle0_locked равна единице), что выглядит очень странно.

Скорее всего хотели написать так:

m_arrModelJoints[i]....flags |= (....) * (angle0_locked << j);

Список из 35 подозрительных мест с приоритетом операторов сдвига приведён в файле CryEngine5_V634.txt ^[31].

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

Неопределённое поведение — свойство некоторых языков программирования в определённых ситуациях выдавать результат, зависящий от случайных факторов наподобие состояния памяти или сработавшего прерывания. Другими словами, спецификация не определяет поведение языка в любых возможных ситуациях. Допускать такую ситуацию в программе считается ошибкой, даже если на некотором компиляторе программа успешно выполняется, она не будет кроссплатформенной и может отказать на другой машине, в другой ОС и даже на других настройках компилятора.

V610 ^[32] Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. physicalplaceholder.h 25

#ifndef physicalplaceholder_h
#define physicalplaceholder_h
#pragma once
....
const int NO_GRID_REG = -1<<14;
const int GRID_REG_PENDING = NO_GRID_REG+1;
....

Согласно современному стандарту C++, сдвиг влево отрицательного числа приводит к неопределённому поведению. Кроме этого, в коде CryEngine V есть ещё несколько таких мест:

• V610 Undefined behavior. Check the shift operator '<<'. The left operand '~(TFragSeqStorage(0))' is negative. udpdatagramsocket.cpp 757
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 324
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 350
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 617
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 622
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(0xF))' is negative. d3ddeferredrender.cpp 876
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(0xF))' is negative. d3ddeferredshading.cpp 791
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(1 << 0))' is negative. d3dsprites.cpp 1038

V567 ^[33] Undefined behavior. The 'm_current' variable is modified while being used twice between sequence points. operatorqueue.cpp 105

bool COperatorQueue::Prepare(....)
{
  ++m_current &= 1;
  m_ops[m_current].clear();
  return true;
}

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

Ещё подозрительные места:

• V567 Undefined behavior. The 'itail' variable is modified while being used twice between sequence points. trimesh.cpp 3101
• V567 Undefined behavior. The 'ihead' variable is modified while being used twice between sequence points. trimesh.cpp 3108
• V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1194
• V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1202
• V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1220
• V567 Undefined behavior. The 'm_commandBufferIndex' variable is modified while being used twice between sequence points. xconsole.cpp 180
• V567 Undefined behavior. The 'm_FrameFenceCursor' variable is modified while being used twice between sequence points. ccrydx12devicecontext.cpp 952
• V567 Undefined behavior. The 'm_iNextAnimIndex' variable is modified while being used twice between sequence points. hitdeathreactionsdefs.cpp 192

Разные ошибки в условиях

V579 ^[34] The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 58

bool
operator==(const SComputePipelineStateDescription& other) const
{
  return 0 == memcmp(this, &other, sizeof(this)); // <=
}

В операторе равенства допустили ошибку в вызове функции memcmp(), передав в неё размер указателя, а не размер объекта. Теперь объекты сравниваются по первым нескольким байтам.

Исправленный вариант:

memcmp(this, &other, sizeof(*this));

К сожалению, в проекте есть ещё три таких места, которые стоит проверить:

• V579 The memcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. geomcacherendernode.cpp 286
• V579 The AddObject function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. clipvolumemanager.cpp 145
• V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 34

V640 ^[35] The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. livingentity.cpp 181

CLivingEntity::~CLivingEntity()
{
  for(int i=0;i<m_nParts;i++) {
    if (!m_parts[i].pPhysGeom || ....)
      delete[] m_parts[i].pMatMapping; m_parts[i].pMatMapping=0;
  }
  ....
}

В коде игрового движка я заметил очень много мест, где разработчики пишут по несколько операторов в строчку. Часто это даже не обычные присваивания, а циклы, условия, вызовы функции, а иногда всё это вперемешку (рисунок 3).

Рисунок 3 — Плохое форматирование кода

С таким стилем программирования избежать ошибок на большом объёме практически невозможно. Так, в рассматриваемом случае планировали при определённом условии освобождать память из-под массива объектов и обнулять указатель. Но из-за неправильного форматирования указатель m_parts[i].pMatMapping обнуляется на каждой итерации цикла. Какие негативные последствия это может иметь, я предсказать не могу, но код однозначно настораживает.

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

• V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. physicalworld.cpp 2449
• V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1723
• V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1726

V695 ^[36] Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 538, 540. statobjrend.cpp 540

bool CStatObj::RenderDebugInfo(....)
{
  ....
  ColorB clr(0, 0, 0, 0);
  if (nRenderMats == 1)
    clr = ColorB(0, 0, 255, 255);
  else if (nRenderMats == 2)
    clr = ColorB(0, 255, 255, 255);
  else if (nRenderMats == 3)
    clr = ColorB(0, 255, 0, 255);
  else if (nRenderMats == 4)
    clr = ColorB(255, 0, 255, 255);
  else if (nRenderMats == 5)
    clr = ColorB(255, 255, 0, 255);
  else if (nRenderMats >= 6)          // <=
    clr = ColorB(255, 0, 0, 255);
  else if (nRenderMats >= 11)         // <=
    clr = ColorB(255, 255, 255, 255);
  ....
}

В этом коде программист допустил ошибку, из-за которой цвет ColorB(255, 255, 255, 255) никогда не будет выбран. Сначала значения nRenderMats сравниваются по порядку с числами от 1 до 5, но когда разработчик перешёл к работе с диапазоном значений, то не учёл, что значения больше 11 уже входят в диапазон больше 6, следовательно, последнее условие никогда не выполняется.

Этот каскад условий был полностью скопирован ещё в одно место:

• V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 338, 340. modelmesh_debugpc.cpp 340

V695 ^[36] Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 393, 399. xmlcpb_nodelivewriter.cpp 399

enum eNodeConstants
{
  ....
  CHILDBLOCKS_MAX_DIST_FOR_8BITS = BIT(7) - 1,    // 127
  CHILDBLOCKS_MAX_DIST_FOR_16BITS   = BIT(6) - 1, // 63
  ....
};

void CNodeLiveWriter::Compact()
{
  ....
  if (dist <= CHILDBLOCKS_MAX_DIST_FOR_8BITS) // dist <= 127
  {
    uint8 byteDist = dist;
    writeBuffer.AddData(&byteDist, sizeof(byteDist));
    isChildBlockSaved = true;
  }
  else if (dist <= CHILDBLOCKS_MAX_DIST_FOR_16BITS) // dist <= 63
  {
    uint8 byteHigh = CHILDBLOCKS_USING_MORE_THAN_8BITS | ....);
    uint8 byteLow = dist & 255;
    writeBuffer.AddData(&byteHigh, sizeof(byteHigh));
    writeBuffer.AddData(&byteLow, sizeof(byteLow));
    isChildBlockSaved = true;
  }
  ....
}

Похожую ошибку в условии допустили и в этом фрагменте кода, только теперь управление не получает довольно большой фрагмент кода. Значения констант CHILDBLOCKS_MAX_DIST_FOR_8BITS и CHILDBLOCKS_MAX_DIST_FOR_16BITS имеют такие значения, что второе условие никогда не будет истинным.

V547 ^[37] Expression 'pszScript[iSrcBufPos] != '=='' is always true. The value range of char type: [-128, 127]. luadbg.cpp 716

bool CLUADbg::LoadFile(const char* pszFile, bool bForceReload)
{
  FILE* hFile = NULL;
  char* pszScript = NULL, * pszFormattedScript = NULL;
  ....
  while (pszScript[iSrcBufPos] != ' ' &&
    ....
    pszScript[iSrcBufPos] != '=' &&
    pszScript[iSrcBufPos] != '==' &&  // <=
    pszScript[iSrcBufPos] != '*' &&
    pszScript[iSrcBufPos] != '+' &&
    pszScript[iSrcBufPos] != '/' &&
    pszScript[iSrcBufPos] != '~' &&
    pszScript[iSrcBufPos] != '"')
  {}
  ....
}

В большом условном выражении есть подвыражение, которое всегда истинно. Литерал '==' будет иметь тип int и будет равен 15677. Массив pszScript состоит из элементов типа char. Значение типа char не может быть равно 15677. Именно поэтому выражение pszScript[iSrcBufPos] != '==' всегда истинно.

V734 ^[38] An excessive expression. Examine the substrings "_ddn" and "_ddna". texture.cpp 4212

void CTexture::PrepareLowResSystemCopy(byte* pTexData, ....)
{
  ....
  // make sure we skip non diffuse textures
  if (strstr(GetName(), "_ddn")              // <=
      || strstr(GetName(), "_ddna")          // <=
      || strstr(GetName(), "_mask")
      || strstr(GetName(), "_spec.")
      || strstr(GetName(), "_gloss")
      || strstr(GetName(), "_displ")
      || strstr(GetName(), "characters")
      || strstr(GetName(), "$")
      )
    return;
  ....
}

Функция strstr() ищет первое вхождение указанной подстроки в другой строке и возвращает указатель на первое вхождение подстроки или пустой указатель. Первой ищется подстрока "_ddn", а потом "_ddna", это означает, что условие выполнится, если будет найдена более короткая подстрока. Возможно, код работает не так, как планировал программист. Ну или по крайней мере выражение является избыточным и его можно упростить, удалив лишнюю проверку.

V590 ^[39] Consider inspecting this expression. The expression is excessive or contains a misprint. goalop_crysis2.cpp 3779

void COPCrysis2FlightFireWeapons::ParseParam(....)
{
  ....
  else if (!paused &&
          (m_State == eFP_PAUSED) &&        // <=
          (m_State != eFP_PAUSED_OVERRIDE)) // <=
  ....
}

Условное выражение в функции ParseParam() написано таким образом, что результат не зависит от подвыражения (m_State != eFP_PAUSED_OVERRIDE).

Рассмотрим упрощённый пример:

if ( err == code1 && err != code2)
{
  ....
}

Результат всего условного выражения не зависит от результата подвыражения (err != code2), что хорошо видно при построении таблицы истинности для данного примера (рисунок 4)

Рисунок 4 — Таблица истинности для логического выражения

Сравнение беззнаковых чисел с нулём

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

V547 ^[37] Expression 'm_socket < 0' is always false. Unsigned type value is never < 0. servicenetwork.cpp 585

typedef SOCKET CRYSOCKET;
// Internal socket data
CRYSOCKET m_socket;

bool CServiceNetworkConnection::TryReconnect()
{
  ....
  // Create new socket if needed
  if (m_socket == 0)
  {
    m_socket = CrySock::socketinet();
    if (m_socket < 0)
    {
      ....
      return false;
    }
  }
  ....
}

Подробно хочу остановиться на типе SOCKET, который может быть знаковым и беззнаковым на разных платформах, поэтому для работы с этим типом настоятельно рекомендуется использовать специальные макросы и константы, определённые в стандартных заголовочных файлах.

В кроссплатформенных проектах часто встречаются сравнения со значениями 0 или -1, которые приводят к некорректной интерпретации кода ошибки. Проект CryEngine V не является исключением, хотя кое-где присутствуют правильные проверки, например:

if (m_socket == CRY_INVALID_SOCKET)

но во многих местах по коду используются разные способы проверки.

47 мест, где беззнаковые переменные подозрительно сравниваются с нулём, вынесены в файл CryEngine5_V547.txt ^[40]. Разработчикам желательно проверить указанные места.

Опасные указатели

Диагностическое правило V595 ^[41] находит в коде разыменование указателей, проверка валидности которых выполняется ниже по коду, т.е. уже после использования указателя. На практике эта диагностика находит очень крутые ошибки. Небольшое количество ложных срабатываний возникает из-за того, что проверка указателя выполняется косвенно, т.е. через одну или несколько других переменных, но согласитесь, что и для человека разобраться в таком коде будет весьма непросто. Я приведу три примера срабатывания этой диагностики, которые вызывают особое удивление, как работает такой код, остальные можно посмотреть в файле CryEngine5_V595.txt ^[42].

Пример 1

V595 The 'm_pPartManager' pointer was utilized before it was verified against nullptr. Check lines: 1441, 1442. 3denginerender.cpp 1441

void C3DEngine::RenderInternal(....)
{
  ....
  m_pPartManager->GetLightProfileCounts().ResetFrameTicks();
  if (passInfo.IsGeneralPass() && m_pPartManager)
    m_pPartManager->Update();
  ....
}

Разыменование и проверка указателя m_pPartManager.

Пример 2

V595 The 'gEnv->p3DEngine' pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477

bool CGameSerialize::LoadLevel(....)
{
  ....
  // can quick-load
  if (!gEnv->p3DEngine->RestoreTerrainFromDisk())
    return false;

  if (gEnv->p3DEngine)
  {
    gEnv->p3DEngine->ResetPostEffects();
  }
  ....
}

Разыменование и проверка указателя gEnv->p3DEngine.

Пример 3

V595 The 'pSpline' pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158

void FaceChannel::CleanupKeys(....)
{

  CFacialAnimChannelInterpolator backupSpline(*pSpline);

  // Create the key entries array.
  int numKeys = (pSpline ? pSpline->num_keys() : 0);
  ....
}

Разыменование и проверка указателя pSpline.

Разные предупреждения

V622 ^[43] Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. mergedmeshrendernode.cpp 999

static inline void ExtractSphereSet(....)
{
  ....
  switch (statusPos.pGeom->GetType())
  {
    if (false)
    {
    case GEOM_CAPSULE:
      statusPos.pGeom->GetPrimitive(0, &cylinder);
    }
    if (false)
    {
    case GEOM_CYLINDER:
      statusPos.pGeom->GetPrimitive(0, &cylinder);
    }
    for (int i = 0; i < 2 && ....; ++i)
    {
      ....
    }
    break;
  ....
}

Пожалуй, этот фрагмент кода самый странный из всех, что встречались в проекте CryEngine V. Выбор метки case не зависит от условного оператора if, даже если там if (false). В операторе switch выполняется безусловный переход к метке, если она удовлетворяет выражению switch. Без использования оператора break, с помощью такого кода можно «обходить» выполнение ненужных операторов, но опять же, сопровождать такой неочевидный код будет легко не всем разработчикам. И почему при переходе к меткам GEOM_CAPSULE и GEOM_CYLINDER выполняется один и тот же код?

V510 ^[44] The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_action.cpp 143

typedef CryStringT<char> string;
// The actual fragment name.
string m_fragName;
//! cast to C string.
const value_type* c_str() const { return m_str; }
const value_type* data() const  { return m_str; };
  
void LogError(const char* format, ...) const
{ .... }
  
void QueueAction(const UpdateContext& context)
{
  ....
  ErrorReporter(*this, context).LogError("....'%s'", m_fragName);
  ....
}

Если в описании функции невозможно указать число и типы всех допустимых параметров, то список формальных параметров завершается эллипсисом (...), что означает: «и, возможно, еще несколько аргументов». В качестве фактического параметра для эллипсиса могут выступать только POD (Plain Old Data) типы. Если эллипсис функции в качестве параметра передается объект класса, то практически всегда свидетельствует о наличии ошибки в программе. Вместо указателя на строку в стек попадает содержимое объекта. Такой код приведет к формированию в буфере «абракадабры» или к аварийному завершению программы. В коде CryEngine V используется свой класс строки, и у него уже есть подходящий метод c_str().

Исправленный вариант:

LogError("....'%s'", m_fragName.c_str();

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

• V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339
• V510 The 'Format' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648
• V510 The 'CryWarning' function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324
• V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333
• V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864
• V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931
• V510 The 'Format' function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727

V529 ^[45] Odd semicolon ';' after 'for' operator. boolean3d.cpp 1314

int CTriMesh::Slice(....)
{
  ....
  bop_meshupdate *pmd = new bop_meshupdate, *pmd0;
  pmd->pMesh[0]=pmd->pMesh[1] = this;  AddRef();AddRef();
  for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // <=
    pmd0->next = pmd;
  ....
}

Очень подозрительный фрагмент кода. После цикла for поставили точку с запятой, хотя форматирование кода подразумевает о наличии тела у цикла.

V535 ^[46] The variable 'j' is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490

void CPhysicalWorld::SimulateExplosion(....)
{
  ....
  for(j=0;j<pmd->nIslands;j++)                 // <= line 3447
  {
    ....
    for(j=0;j<pcontacts[ncont].nborderpt;j++)  // <= line 3490
    {
  ....
}

Код проекта полон и другим опасным кодом, например, использованием одного счётчика во вложенном и внешнем циклах. Большие файлы с исходным кодом содержат запутанное форматирование и изменение одних переменных в разных местах — без статического анализа здесь не обойтись!

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

• V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1630, 1683. entity.cpp 1683
• V535 The variable 'i1' is being used for this loop and for the outer loop. Check lines: 1521, 1576. softentity.cpp 1576
• V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2315, 2316. physicalentity.cpp 2316
• V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1288, 1303. shadercache.cpp 1303

V539 ^[47] Consider inspecting iterators which are being passed as arguments to function 'erase'. frameprofilerender.cpp 1090

float CFrameProfileSystem::RenderPeaks()
{
  ....
  std::vector<SPeakRecord>& rPeaks = m_peaks;
  
  // Go through all peaks.
  for (int i = 0; i < (int)rPeaks.size(); i++)
  {
    ....
    if (age > fHotToColdTime)
    {
      rPeaks.erase(m_peaks.begin() + i); // <=
      i--;
    }
  ....
}

Анализатор посчитал, что в функцию, выполняющую действие с контейнером, передаётся итератор от другого контейнера. В этом фрагменте кода это не так и ошибки нет. Переменная rPeaks является ссылкой на m_peaks. Однако такой код может вводить в заблуждение не только анализатор кода, но и людей, которые будут сопровождать код. Не стоит так писать.

V713 ^[48] The pointer pCollision was utilized in the logical expression before it was verified against nullptr in the same logical expression. actiongame.cpp 4235

int CActionGame::OnCollisionImmediate(const EventPhys* pEvent)
{
  ....
  else if (pMat->GetBreakability() == 2 &&
   pCollision->idmat[0] != pCollision->idmat[1] &&
   (energy = pMat->GetBreakEnergy()) > 0 &&
   pCollision->mass[0] * 2 > energy &&
   ....
   pMat->GetHitpoints() <= FtoI(min(1E6f, hitenergy / energy)) &&
   pCollision) // <=
    return 0;
  ....
}

Оператор if содержит довольно большое условное выражение, в котором везде выполняется обращение к указателю pCollision. Ошибка заключается в том, что на равенство нулю указатель pCollision проверяется самым последним, т.е. уже после многократного разыменовывания.

V758 ^[49] The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 274

typedef std::shared_ptr<....> CDeviceGraphicsCommandListPtr;

CDeviceGraphicsCommandListPtr
CDeviceObjectFactory::GetCoreGraphicsCommandList() const
{
  return m_pCoreCommandList;
}

void CRenderItemDrawer::DrawCompiledRenderItems(....) const
{
  ....
  {
    auto& RESTRICT_REFERENCE commandList = *CCryDeviceWrapper::
      GetObjectFactory().GetCoreGraphicsCommandList();

    passContext....->PrepareRenderPassForUse(commandList);
  }
  ....
}

В переменную commandList сохраняется ссылка на значение, которое хранит умный указатель. При разрушении объекта умным указателем ссылка становится недействительной.

Ещё несколько таких мест:

• V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 384
• V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 368
• V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 485
• V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 553

Заключение

Исправление ошибок во время написания кода почти ничего не стоит, в отличии от тех, которые находятся на этапе работы тестировщиков, а ошибки в выпущенном продукте несут уже колоссальные расходы. Независимо от используемого анализатора, сама методология статического анализа кода уже давно показала себя как очень эффективный способ контроля качества кода и программного продукта в целом.

Сотрудничество с Epic Games продемонстрировало пользу от внедрения статического анализатора в Unreal Engine 4 ^[16]. Мы помогли разработчикам во всех вопросах интеграции анализатора и даже исправили найденные ошибки, чтобы они могли регулярно проверять новый код проекта. К аналогичному сотрудничеству мы готовы и с компанией Crytek.

Предлагаю всем желающим попробовать PVS-Studio ^[5] на своих C/C++/C# проектах.

Разработчики CryEngineV заранее были уведомлены о проверке проекта, поэтому некоторые ошибки могут быть уже исправлены.