Мы используем куки, чтобы пользоваться сайтом
было удобно.
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
В преддверии выхода игры "Amnesia: Rebirth" издательство "Fractional Games" выложило в открытый доступ исходный код легендарной "Amnesia: The Dark Descent" и её продолжения "Amnesia: A Machine For Pigs". Почему бы и не посмотреть с помощью инструмента статического анализа, насколько страшные ошибки скрывает в себе нутро этих культовых хоррор-игр?
Увидев на Реддите новость о том, что исходный код игр "Amnesia: The Dark Descend" и "Amnesia: A Machine for Pigs" был опубликован, я не могла пройти мимо и не проверить этот код с помощью PVS-Studio, а заодно и написать об этом статью. Особенно с учётом того, что 20 октября выходит (а на момент публикации этой статьи, уже даже вышла) новая часть этой серии: "Amnesia: Rebirth".
"Amnesia: The Dark Descent" вышла в 2010 году и стала культовой игрой в жанре survival horror. Честно говоря, мне ни разу не удавалось пройти её, даже чуть-чуть, так как в хоррор игры я играю по одному алгоритму: поставить, зайти на пять минут, выйти по "alt+f4" при первом же криповом моменте и удалить игру. Но вот смотреть прохождения этой игры на ютубчике мне нравилось.
А на случай, если кто-то ещё не знаком с PVS-Studio, это статический анализатор, который ищет ошибки и подозрительные места в исходном коде программ.
Мне особенно нравится заглядывать в исходный код игр, так что, если вам интересно какие ошибки в нём допускают, можете почитать мои предыдущие статьи. Ну или взглянуть на статьи моих коллег про проверку исходного кода игр.
После проверки, оказалось, что большое количество кода в "The Dark Descend" и "A Machine For Pigs" пересекается, и отчеты для этих двух проектов были очень похожи. Так что почти все ошибки, которые я приведу далее, содержатся в обоих проектах.
И самый большой пласт ошибок из всех, обнаруженных анализатором в этих проектах, был пласт ошибок "копипасты". Отсюда и название статьи. Основной причиной появления этих ошибок является "эффект последней строки".
Ну и перейдём к делу.
Подозрительных мест, похожих на невнимательное копирование, нашлось достаточно много. Некоторые случаи, возможно, как-то и обусловлены внутренней логикой самой игры. Но если уж они смутили и меня, и анализатор, то уж стоило хотя бы предоставить к ним комментарий. Ведь другие разработчики могут оказаться столь же недогадливыми, как и я.
Фрагмент 1.
Начнем с примера, где вся функция состоит из сравнения результатов методов и полей двух объектов aObjectDataA и aObjectDataB. Приведу эту функцию целиком для наглядности. Попробуйте сами заметить, где в функции была допущена ошибка:
static bool SortStaticSubMeshesForBodies(const ....& aObjectDataA,
const ....& aObjectDataB)
{
//Is shadow caster check
if( aObjectDataA.mpObject->GetRenderFlagBit(....)
!= aObjectDataB.mpObject->GetRenderFlagBit(....))
{
return aObjectDataA.mpObject->GetRenderFlagBit(....)
< aObjectDataB.mpObject->GetRenderFlagBit(....);
}
//Material check
if( aObjectDataA.mpPhysicsMaterial != aObjectDataB.mpPhysicsMaterial)
{
return aObjectDataA.mpPhysicsMaterial < aObjectDataB.mpPhysicsMaterial;
}
//Char collider or not
if( aObjectDataA.mbCharCollider != aObjectDataB.mbCharCollider)
{
return aObjectDataA.mbCharCollider < aObjectDataB.mbCharCollider;
}
return aObjectDataA.mpObject->GetVertexBuffer()
< aObjectDataA.mpObject->GetVertexBuffer();
}Картиночка, чтобы случайно не подсмотреть ответ:
Смогли найти ошибку? Да, в последнем return идёт сравнение с использованием aObjectDataA с обеих сторон. Заметьте, что все выражения в оригинальном коде были выписаны в строчку, тут я сделала переносы, чтобы все ровно вписывалось в ширину строки. Представьте, какого будет искать такой недочёт в конце рабочего дня. А анализатор найдёт его сразу же после сборки и запуска инкрементального анализа.
V501 There are identical sub-expressions 'aObjectDataA.mpObject->GetVertexBuffer()' to the left and to the right of the '<' operator. WorldLoaderHplMap.cpp 1123
В итоге, подобная ошибка будет найдена практически в момент написания кода, вместо того чтобы скрыться в недрах кода от нескольких этапов QA, многократно усложнив свой поиск.
Примечание коллеги Андрея Карпова. Да, это классическая "ошибка последней строки". Однако, это ещё и классический паттерн ошибки сравнения двух объектов. См. статью "Зло живёт в функциях сравнения".
Фрагмент 2.
Сразу взглянем на код, который вызвал предупреждение:
Привожу код скриншотом для наглядности.
Само предупреждение выглядит следующим образом:
V501 There are identical sub-expressions 'lType == eLuxJournalState_OpenNote' to the left and to the right of the '||' operator. LuxJournal.cpp 2262
Анализатор обнаружил, что здесь есть ошибка в проверке значения переменной lType. Дважды проверяется равенство с одним и тем же элементом перечислителя eLuxJournalState_OpenNote.
Во-первых, хотелось бы, чтобы такое условие все-таки было записано в "табличном" виде для улучшения читаемости. См. главу N13 в мини-книге "Главный вопрос программирования, рефакторинга и всего такого".
if(!( lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenDiary
|| lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenNarratedDiary))
return false;В таком виде заметить ошибку и без анализатора становится гораздо проще.
Однако возникает вопрос, приводит ли такая ошибочная проверка к искажению логики программы? Ведь возможно нужно проверять и ещё какое-либо значение lType, но проверка было упущена из-за ошибки копипасты. Взглянем на само перечисление:
enum eLuxJournalState
{
eLuxJournalState_Main,
eLuxJournalState_Notes,
eLuxJournalState_Diaries,
eLuxJournalState_QuestLog,
eLuxJournalState_OpenNote,
eLuxJournalState_OpenDiary,
eLuxJournalState_OpenNarratedDiary,
eLuxJournalState_LastEnum,
};Есть всего три значения, которые имеют в своём имени слово "Open". И в проверке присутствуют все три. Скорее всего искажения логики здесь нет, но точно сказать все равно нельзя. Так что анализатор либо нашел логическую ошибку, которую смог бы поправить разработчик игры, либо нашел "некрасиво" написанное место, которое стоило бы переписать для лучшей ясности.
Фрагмент 3.
Следующий случай является вообще самым очевидным примером ошибки копипасты.
V778 Two similar code fragments were found. Perhaps, this is a typo and 'mvSearcherIDs' variable should be used instead of 'mvAttackerIDs'. LuxSavedGameTypes.cpp 615
void cLuxMusicHandler_SaveData::ToMusicHandler(....)
{
....
// Enemies
//Attackers
for(size_t i=0; i<mvAttackerIDs.Size(); ++i)
{
iLuxEntity *pEntity = apMap
->GetEntityByID(mvAttackerIDs[i]);
if(....)
{
....
}
else
{
Warning("....", mvAttackerIDs[i]);
}
}
//Searchers
for(size_t i=0; i<mvSearcherIDs.Size(); ++i)
{
iLuxEntity *pEntity = apMap->GetEntityByID(mvSearcherIDs[i]);
if(....)
{
....
}
else
{
Warning("....", mvAttackerIDs[i]);
}
}
}В первом цикле работа идёт с указателем pEntity, который был получен через mvAttackerIDs, и при невыполнении условия сообщение для отладки выдается для того же mvAttackerIDs. Однако в следующем цикле, который оформлен точно так же, как и предыдущий участок кода, pEntity получается с помощью mvSearcherIDs. А вот предупреждение все еще выдается с упоминанием mvAttackerIDs.
Скорее всего, блок кода с подписью "Searchers" был скопирован с блока "Attackers", mvAttackerIDs был заменен на mvSearcherIDs, но блок else не был изменён. В итоге, в сообщении об ошибке используется элемент не того массива.
На логику игры эта ошибка не влияет, но так можно подложить серьёзную свинью человеку, которому придется заниматься отладкой этого места и впустую потратить время на работу не с тем элементом mvSearcherIDs.
Фрагмент 4.
На следующее подозрительное место анализатор указал аж тремя предупреждениями:
Посмотрим на код:
void __stdcall cLuxScriptHandler::PlaceEntityAtEntity(....)
{
cLuxMap *pMap = gpBase->mpMapHandler->GetCurrentMap();
iLuxEntity *pEntity = GetEntity(....);
if(pEntity == NULL) return;
if(pEntity->GetBodyNum() == 0)
{
....
}
iPhysicsBody *pBody = GetBodyInEntity(....);
if(pBody == NULL) return;
iLuxEntity *pTargetEntity = GetEntity(....);
if(pEntity == NULL) return; // <=
iPhysicsBody *pTargetBody = GetBodyInEntity(....);
if(pTargetBody == NULL) return;
....
}Предупреждение V547 было выдано на вторую проверку pEntity == NULL. Для анализатора эта проверка всегда будет false, так как, если бы это условие было true, то выход из функции произошел бы выше из-за предыдущей аналогичной проверки.
Следующее предупреждение, V649 было выдано как раз-таки на то, что у нас есть две одинаковых проверки. Обычно такой случай может и не быть ошибкой. Вдруг в одной части кода реализуется одна логика, а в другом участке кода по той же проверке должно реализовываться что-то еще. Но в этом случае, тело первой проверки состоит из return, так что до второй проверки, если условие окажется истинным, дело даже и не дойдёт. С помощью отслеживания такой логики анализатор сокращает количество ложных сообщений на подозрительный код и выдаёт их только на совсем странную логику.
Ошибка, на которую указывает последнее предупреждение, по своей сути очень похожа на предыдущий пример. Скорее всего, все проверки дублировались с первой проверки if(pEntity == NULL), а потом проверяемый объект заменялся на необходимый. В случае объектов pBody и pTargetBody замена была произведена, а про объект pTargetEntity забыли. В итоге проверка этого объекта не производится.
В рассматриваемом нами примере, если копнуть код чуть глубже, оказывается, что такая ошибка в целом не повлияет на ход работы программы. Указатель pTargetBody получает свое значение из функции GetBodyInEntity:
iPhysicsBody *pTargetBody = GetBodyInEntity(pTargetEntity,
asTargetBodyName);Первым аргументом здесь как раз передаётся непроверенный ранее указатель, который больше нигде не используется. И благо внутри этой функции есть проверка первого аргумента на NULL:
iPhysicsBody* ....::GetBodyInEntity(iLuxEntity* apEntity, ....)
{
if(apEntity == NULL){
return NULL;
}
....
}Благодаря чему этот код в итоге работает верно, хотя и содержит в себе ошибку.
Фрагмент 5.
И еще одно подозрительное место с копипастой!
В этом методе обнуляются поля объекта класса cLuxPlayer.
void cLuxPlayer::Reset()
{
....
mfRoll=0;
mfRollGoal=0;
mfRollSpeedMul=0; // <=
mfRollMaxSpeed=0; // <=
mfLeanRoll=0;
mfLeanRollGoal=0;
mfLeanRollSpeedMul=0;
mfLeanRollMaxSpeed=0;
mvCamAnimPos =0;
mvCamAnimPosGoal=0;
mfRollSpeedMul=0; // <=
mfRollMaxSpeed=0; // <=
....
}Но по какой-то причине две переменные mfRollSpeedMul и mfRollMaxSpeed зануляются дважды:
Заглянем в сам класс и посмотрим, какие поля в нём есть:
class cLuxPlayer : ....
{
....
private:
....
float mfRoll;
float mfRollGoal;
float mfRollSpeedMul;
float mfRollMaxSpeed;
float mfLeanRoll;
float mfLeanRollGoal;
float mfLeanRollSpeedMul;
float mfLeanRollMaxSpeed;
cVector3f mvCamAnimPos;
cVector3f mvCamAnimPosGoal;
float mfCamAnimPosSpeedMul;
float mfCamAnimPosMaxSpeed;
....
}Интересно, тут есть три аналогичных блока переменных с родственными названиями: mfRoll, mfLeanRoll и mvCamAnimPos. В Reset эти три блока обнуляются, кроме двух последних переменных из третьего блока, mfCamAnimPosSpeedMul и mfCamAnimPosMaxSpeed. И как раз на месте этих двух переменных и находятся продублированные присвоения. Скорее всего, все эти присвоения были скопированы из первого блока присвоений, а потом имена переменных были заменены на нужные.
Может быть, что две пропущенные переменные обнулять было и не надо, но вероятность обратного также весьма велика. Да и повторные присвоения явно не помогут в поддержке этого кода. Как видите, в длинной портянке одинаковых действий можно не заметить такой ошибки и анализатор тут выручает.
Фрагмент 5.5.
Этот пример очень похож на предыдущий. Сразу приведу фрагмент кода и предупреждение анализатора для него.
V519 The 'mfTimePos' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 49, 53. AnimationState.cpp 53
cAnimationState::cAnimationState(....)
{
....
mfTimePos = 0;
mfWeight = 1;
mfSpeed = 1.0f;
mfBaseSpeed = 1.0f;
mfTimePos = 0;
mfPrevTimePos=0;
....
}Переменной mfTimePos дважды было присвоено значение 0. Как и в предыдущем примере, заглянем в объявление этого поля:
class cAnimationState
{
....
private:
....
//Properties of the animation
float mfLength;
float mfWeight;
float mfSpeed;
float mfTimePos;
float mfPrevTimePos;
....
}Можно заметить, что этот блок объявлений соответствует и порядку присвоения в фрагменте кода с ошибкой, как и в предыдущем примере. Здесь в присвоении вместо переменной mfLength значение получает mfTimePos. Но тут уже ошибку не объяснить копированием блока и "эффектом последней строки". Возможно, mfLength и не нужно присваивать новое значение, однако это место все равно остаётся подозрительным.
Фрагмент 6.
На следующий участок кода из "Amnesia: A Machine For Pigs" анализатор выдал целый букет предупреждений. Я приведу лишь часть кода, на который были выданы ошибки одного рода:
void cLuxEnemyMover::UpdateMoveAnimation(float afTimeStep)
{
....
if(prevMoveState != mMoveState)
{
....
//Backward
if(mMoveState == eLuxEnemyMoveState_Backward)
{
....
}
....
//Walking
else if(mMoveState == eLuxEnemyMoveState_Walking)
{
bool bSync = prevMoveState == eLuxEnemyMoveState_Running
|| eLuxEnemyMoveState_Jogging
? true : false;
....
}
....
}
}Где же здесь ошибка?
Анализатор выдал следующие предупреждения:
Цепочка if-else-if в оригинальном коде повторяется, и дальше эти предупреждения как раз выдавались на каждое тело каждого else if.
Рассмотрим строку, на которую указывает анализатор:
bool bSync = prevMoveState == eLuxEnemyMoveState_Running
|| eLuxEnemyMoveState_Jogging
? true : false;Неудивительно, что в такое выражение, которое в оригинале еще и записано в строчку, закралась ошибка. И наверняка вы её уже заметили. Элемент перечисления eLuxEnemyMoveState_Jogging ни с чем не сравнивается, проверке подвергается его значение. Скорее всего, подразумевалось выражение 'prevMoveState == eLuxEnemyMoveState_Jogging'.
Такая ошибка может показаться совсем безобидной. Но в другой своей статье, о проверке Bullet Engine, среди коммитов к проекту я нашла исправление ошибки того же рода, приводившей к тому, что силы применялись к объектам с неверной стороны. А в этом случае эта ошибка была совершена несколько раз. Ну и еще замечу, что тернарное условие совсем бессмысленно, так как выполнено оно будет, в последнюю очередь, к булевым результатам логических операторов.
Фрагмент 7.
И, наконец, последняя пара примеров ошибки копипасты. В этот раз снова в условном операторе. Анализатор выдал предупреждение вот на такой участок кода:
void iParticleEmitter::SetSubDivUV(const cVector2l &avSubDiv)
{
//Check so that there is any subdivision
// and that no sub divison axis is
//equal or below zero
if( (avSubDiv.x > 1 || avSubDiv.x > 1) && (avSubDiv.x >0 && avSubDiv.y >0))
{
....
}
....
}Думаю, в таком отдельном от всего кода фрагменте достаточно легко заметить подозрительное место. Однако эта ошибка сумела скрыться от разработчиков этой игры.
Анализатор выдал следующее предупреждение:
V501 There are identical sub-expressions to the left and to the right of the '||' operator: avSubDiv.x > 1 || avSubDiv.x > 1 ParticleEmitter.cpp 199
По второй скобке в условии видно, что подразумевается проверка полей и x, и y. Но в первой скобке по какой-то причине этот момент был упущен и проверяется лишь поле x. К тому же, судя по комментарию к проверке, оба поля должны были быть проверены. Тут каким-то образом сработал не "эффект последней строки", а скорее первой, так как в первой скобке забыли заменить обращение к полю x на обращение к полю y.
Так что такие ошибки весьма коварны, так как в этом случае разработчику даже не помогло написание пояснительного комментария к условию.
Я бы порекомендовала в таких случаях взять за привычку запись родственных проверок в табличном виде. Так и править легче, и недочёт будет гораздо заметнее:
if( (avSubDiv.x > 1 || avSubDiv.x > 1)
&& (avSubDiv.x > 0 && avSubDiv.y > 0))Фрагмент 7.5.
Абсолютно такая же, по сути, ошибка нашлась совсем в другом месте:
static bool EdgeTriEqual(const cTriEdge &edge1, const cTriEdge &edge2)
{
if(edge1.tri1 == edge2.tri1 && edge1.tri2 == edge2.tri2)
return true;
if(edge1.tri1 == edge1.tri1 && edge1.tri2 == edge2.tri1)
return true;
return false;
}Ну как, получилось сходу увидеть, где она спряталась? Не зря же уже разобрали столько примеров :)
Анализатор выдал такое предупреждение:
V501 There are identical sub-expressions to the left and to the right of the '==' operator: edge1.tri1 == edge1.tri1 Math.cpp 2914
Разберем этот фрагмент по порядку. Очевидно, что в первой проверке проверяется равенство полей edge1.tri1 и edge2.tri2, и в то же время равенство edge1.tri2 и edge2.tri2:
edge1.tri1 -> edge2.tri1
edge1.tri2 -> edge2.tri2А во второй проверке, судя по корректной части проверки 'edge1.tri2 == edge2.tri1' необходимо было проверить равенство этих полей "крест-накрест":
Но вместо проверки на edge1.tri1 == edge2.tri2, была проведена бессмысленная проверка edge1.tri1 == edge1.tri1. А ведь это всё содержание функции, я ничего из неё не удаляла. И все равно такая ошибка затесалась в код.
Фрагмент 1.
Приведу следующий фрагмент кода с оригинальными отступами.
void iCharacterBody::CheckMoveCollision(....)
{
....
/////////////////////////////////////
//Forward velocity reflection
//Make sure that new velocity points in the right direction
//and that it is not too large!
if(mfMoveSpeed[eCharDir_Forward] != 0)
{
vForwardVel = ....;
float fForwardSpeed = vForwardVel.Length();
if(mfMoveSpeed[eCharDir_Forward] > 0)
if(mfMoveSpeed[eCharDir_Forward] > fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = fForwardSpeed;
else
if(mfMoveSpeed[eCharDir_Forward] < fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = -fForwardSpeed;
}
....
}Предупреждение PVS-Studio: V563 It is possible that this 'else' branch must apply to the previous 'if' statement. CharacterBody.cpp 1591
Этот пример может сбить с толку. Почему else имеет тот же отступ, что и внешний по уровню if? Подразумевается, что это else для внешнего условия? Ну тогда нужно расставить скобки, иначе else относится к идущему прямо передним if.
if(mfMoveSpeed[eCharDir_Forward] > 0)
{
if(mfMoveSpeed[eCharDir_Forward] > fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = fForwardSpeed;
}
else if(mfMoveSpeed[eCharDir_Forward] < fForwardSpeed)
{
mfMoveSpeed[eCharDir_Forward] = -fForwardSpeed;
}Или нет? В процессе написания статьи я несколько раз меняла своё мнение насчет того, какой вариант последовательности задуманных действий для этого кода наиболее вероятен.
Если мы вникнем в этот код чуть глубже, то выяснится, что переменная fForwardSpeed, с которой производится сравнение в младших if, не может иметь значение меньше нуля, так как она получает значение из метода Length:
inline T Length() const
{
return sqrt( x * x + y * y + z * z);
}Тогда, скорее всего, суть этих проверок состоит в том, что сначала проверяем, является ли элемент mfMoveSpeed большим, чем ноль, а потом проверяем его значение относительно fForwardSpeed. Тем более, что два последних if соответствуют друг другу по формулировке.
В таком случае, оригинальный код будет работать, как и задумано! Но он точно заставит поломать голову того, кто придёт его править/рефакторить.
Я думала, что мне никогда не встретится код, который выглядит таким образом. Из интереса, я заглянула в нашу базу ошибок, найденных в open-source проектах и описанных в статьях. И примеры этой ошибки находились и в других проектах - можете посмотреть на них и сами.
И, пожалуйста, не пишите так, даже если вам самим все в этом случае ясно. Или фигурные скобки или верная индентация, а лучше и то, и другое. Не повергайте в страдания, тех, кто придёт разбираться в вашем коде, да и себя самих в будущем ;)
Фрагмент 2.
Следующая ошибка меня несколько смутила, и я долго пыталась найти тут логику. Но в итоге, мне все же кажется, что это скорее всего именно ошибка, причем достаточно грубая.
Взглянем на код:
bool cBinaryBuffer::DecompressAndAdd(char *apSrcData, size_t alSize)
{
....
///////////////////////////
// Init decompression
int ret = inflateInit(&zipStream);
if (ret != Z_OK) return false;
///////////////////////////
// Decompress, chunk by chunk
do
{
//Set current output chunk
zipStream.avail_out = lMaxChunkSize;
....
//Decompress as much as possible to current chunk
int ret = inflate(&zipStream, Z_NO_FLUSH);
if(ret != Z_OK && ret != Z_STREAM_END)
{
inflateEnd(&zipStream);
return false;
}
....
}
while (zipStream.avail_out == 0 && ret != Z_STREAM_END);
....
return true;
}V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. BinaryBuffer.cpp 371
Итак, у нас есть переменная ret, по которой контролируется выход из цикла do-while. Но внутри этого цикла, вместо присвоения нового значения этой внешней переменной, объявляется новая переменная с именем ret. В итоге, она перекрывает внешнюю переменную ret, и переменная, которая проверяется в условии цикла, никогда не изменится.
При неудачном стечении обстоятельств такой цикл мог бы стать бесконечным. Скорее всего, в этом случае этот код спасает внутреннее условие, которое проверяет значение внутренней переменной ret и приводит к выходу из функции.
Очень часто, разработчики используют статический анализ не регулярно, а с большими перерывами. Или вообще прогоняют проект через анализатор один раз. В итоге такого подхода зачастую анализатор не обнаруживает ничего серьёзного или находит что-то вроде рассматриваемых нами примеров, которые возможно особо и не влияли на дееспособность игры. Создаётся впечатление, что анализатор не особо то и полезен. Ну нашел он такие места, но всё же работает.
А дело в том, что аналогичные места, но в которых ошибка не маскировалась, а явно приводила к багу в программе, уже были выправлены долгими часами дебага, прогонов через тесты, отдел тестирования. В итоге, анализатор при проверке проекта всего один раз показывает лишь те проблемы, которые себя никак не проявили. Иногда среди таких проблем оказываются и серьёзные моменты, которые реально влияли на работу программы, но вероятность того, что программа пойдёт по их пути была низка, и потому эта ошибка была неизвестна разработчикам.
Поэтому крайне важно оценивать полезность статического анализа лишь после регулярного его использования. Если уж единовременный прогон через PVS-Studio выявил такие подозрительные и неаккуратные места в коде этой игры, то сколько тогда явных такого рода ошибок приходилось локализировать и править в процессе разработки.
Используйте статический анализатор регулярно!
0
0
0
0