В мае 2016 года немецкая компания Crytek решила опубликовать на GitHub исходный код игрового движка CryEngine V. Проект находится в стадии активной разработки, что влечёт за собой появление множества ошибок в коде. Мы уже проверяли проект с помощью PVS-Studio для Windows, а теперь смогли проверить проект с помощью PVS-Studio для Linux. Материала снова набралось на большую статью с описанием только очень серьёзных ошибок.
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.14 для Linux. Теперь разработчикам кроссплатформенных проектов стало ещё удобнее следить за качеством кода с помощью одного инструмента анализа кода. Скачать версию для Linux можно в виде архива или пакета для пакетного менеджера. Для большинства дистрибутивов можно настроить установку и обновление, используя наш репозиторий.
В статью вошли предупреждения общего назначения и только уровня достоверности "High" (есть ещё Medium и Low). Честно говоря, я не осилил досмотреть внимательно и все "High" предупреждения, т.к. почти сразу насобирал ошибок для статьи при быстром просмотре. За проект я брался несколько раз и долго не мог найти время написать обзорную статью, поэтому про приведённые баги могу сказать, что они живут в коде уже не один месяц. Также некоторые ошибки не исправили из предыдущей статьи о проверке проекта.
Скачать и проверить исходный код в Linux было очень просто. Вот список всех необходимых команд:
mkdir ~/projects && cd ~/projects
git clone https://github.com/CRYTEK/CRYENGINE.git
cd CRYENGINE/
git checkout main
chmod +x ./download_sdks.py
./download_sdks.py
pvs-studio-analyzer trace -- \
sh ./cry_waf.sh build_linux_x64_clang_profile -p gamesdk
pvs-studio-analyzer analyze \
-l /path/to/PVS-Studio.lic \
-o ~/projects/CRYENGINE/cryengine.log \
-r ~/projects/CRYENGINE/ \
-C clang++-3.8 -C clang-3.8 \
-e ~/projects/CRYENGINE/Code/SDKs \
-j4
plog-converter -a GA:1,2 -t tasklist \
-o ~/projects/CRYENGINE/cryengine_ga.tasks \
~/projects/CRYENGINE/cryengine.logФайл отчёта cryengine_ga.tasks можно открыть и просмотреть в QtCreator. Что же удалось найти в исходном коде CryEngine V?
V501 There are identical sub-expressions to the left and to the right of the '==' operator: bActive == bActive LightEntity.h 124
void SetActive(bool bActive)
{
if (bActive == bActive)
return;
m_bActive = bActive;
OnResetState();
}Из-за опечатки функция ничего не делает. Мне кажется, если бы был конкурс "Мисс Опечатка", то этот фрагмент кода точно бы занял первое место. Думаю, эта ошибка имеет все шансы попасть в раздел "C/C++ bugs of the month".
Но это ещё не всё, вот функция из другого класса:
V501 There are identical sub-expressions 'm_staticObjects' to the left and to the right of the '||' operator. FeatureCollision.h 66
class CFeatureCollision : public CParticleFeature
{
public:
CRY_PFX2_DECLARE_FEATURE
public:
CFeatureCollision();
....
bool IsActive() const { return m_terrain ||
m_staticObjects ||
m_staticObjects; }
....
bool m_terrain;
bool m_staticObjects;
bool m_dynamicObjects;
};Тут в функции IsActive() два раза используется переменная m_staticObjects, хотя рядом есть неиспользуемая переменная m_dynamicObjects. Возможно, в коде хотели использовать именно её.
V547 Expression 'outArrIndices[i] < 0' is always false. Unsigned type value is never < 0. CGFLoader.cpp 881
static bool CompactBoneVertices(....,
DynArray<uint16>& outArrIndices, ....) // <= uint16
{
....
outArrIndices.resize(3 * inFaceCount, -1);
int outVertexCount = 0;
for (int i = 0; i < verts.size(); ++i)
{
....
outArrIndices[....] = outVertexCount - 1;
}
// Making sure that the code above has no bugs // <= LOL
for (int i = 0; i < outArrIndices.size(); ++i)
{
if (outArrIndices[i] < 0) // <= LOL
{
return false;
}
}
return true;
}Эта ошибка достойна отдельного раздела. Вообще, в коде CryEngine ну очень много мест, где беззнаковые переменные бессмысленно сравнивают с нулём. Таких мест сотни, но этому фрагменту хочется уделить особое внимание, т.к. код писался намерено.
Так вот, есть массив беззнаковых чисел - outArrIndices. Далее массив заполняется по некому алгоритму. После чего выполняется гениальная проверка каждого элемента массива, чтобы среди них не было ни одного отрицательного числа. Элементы массива имеют тип uint16.
V512 A call of the 'memcpy' function will lead to underflow of the buffer 'hashableData'. GeomCacheRenderNode.cpp 285
void CGeomCacheRenderNode::Render(....)
{
....
CREGeomCache* pCREGeomCache = iter->second.m_pRenderElement;
....
uint8 hashableData[] =
{
0, 0, 0, 0, 0, 0, 0, 0,
(uint8)std::distance(pCREGeomCache->....->begin(), &meshData),
(uint8)std::distance(meshData....->....begin(), &chunk),
(uint8)std::distance(meshData.m_instances.begin(), &instance)
};
memcpy(hashableData, pCREGeomCache, sizeof(pCREGeomCache));
....
}Обратите внимание на аргументы функции memcpy(). Объект pCREGeomCache планируют скопировать в массив hashableData, но случайно вычислили с помощью оператора sizeof не размер объекта, а размер указателя. Из-за ошибки объект копируется не полностью, а всего 4 или 8 байт.
V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'this' class object. ClipVolumeManager.cpp 145
void
CClipVolumeManager::GetMemoryUsage(class ICrySizer* pSizer) const
{
pSizer->AddObject(this, sizeof(this));
for (size_t i = 0; i < m_ClipVolumes.size(); ++i)
pSizer->AddObject(m_ClipVolumes[i].m_pVolume);
}Похожую ошибку допустили, вычислив вместо размера класса, размер указателя this. Правильный вариант: sizeof(*this).
V530 The return value of function 'release' is required to be utilized. ClipVolumes.cpp 492
vector<unique_ptr<CFullscreenPass>> m_jitteredDepthPassArray;
void CClipVolumesStage::PrepareVolumetricFog()
{
....
for (int32 i = 0; i < m_jitteredDepthPassArray.size(); ++i)
{
m_jitteredDepthPassArray[i].release();
}
m_jitteredDepthPassArray.resize(depth);
for (int32 i = 0; i < depth; ++i)
{
m_jitteredDepthPassArray[i] = CryMakeUnique<....>();
m_jitteredDepthPassArray[i]->SetViewport(viewport);
m_jitteredDepthPassArray[i]->SetFlags(....);
}
....
}Если обратиться к документации по классу std::unique_ptr, то функция release() должна использоваться примерно следующим образом:
std::unique_ptr<Foo> up(new Foo());
Foo* fp = up.release();
delete fp;Скорее всего, в данном фрагменте кода хотели использовать функцию reset() вместо release().
V549 The first argument of 'memcpy' function is equal to the second argument. ObjectsTree_Serialize.cpp 1135
void COctreeNode::LoadSingleObject(....)
{
....
float* pAuxDataDst = pObj->GetAuxSerializationDataPtr(....);
const float* pAuxDataSrc = StepData<float>(....);
memcpy(pAuxDataDst, pAuxDataDst, min(....) * sizeof(float));
....
}Забыли передать pAuxDataSrc в функцию memcpy(). Вместо этого в качестве источника и приёмника используют одну и ту же переменную pAuxDataDst. Никто не застрахован от опечаток.
V501 There are identical sub-expressions to the left and to the right of the '||' operator: val == 0 || val == - 0 XMLCPB_AttrWriter.cpp 363
void CAttrWriter::PackFloatInSemiConstType(float val, ....)
{
uint32 type = PFSC_VAL;
if (val == 0 || val == -0) // <=
type = PFSC_0;
else if (val == 1)
type = PFSC_1;
else if (val == -1)
type = PFSC_N1;
....
}Разработчики планировали сравнить вещественную переменную val с нулём и отрицательным нулём (signed zero / negative zero), но сделали это неправильно. Объявив целочисленные константы, значения нулей стали одинаковыми.
Скорее всего, код стоит исправить следующим образом, объявив константы вещественного типа:
if (val == 0.0f || val == -0.0f)
type = PFSC_0;С другой стороны, условное выражение является избыточным, т.к. переменную достаточно сравнивать с обычным нулём. По этой причине оригинальный код выполняется так, как ожидает программист.
Но если необходимо идентифицировать именно отрицательный ноль, то правильно это сделать с помощью функции std::signbit.
V501 There are identical sub-expressions 'm_joints[i].limits[1][j]' to the left and to the right of the '-' operator. articulatedentity.cpp 1326
int CArticulatedEntity::Step(float time_interval)
{
....
for (j=0;j<3;j++) if (!(m_joints[i].flags & angle0_locked<<j)&&
isneg(m_joints[i].limits[0][j]-m_joints[i].qext[j]) +
isneg(m_joints[i].qext[j]-m_joints[i].limits[1][j]) +
isneg(m_joints[i].limits[1][j]-m_joints[i].limits[1][j]) < 2)
{
....
}В последней части условного выражения присутствует вычитание из переменной m_joints[i].limits[1][j] самой себя. Код выглядит подозрительным. В выражении много индексов, возможно, в одном из них ошибка.
Ещё одно такое место:
V590 Consider inspecting this expression. The expression is excessive or contains a misprint. GoalOp_Crysis2.cpp 3779
void COPCrysis2FlightFireWeapons::ParseParam(....)
{
....
bool paused;
value.GetValue(paused);
if (paused && (m_State != eFP_PAUSED) &&
(m_State != eFP_PAUSED_OVERRIDE))
{
m_NextState = m_State;
m_State = eFP_PAUSED;
m_PausedTime = 0.0f;
m_PauseOverrideTime = 0.0f;
}
else if (!paused && (m_State == eFP_PAUSED) && // <=
(m_State != eFP_PAUSED_OVERRIDE)) // <=
{
m_State = m_NextState;
m_NextState = eFP_STOP;
m_PausedTime = 0.0f;
m_PauseOverrideTime = 0.0f;
}
....
}Условное выражение написано таким образом, что результат не зависит от подвыражения m_State != eFP_PAUSED_OVERRIDE. Хотя кому я рассказываю, этот фрагмент кода разработчики так и не исправили после первой статьи.
Если кому интересно, то подобную разновидность ошибок я уже описывал ранее в отдельной статье "Логические выражения в C/C++. Как ошибаются профессионалы".
V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1077
int CTriMesh::Slice(...)
{
....
pmd->pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef();
for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // <=
pmd0->next = pmd;
....
}Ещё один фрагмент кода, неисправленный с момента предыдущей проверки. По коду по-прежнему не ясно: тут ошибочное форматирование или логическая ошибка?
V522 Dereferencing of the null pointer 'pCEntity' might take place. BreakableManager.cpp 2396
int CBreakableManager::HandlePhysics_UpdateMeshEvent(....)
{
CEntity* pCEntity = 0;
....
if (pmu && pSrcStatObj && GetSurfaceType(pSrcStatObj))
{
....
if (pEffect)
{
....
if (normal.len2() > 0)
pEffect->Spawn(true, pCEntity->GetSlotWorldTM(...); // <=
}
}
....
if (iForeignData == PHYS_FOREIGN_ID_ENTITY)
{
pCEntity = (CEntity*)pForeignData;
if (!pCEntity || !pCEntity->GetPhysicalProxy())
return 1;
}
....
}Анализатор обнаружил разыменование нулевого указателя. Код функции написан или отрефакторен таким образом, что образовалась ветка кода, в которой будет использован указатель pCEntity, инициализированный нулём.
Теперь рассмотрим вариант потенциального разыменования нулевого указателя.
V595 The 'pTrack' pointer was utilized before it was verified against nullptr. Check lines: 60, 61. AudioNode.cpp 60
void CAudioNode::Animate(SAnimContext& animContext)
{
....
const bool bMuted = gEnv->IsEditor() && (pTrack->GetFlags() &
IAnimTrack::eAnimTrackFlags_Muted);
if (!pTrack || pTrack->GetNumKeys() == 0 ||
pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Disabled)
{
continue;
}
....
}Автор этого фрагмента кода сначала воспользовался указателем pTrack, а на следующей же строке кода проверяет его валидность перед разыменованием. Скорее всего, это потенциальное место некорректной работы программы.
Предупреждений V595 достаточно много, и в статью они не поместятся. Очень часто такой код является настоящей ошибкой, но благодаря везению, работает корректно.
V571 Recurring check. The 'if (rLightInfo.m_pDynTexture)' condition was already verified in line 69. ObjMan.cpp 70
// Safe memory helpers
#define SAFE_RELEASE(p){ if (p) { (p)->Release(); (p) = NULL; } }
void CObjManager::UnloadVegetationModels(bool bDeleteAll)
{
....
SVegetationSpriteLightInfo& rLightInfo = ....;
if (rLightInfo.m_pDynTexture)
SAFE_RELEASE(rLightInfo.m_pDynTexture);
....
}В этом фрагменте кода нет серьёзной ошибки, но не стоит писать лишний код, если в специальный макрос уже включены соответствующие проверки.
Ещё одно место с лишним кодом:
V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. SystemInit.cpp 4045
class CLvlRes_finalstep : public CLvlRes_base
{
....
for (;; )
{
if (*p == '/' || *p == '\\' || *p == 0)
{
char cOldChar = *p;
*p = 0; // create zero termination
_finddata_t fd;
bool bOk = FindFile(szFilePath, szFile, fd);
if (bOk)
assert(strlen(szFile) == strlen(fd.name));
*p = cOldChar; // get back the old separator
if (!bOk)
return;
memcpy((void*)szFile, fd.name, strlen(fd.name)); // <=
if (*p == 0)
break;
++p;
szFile = p;
}
else ++p;
}
....
}Возможно, в этом коде допущена ошибка. При копировании строки теряется последний терминальный ноль. В данном случае, необходимо копировать strlen() + 1 символ, либо использовать специальные функции для копирования строк: strcpy или strcpy_s.
V521 Such expressions using the ',' operator are dangerous. Make sure the expression '!sWords[iWord].empty(), iWord ++' is correct. TacticalPointSystem.cpp 3243
bool CTacticalPointSystem::Parse(....) const
{
string sInput(sSpec);
const int MAXWORDS = 8;
string sWords[MAXWORDS];
int iC = 0, iWord = 0;
for (; iWord < MAXWORDS; !sWords[iWord].empty(), iWord++) // <=
{
sWords[iWord] = sInput.Tokenize("_", iC);
}
....
}Обратите внимание на секцию цикла for с счётчиками. Что там делает логическое выражение? Скорее всего, его стоит перенести в другое место и написать так:
for (; iWord < MAXWORDS && !sWords[iWord].empty(); iWord++) {...}V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. HommingSwarmProjectile.cpp 187
void CHommingSwarmProjectile::HandleEvent(....)
{
....
explodeDesc.normal = -pCollision->n,pCollision->vloc[0];
....
}Ещё один странный фрагмент кода с оператором ','.
V571 Recurring check. The 'if (pos == npos)' condition was already verified in line 1530. CryString.h 1539
//! Find last single character.
// \return -1 if not found, distance from beginning otherwise.
template<class T>
inline typename CryStringT<T>::....::rfind(....) const
{
const_str str;
if (pos == npos)
{
// find last single character
str = _strrchr(m_str, ch);
// return -1 if not found, distance from beginning otherwise
return (str == NULL) ?
(size_type) - 1 : (size_type)(str - m_str);
}
else
{
if (pos == npos)
{
pos = length();
}
if (pos > length())
{
return npos;
}
value_type tmp = m_str[pos + 1];
m_str[pos + 1] = 0;
str = _strrchr(m_str, ch);
m_str[pos + 1] = tmp;
}
return (str == NULL) ?
(size_type) - 1 : (size_type)(str - m_str);
}Анализатор обнаружил повторную проверку переменной pos. Из-за такой ошибки часть кода никогда не выполняется. Также в функции есть дублирующийся код, поэтому функцию стоит переписать получше.
Такой код благополучно продублировали ещё в одном месте:
V523 The 'then' statement is equivalent to the 'else' statement. ScriptTable.cpp 789
bool CScriptTable::AddFunction(const SUserFunctionDesc& fd)
{
....
char sFuncSignature[256];
if (fd.sGlobalName[0] != 0)
cry_sprintf(sFuncSignature, "%s.%s(%s)", fd.sGlobalName,
fd.sFunctionName, fd.sFunctionParams);
else
cry_sprintf(sFuncSignature, "%s.%s(%s)", fd.sGlobalName,
fd.sFunctionName, fd.sFunctionParams);
....
}Независимо от содержимого строки, её всё равно пытаются распечатать одинаковым образом. Такого кода тоже очень много в проекте, вот некоторые предупреждения:
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. physicalplaceholder.h 25
class CPhysicalEntity;
const int NO_GRID_REG = -1<<14;
const int GRID_REG_PENDING = NO_GRID_REG+1;
const int GRID_REG_LAST = NO_GRID_REG+2;Анализатор умеет находить несколько типов ошибок, приводящих к неопределённому поведению. Согласно современному стандарту языка, сдвиг отрицательного числа влево приводит к неопределённому поведению.
Вот ещё несколько сомнительных мест:
Другой тип неопределённого поведения связан с неоднократным изменением переменной между двумя точками следования:
V567 Undefined behavior. The 'm_current' variable is modified while being used twice between sequence points. OperatorQueue.cpp 101
boolCOperatorQueue::Prepare(....)
{
++m_current &= 1;
m_ops[m_current].clear();
return true;
}К сожалению, такое место не единственное в коде:
В коде CryEngine V я нашёл интересный способ общения разработчиков через комментарии в коде.
Вот самый забавный комментарий, который я нашёл с помощью предупреждения:
V763 Parameter 'enable' is always rewritten in function body before being used.
void CNetContext::EnableBackgroundPassthrough(bool enable)
{
SCOPED_GLOBAL_LOCK;
// THIS IS A TEMPORARY HACK TO MAKE THE GAME PLAY NICELY,
// ASK peter@crytek WHY IT'S STILL HERE
enable = false;
....
}Далее я решил с помощью поиска по содержимому файлов поикать подобные тексты и выписал несколько мест:
....
// please ask me when you want to change [tetsuji]
....
// please ask me when you want to change [dejan]
....
//if there are problems with this function, ask Ivo
uint32 numAnims =
pCharacter->GetISkeletonAnim()->GetNumAnimsInFIFO(layer);
if (numAnims)
return pH->EndFunction(true);
....
//ask Ivo for details
//if (pCharacter->GetCurAnimation() &&
// pCharacter->GetCurAnimation()[0] != '\0')
// return pH->EndFunction(pCharacter->GetCurAnimation());
....
/////////////////////////////////////////////////////////////////
// Strange, !do not remove... ask Timur for the meaning of this.
/////////////////////////////////////////////////////////////////
if (m_nStrangeRatio > 32767)
{
gEnv->pScriptSystem->SetGCFrequency(-1); // lets get nasty.
}
/////////////////////////////////////////////////////////////////
// Strange, !do not remove... ask Timur for the meaning of this.
/////////////////////////////////////////////////////////////////
if (m_nStrangeRatio > 1000)
{
if (m_pProcess && (m_pProcess->GetFlags() & PROC_3DENGINE))
m_nStrangeRatio += cry_random(1, 11);
}
/////////////////////////////////////////////////////////////////
....
// tank specific:
// avoid steering input around 0.5 (ask Anton)
....
CryWarning(VALIDATOR_MODULE_EDITOR, VALIDATOR_WARNING,
"....: Wrong edited item. Ask AlexL to fix this.");
....
// If this renders black ask McJohn what's wrong.
glGenerateMipmap(GL_TEXTURE_2D);
....Ну и самый главный вопрос к разработчикам: почему они не пользуются специализированными инструментами для повышения качества кода? Я, конечно, имею в виду PVS-Studio. :)
Еще раз хочу отметить, что в статье приведена только часть ошибок. Я не досмотрел до конца даже предупреждения уровня High. Так что проект ещё ждет тех, кто внимательно проверит его. Я, к сожалению, не могу найти столько времени, меня ждут десятки других проектов.
За много лет разработки статического анализатора кода я пришёл к выводу, что в командах разработчиков с некоторого количества людей уже невозможно писать код без ошибок. Нет, я не против Code Review, но посчитайте сами, сколько времени понадобится руководителю, чтобы сделать обзор кода 10 человек? А на следующий день? А большего числа людей? В таких условиях Code Review необходим при изменении только ключевых компонентов продукта. В больших объёмах такой подход уже крайне неэффективен. Автоматизированная проверка кода статическими анализаторами поможет значительно выправить ситуацию в лучшую сторону. Это не замена существующим тестам, а совсем иной подход к контролю качества кода (к слову, в тестах тоже находятся ошибки с помощью статического анализа кода). Исправление ошибок во время написания кода почти ничего не стоит, в отличии от тех, которые находятся на этапе работы тестировщиков, а ошибки в выпущенном продукте несут уже колоссальные расходы.
Скачать и попробовать PVS-Studio можно по этой ссылке.
Обсудить варианты лицензирования, цены и варианты скидок можно написав нам в поддержку.
Не огорчайте единорога своим кодом...