Недавно в сети появилась новость о том, что был открыт исходный код игры Overgrowth. Мы не смогли пройти мимо и проверили его качество с помощью PVS-Studio. Давайте же вместе посмотрим, где больше интересного экшена: в игре или в её исходном коде!
Overgrowth – вышедшая 14 лет назад игра от компании Wolfire Games. Это экшен с видом от 3-го лица, действие которого происходит в мрачном средневековом мире животных с повадками людей. В игре очень интересная система управления и довольно продвинутый ИИ. В ходе прохождения игроку даётся полная свобода передвижений и организации своих действий. Присутствует многопользовательский режим.
Overgrowth построен на движке Phoenix. Он содержит улучшенную модель движения. Бег, прыжки, перекаты, повороты происходят плавно, а также все позы и анимация изменяются в зависимости от окружения, настроения и даже индивидуальности каждого персонажа. На окружающую среду игры действует погода, даже деревья будут расти быстрее под солнечными лучами.
Анонс Overgrowth произошёл 17 сентября 2008 года, окончательная версия игры вышла 16 октября 2017 года.
Так как с момента публикации исходного кода в проект непрерывно коммитят, я зафиксировал его версией: f2a67f7.
Итак, давайте разберём самые интересные предупреждения, которые удалось найти при помощи проверки проекта PVS-Studio.
Начнём, пожалуй, с функции, в которой PVS-Studio выдал два предупреждения на соседние строки кода.
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
....
const unsigned char* heightfieldData = 0;
....
heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
....
delete heightfieldData;
return ....;
}Похоже, что программист, который писал данную функцию, не очень знаком с принципами работы с динамической памятью в С++.
Начнём с предупреждения V773, как наиболее тривиального. Память под указатель worldImporter была выделена при помощи оператора new и по выходу из функции не была освобождена. Это плохая практика, которая приводит к утечке ресурсов. Поправить данный фрагмент кода можно было бы, позвав оператор delete по завершению работы с этим указателем.
Перейдём к предупреждению V611 и буферу heightfieldData. Тут разработчик очистил динамически выделенную память, однако сделал это неправильно. Вместо того, чтоб позвать оператор delete[] для освобождения аллоцированной ранее памяти при помощи оператора new[], он позвал простой delete. Согласно стандарту, такой код явно приведёт к неопределённому поведению, вот ссылка на соответствующий пункт.
Поправленный фрагмент кода:
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
....
const unsigned char* heightfieldData = 0;
....
heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
....
delete worldImporter;
delete[] heightfieldData;
return ....;
}Также проблем с ручной очисткой памяти можно избежать, написав код посовременнее. Например, использовав std::unique_ptr для автоматического освобождения памяти. Такой код будет короче и надёжнее. Он защитит от ошибок неосвобождённой памяти при досрочном выходе из функции:
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
auto worldImporter = std::make_unique<btMultiBodyWorldImporter> ();
....
std::unique_ptr<unsigned char[]> heightfieldData;
....
heightfieldData = std::make_unique_for_overwrite<unsigned char[]>
(width * height * sizeof(btScalar));
....
return ....;
}V772 [CERT-MSC15-C] Calling a 'delete' operator for a void pointer will cause undefined behavior. OVR_CAPI_Util.cpp 380
typedef struct ovrHapticsClip_
{
const void* Samples;
....
} ovrHapticsClip;
....
OVR_PUBLIC_FUNCTION(void) ovr_ReleaseHapticsClip(ovrHapticsClip* hapticsClip)
{
if (hapticsClip != NULL && hapticsClip->Samples != NULL)
{
delete[] hapticsClip->Samples;
....
}
}Применение операторов delete и delete[] для указателя на void ведёт к неопределённому поведению. Чтобы избежать ошибки, нужно явно привести указатель к его фактическому типу при очистке памяти.
Чтобы понять, как глубока проблема, я провёл небольшое исследование. Поле Samples инициализируется только в одном месте и типом uint8_t*. Вот пруф:
.... ovr_GenHapticsFromAudioData(ovrHapticsClip* outHapticsClip, ....)
{
....
uint8_t* hapticsSamples = new uint8_t[hapticsSampleCount];
....
outHapticsClip->Samples = hapticsSamples;
....
}Это говорит об архитектурной ошибке при проектировании класса. Возможно, раньше оно инициализировалось разными типами и это убрали вследствие рефакторинга, а изменить тип поля Samples с void* на uint8_t* забыли.
Разработчикам в любом случае стоит обратить внимание на этот участок кода и поправить его, он странный и ведёт к UB.
V595 [CERT-EXP12-C] The 'ctx' pointer was utilized before it was verified against nullptr. Check lines: 130, 131. ascontext.cpp 130
class ASContext
{
public:
asIScriptContext *ctx;
}
ASContext::ASContext(....)
{
ctx = ....;
ctx->SetUserData(this, 0);
if( ctx == 0 )
{
FatalError("Error","Failed to create the context.");
return;
}
....
}В данном фрагменте кода указатель ctx сначала разыменовывается, а потом проверяется на 0. Это выглядит довольно подозрительно. Если программист опасается, что ctx может быть равен nullptr, то, возможно, его стоит разыменовывать уже после проверки:
ASContext::ASContext(....)
{
ctx = ....;
if( !ctx )
{
FatalError("Error","Failed to create the context.");
return;
}
ctx->SetUserData(this, 0);
....
}V547 Expression 'connect_id_ == - 1' is always true. placeholderobject.cpp 342
class PlaceholderObject
{
private:
int connect_id_;
....
};
ObjectSanityState PlaceholderObject::GetSanity()
{
....
if( .... && connect_id_ == -1)
{
if( connect_id_ == -1)
{
....
}
}
....
}В данном фрагменте кода анализатор заметил избыточную проверку connect_id_ == -1. Она уже была выполнена в условии верхнего уровня, и переменная connect_id_ с этого момента не менялась.
Возможно, в условии, на которое указывает анализатор, должна была проверяться какая-нибудь другая переменная или данная проверка просто избыточна и код можно упростить:
ObjectSanityState PlaceholderObject::GetSanity()
{
....
if( .... && connect_id_ == -1 )
{
....
}
....
}V791 The initial value of the index in the nested loop equals 'i'. Perhaps, 'i + 1' should be used instead. navmeshhintobject.cpp 65
NavmeshHintObject::NavmeshHintObject()
{
....
for( int i = 0; i < 8; i++ )
{
for( int k = i; k < 8; k++ )
{
if( i != k )
{
if(
corners[i][0] == corners[k][0] ||
corners[i][1] == corners[k][1] ||
corners[i][2] == corners[k][2]
)
{
cross_marking.push_back(corners[i]);
cross_marking.push_back(corners[k]);
}
}
}
}
....
}Здесь анализатор выявил неоптимальный цикл. Используется паттерн кода с выполнением некоторых операций для пар элементов массива. При этом анализатор понял, что нет смысла выполнять операцию для пары, состоящей из одного и того же элемента i == j. Данный фрагмент кода можно упростить:
NavmeshHintObject::NavmeshHintObject()
{
....
for( int i = 0; i < 8; i++ )
{
for( int k = i + 1; k < 8; k++ )
{
if(
corners[i][0] == corners[k][0] ||
corners[i][1] == corners[k][1] ||
corners[i][2] == corners[k][2]
)
{
cross_marking.push_back(corners[i]);
cross_marking.push_back(corners[k]);
}
}
}
....
}V561 [CERT-DCL01-C] It's probably better to assign value to 'other_radius_sq' variable than to declare it anew. Previous declaration: scenegraph.cpp, line 2006. scenegraph.cpp 2010
bool SceneGraph::AddDynamicDecal(....)
{
....
float other_radius_sq = ....;
if(....)
{
....
float other_radius_sq = ....;
}
....
}Анализатор обнаружил подозрительный фрагмент кода. Здесь происходит переопределение переменной other_radius_sq. Зачастую появление сущностей с одинаковыми именами — следствие неудачной копипасты.
void TextureData::GetUncompressedData(unsigned char* data)
{
int imageBits = 32;
....
if (imageBits == 8)
{
....
}
else if (imageBits == 24)
{
....
}
....
}Значение переменной imageBits не меняется между инициализацией и проверками. Скорее всего, это не ошибка, просто анализатор выявил странный недописанный или избыточный фрагмент кода. Разработчикам стоит обратить на него внимание!
V769 [CERT-EXP08-C] The 'idx_buffer_offset' pointer in the 'idx_buffer_offset += pcmd->ElemCount' expression equals nullptr. The resulting value is senseless and it should not be used. imgui_impl_sdl_gl3.cpp 138
void ImGui_ImplSdlGL3_RenderDrawLists(ImDrawData* draw_data)
{
const ImDrawIdx* idx_buffer_offset = 0;
....
idx_buffer_offset += pcmd->ElemCount;
....
}Анализатор обнаружил подозрительную операцию сложения, применяемую к нулевому указателю. Указатель далее не используется. Да и нельзя его использовать. В общем, это какой-то бессмысленный код.
Ещё одно похожее срабатывание:
V769 [CERT-EXP08-C] The 'cp' pointer in the 'cp ++' expression equals nullptr. The resulting value is senseless and it should not be used. crn_file_utils.cpp 547
int file_utils::wildcmp(...., const char* pString)
{
const char* cp = NULL;
....
pString = cp++;
....
}Данный фрагмент также похож на последствия рефакторинга или криво реализованный алгоритм. Что имели в виду разработчики — остаётся только гадать...
V523 The 'then' statement is equivalent to the 'else' statement. skeleton.cpp 152
void Skeleton::SetGravity( bool enable )
{
if(enable)
{
for(unsigned i=0; i<physics_bones.size(); i++)
{
if(!physics_bones[i].bullet_object)
{
continue;
}
physics_bones[i].bullet_object->SetGravity(true);
//physics_bones[i].bullet_object->SetDamping(0.0f);
}
}
else
{
for(unsigned i=0; i<physics_bones.size(); i++)
{
if(!physics_bones[i].bullet_object)
{
continue;
}
physics_bones[i].bullet_object->SetGravity(true);
//physics_bones[i].bullet_object->SetDamping(1.0f);
}
}
}В продолжение обзора странного кода. Анализатор обнаружил if с одинаковыми then и else. Скорее всего, разработчик просто не дописал второй фрагмент кода. Об этом можно судить по разным закомментированным фрагментам в ветках кода.
V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. as_compiler.cpp 4317
void asCCompiler::CompileIfStatement(....)
{
bool constructorCall1 = ....;
bool constructorCall2 = ....;
....
if ( (constructorCall1 && !constructorCall2)
||(constructorCall2 && !constructorCall1) )
{
....
}
}Давайте рассмотрим фрагмент кода, который сам по себе не является ошибкой. На самом деле, мне просто очень нравится эта диагностика. Она проста и изящна.
PVS-Studio обнаружил паттерн в проверяемом условии, который можно упростить — и сделать код существенно читабельнее. Программист пытается понять, что произошёл вызов одного или другого конструктора. Операция, которую он применяет очень похожа на XOR. Однако в C++ нет исключающего "ИЛИ" для типа bool, и порой это выливается в гораздо более громоздкий код. Переписать его можно, например, таким образом:
void asCCompiler::CompileIfStatement(....)
{
bool constructorCall1 = ....;
bool constructorCall2 = ....;
....
if (constructorCall1 != constructorCall2)
{
....
}
}V610 [CERT-INT34-C] Undefined behavior. Check the shift operator '<<'. The right operand ('i' = [0..63]) is greater than or equal to the length in bits of the promoted left operand. bitarray.cpp 77
class Bitarray
{
private:
uint64_t *arr;
....
};
void Bitarray::SetBit( size_t index )
{
size_t p = index/64;
size_t i = index%64;
arr[p] |= (1UL << i);
}PVS-Studio обнаружил опасный код со сдвигом влево беззнакового числа. Согласно стандарту, это неопределённое поведение, если правый операнд больше или равен разрядности левого операнда. Литерал 1UL под MSVC представлен 32 битами, правый операнд же лежит в диапазоне от 0 до 63.
Так как код рассчитан в том числе и для сборки под Windows, разработчикам стоит обратить внимание на него. Вот ещё несколько предупреждений с такой же проблемой:
V751 [CERT-MSC13-C] Parameter 'rayTo' is not used inside function body. btSoftBody.cpp 2148
btScalar btSoftBody::RayFromToCaster::rayFromToTriangle(
const btVector3& rayFrom,
const btVector3& rayTo,
const btVector3& rayNormalizedDirection,
const btVector3& a,
const btVector3& b,
const btVector3& c,
btScalar maxt)
{
static const btScalar ceps = -SIMD_EPSILON * 10;
static const btScalar teps = SIMD_EPSILON * 10;
const btVector3 n = btCross(b - a, c - a);
const btScalar d = btDot(a, n);
const btScalar den = btDot(rayNormalizedDirection, n);
if (!btFuzzyZero(den))
{
const btScalar num = btDot(rayFrom, n) - d;
const btScalar t = -num / den;
if ((t > teps) && (t < maxt))
{
const btVector3 hit = rayFrom + rayNormalizedDirection * t;
if ((btDot(n, btCross(a - hit, b - hit)) > ceps) &&
(btDot(n, btCross(b - hit, c - hit)) > ceps) &&
(btDot(n, btCross(c - hit, a - hit)) > ceps))
{
return (t);
}
}
}
return (-1);
}Здесь анализатор говорит о формальном параметре rayTo, который не используется в теле функции. При этом параметр rayFrom используется несколько раз, что может свидетельствовать об ошибке или не очень удачном рефакторинге кода.
В результате проверки анализатор нашёл в проекте ошибки разного рода: традиционные опечатки, довольно много неправильной работы с памятью, логические ошибки. Мы надеемся, что благодаря данной статье, разработчики Overgrowth смогут поправить некоторые недочёты, а может быть, захотят самостоятельно перепроверить свою кодовую базу PVS-Studio. Это безусловно поможет замечательной игре и дальше радовать своих фанатов в уже новых, менее подверженных багам сборках :)