Святослав Размыслов

03 Авг 2016

Теги:

#Cpp #GameDev

Долгожданная проверка CryEngine V

03 Авг 2016

Автор: Святослав Размыслов

Введение
Структура отчёта анализатора
Результаты проверки
Заключение

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

Введение

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

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

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

Также однажды был проверен CryEngine 3 SDK.

Особое внимание хочу уделить проверке игрового движка Unreal Engine 4. На примере этого проекта мы смогли в подробностях рассказать о правильном применении методологии статического анализа на реальном проекте: от внедрения анализатора в проект до сведения количества предупреждений до нуля с последующим контролем за появлением ошибок на новом коде. Наша работа с проектом Unreal Engine 4 вылилась в сотрудничество с компанией Epic Games, в рамках которого команда PVS-Studio исправила все предупреждения анализатора в исходном коде движка, и была написана совместная статья о проделанной работе, которая опубликована в Unreal Engine Blog (ссылка на русский перевод). Также компания 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%) - некоторый процент ложных срабатываний был выписан для доработки анализатора в будущем.
Other (40%) - все остальные сообщения анализатора. В них входят предупреждения, которые не удалось уместить в статье, не являющиеся критичными, не очень интересные, либо встретившиеся на участках кода, в которых сложно разобраться человеку не из команды разработчиков проверяемого проекта. Как показала работа с Unreal Engine 4, на практике такой код "пахнет" и все предупреждения всё равно исправляются.

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

Обидный copy-paste

V501 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 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 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 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 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 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 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 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 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 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 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 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 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 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++. Как ошибаются профессионалы.

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

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

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

V610 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 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 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 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 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 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 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 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 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 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. Разработчикам желательно проверить указанные места.

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

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

Пример 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 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 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 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 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 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 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 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. Мы помогли разработчикам во всех вопросах интеграции анализатора и даже исправили найденные ошибки, чтобы они могли регулярно проверять новый код проекта. К аналогичному сотрудничеству мы готовы и с компанией Crytek.

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

#Cpp #GameDev