Порой хочется поностальгировать и поиграть в любимую старую игру, но некоторые вещи в таких играх могут показаться устаревшими. Для того чтобы вдохнуть новую жизнь в старый проект, некоторые энтузиасты ставят себе задачу воссоздать и улучшить его исходный код. Мы решили проверить с помощью статического анализатора PVS-Studio, насколько хорошо справляются с этой задачей разработчики VCMI.
VCMI Project – игровой движок с открытым исходным кодом для Героев Меча и Магии 3. Движок VCMI является кроссплатформенным и работает на устройствах под управлением Windows, Linux, Android, macOS и iOS. Привнесены следующие изменения: улучшены анимации, повышена производительность, обновлен графический интерфейс, исправлены баги, улучшена система модов и многое другое. Подробную информацию о проекте можно почитать здесь.
Над проектом трудится большая команда разработчиков, проект регулярно дорабатывается, а количество новых строк кода неизменно увеличивается. В таких условиях возможное появление ошибок в коде не является чудом. Любой программист может допустить ошибку вне зависимости от его опыта, причиной может стать банальная невнимательность или усталость, а разработчики VCMI трудятся в свободное время на некоммерческой основе.
Целью статьи не является каким-либо образом задеть разработчиков. Мы хотим показать, насколько могут быть полезны инструменты статического анализа. Они могут значительно упростить жизнь разработчикам и избавить пользователей от возможных багов. Это не только сократит время, затрачиваемое на разработку и тестирование, но также позволит избежать ошибок в конечном продукте, которые могут испортить удовольствие от его использования.
Начать пользоваться статическим анализатором проще, чем может показаться. Например, у PVS-Studio есть бесплатная версия для open-source проектов, а в документации можно посмотреть, как быстро внедрить его в процесс разработки.
Любителей серии Героев Меча и Магии может также заинтересовать наша статья о проверке Free Heroes of Might and Magic II.
Давайте же проверим эффективность статического анализа, рассмотрев некоторые фрагменты кода из данного проекта, на которые нам указал анализатор.
Перед тем как мы перейдём непосредственно к разбору, стоит упомянуть, что код весьма качественный. Местами разработчики даже перестраховывались, вставляя лишние проверки. Тем не менее в код всё равно закралось немало ошибок.
Проверка проводилась на коммите 10fc6ce.
Вопрос с памятью в играх стоит достаточно остро. Конечно, Герои Меча и Магии 3 сегодня сложно назвать требовательной игрой, работающей на пределе доступной оперативной памяти. Однако крайне неприятно видеть картину, когда через пару часов какая-нибудь игра съела несколько лишних гигабайт оперативной памяти. Что уж тут говорить, никто не любит утечки памяти. Рассмотрим следующие фрагменты кода и предупреждения анализатора.
Фрагмент N1
CGObjectInstance *CMapLoaderH3M::readDwellingRandom(....)
{
auto *object = new CGDwelling();
CSpecObjInfo *spec = nullptr;
switch(objectTemplate->id)
{
case Obj::RANDOM_DWELLING:
spec = new CCreGenLeveledCastleInfo();
break;
case Obj::RANDOM_DWELLING_LVL:
spec = new CCreGenAsCastleInfo();
break;
case Obj::RANDOM_DWELLING_FACTION:
spec = new CCreGenLeveledInfo();
break;
default:
throw std::runtime_error("Invalid random dwelling format");
}
spec->owner = object;
....
object->info = spec;
return object;
}Предупреждение PVS-Studio: V773 The exception was thrown without releasing the 'object' pointer. A memory leak is possible. MapFormatH3M.cpp 1173
Здесь мы видим, что если мы попадём в ветку default конструкции switch, то бросится исключение. При этом память, выделенная оператором new для указателя object, утечёт.
Можно решить эту проблему, переставив декларацию указателя object на позицию после тела конструкции switch, но гораздо лучше воспользоваться "умными" указателями. Один из вариантов исправленного кода:
std::unique_ptr<CGDwelling> CMapLoaderH3M::readDwellingRandom(....)
{
std::unique_ptr<CSpecObjInfo> spec{ nullptr };
switch(objectTemplate->id)
{
case Obj::RANDOM_DWELLING:
spec = std::make_unique<CCreGenLeveledCastleInfo>();
break;
case Obj::RANDOM_DWELLING_LVL:
spec = std::make_unique<CCreGenAsCastleInfo>();
break;
case Obj::RANDOM_DWELLING_FACTION:
spec = std::make_unique<CCreGenLeveledInfo>();
break;
default:
throw std::runtime_error("Invalid random dwelling format");
}
auto object = std::make_unique<CGDwelling>();
spec->owner = object.get();
....
object->info = std::move(spec);
return object;
}Фрагмент N2
CTownHandler::CTownHandler():
randomTown(new CTown()),
randomFaction(new CFaction())
{
randomFaction->town = randomTown;
randomTown->faction = randomFaction;
randomFaction->identifier = "random";
randomFaction->modScope = "core";
}
CTownHandler::~CTownHandler()
{
delete randomTown;
}Предупреждение PVS-Studio: V773 The 'randomFaction' pointer was not released in destructor. A memory leak is possible. CTownHandler.cpp 282
В списке инициализации конструктора два указателя инициализируют с помощью оператора new, а в деструкторе освобождают память только по одному указателю.
В простейшем случае исправленный код выглядит так:
CTownHandler::~CTownHandler()
{
delete randomFaction;
delete randomTown;
}Но можно заменить "простые" указатели на "умные" (например, на std::unique_ptr) и навсегда забыть о подобных проблемах.
Фрагмент N3
template <class T>
void addModificator()
{
modificators.emplace_back(new T(*this, map, generator));
}Предупреждение PVS-Studio: V1023 A pointer without owner is added to the 'modificators' container by the 'emplace_back' method. A memory leak will occur in case of an exception. Zone.h 122
Здесь ситуация поинтереснее. Контейнер modificators – это двусвязный список "умных" указателей std::unique_ptr. При помощи функции emplace_back в конец списка хотят вставить новый "умный" указатель "по месту", идеально передав "сырой" указатель на созданный оператором new объект типа T.
Такой код содержит потенциальную проблему. А что, если при создании очередного узла внутри std::list бросится исключение std::bad_alloc? Верно, произойдёт утечка памяти.
Исправить проблему легко – воспользуемся std::make_unique и заменим emplace_back на push_back:
template <class T>
void addModificator()
{
modificators.push_back(std::make_unique<T>(*this, map, generator));
}Да, в таком коде будет вызван лишний конструктор перемещения. Зато мы устранили вероятность утечки памяти.
У читателя также может возникнуть вопрос: "Зачем было заменять emplace_back на push_back"? Аргумент в пользу использования push_back в данном случае – меньше работы для компилятора, следовательно, меньше времени уходит на компиляцию.
Когда вы вызываете push_back, компилятор должен лишь выбрать перегрузку. Когда вы вызываете emplace_back, компилятор, помимо выбора перегрузки, ещё выполнит инстанцирование шаблона функции. При этом обе функции сделают абсолютно то же самое. Но это не значит, что приоритет всегда стоит отдавать push_back. Используйте emplace_back тогда, когда вы можете идеально передать аргументы в конструктор типа элемента контейнера. В ином случае, если ваш объект уже был создан или идеальная передача аргументов конструктора невозможна, отдайте предпочтение функции push_back.
В данном разделе мы рассмотрим фрагменты кода, приводящие к неопределённому поведению. Кому интересна тема UB, рекомендую также ознакомиться со следующей статьёй. Перейдём к рассмотрению срабатываний анализатора.
Фрагмент N4
void ApplyGhNetPackVisitor::visitTradeOnMarketplace(....)
{
....
bool allyTownSkillTrade = (....
&& gh.getPlayerRelations(player, hero->tempOwner)
&& ....);
if (hero && ....)
gh.throwAndComplain(&pack, "This hero can't use this marketplace!");
....
}Предупреждение PVS-Studio: V595 The 'hero' pointer was utilized before it was verified against nullptr. Check lines: 182, 184. NetPacksServer.cpp 182
Здесь мы видим, что указатель hero разыменовывается до проверки на nullptr. Похожее срабатывание:
Фрагмент N5
void Queries::popIfTop(QueryPtr query)
{
//LOG_TRACE_PARAMS(logGlobal, "query='%d'", query);
if(!query)
logGlobal->error("The query is nullptr! Ignoring.");
popIfTop(*query);
}Предупреждение PVS-Studio: V1004 The 'query' pointer was used unsafely after it was verified against nullptr. Check lines: 246, 249. CQuery.cpp 249
А этот случай поинтереснее. Здесь разработчики обрабатывают ситуацию, когда query равен nullptr, залоггировав ошибку. Однако после этого программа продолжит выполнение, и произойдёт разыменование нулевого указателя, что ведёт к неопределённому поведению. Возможно, после логгирования стоило прервать выполнение функции.
Фрагмент N6
void CCallback::trade(....)
{
....
pack.marketId = dynamic_cast<const CGObjectInstance *>(market)->id;
....
}Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer. CCallback.cpp 255
Тут разыменовывается результат оператора dynamic_cast. Поскольку dynamic_cast может возвращать nullptr, может произойти разыменование nullptr.
Анализатор выдал 112 предупреждений диагностики V522, из них с использованием dynamic_cast было связано около 100 штук. На каждое пятое такое предупреждение в коде содержалась проверка результирующего указателя при помощи макроса assert, что не является панацеей. Макрос assert раскрывается в пустую строку в релизной версии. Поэтому можно сказать, что ни один результат dynamic_cast не был проверен.
Можно возразить, сказав, что разработчики были уверены в возвращаемом результате. Однако следует помнить о том, что завтра код может измениться, и внезапно dynamic_cast начнёт возвращать nullptr. Цена ошибки в таком случае будет стоить гораздо больше, чем лишняя явно написанная проверка.
Вот лишь часть подобных срабатываний:
В этом разделе я бы хотел рассмотреть недостижимый код – код, который не выполнится ни при каких условиях в программе. Чаще всего такие места появляются из-за нарушений в логике условных операторов, но бывают случаи, когда причиной становится невнимательность.
Итак, вот они слева направо.
Фрагмент N7
size_t CTrueTypeFont::getGlyphWidth(const char *data) const
{
if (....)
return fallbackFont->getGlyphWidth(data);
return getStringWidth(....);
int advance;
TTF_GlyphMetrics(....);
return advance;
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. CTrueTypeFont.cpp 86
Разработчикам стоит обратить внимание на эту функцию: код после return getStringWidth(....); никогда не выполнится.
Фрагмент N8
CConnection::CConnection (....)
{
....
while (endpoint_iterator != end)
{
....
if (!error)
{
init();
return;
}
else
{
throw std::runtime_error(....);
}
endpoint_iterator++;
}
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. Connection.cpp 115
Тоже очень странный код. Здесь пытаются проитерироваться до некой границы, но строка с endpoint_iterator++ никогда не выполнится. Задумку автора мне разгадать не удалось, возможно, получится у кого-то из читателей.
Фрагмент N9
void CCreatureHandler::loadStackExp (....)
{
....
switch (mod[0])
{
....
case 'D':
b.type = BonusType::ADDITIONAL_ATTACK; break;
case 'f':
b.type = BonusType::FEARLESS; break;
case 'F':
b.type = BonusType::FLYING; break;
case 'm':
b.type = BonusType::MORALE; break; // <=
b.val = 1;
b.valType = BonusValueType::INDEPENDENT_MAX;
break;
....
}
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. CCreatureHandler.cpp 1094
В case 'm' не выполнится часть кода. Как мы видим, строки кода выше очень похожи. Можно сделать вывод, что данная ошибка появилась в результате copy-paste.
Фрагмент N10
bool Animation::loadFrame(size_t frame, size_t group)
{
....
if (....)
{
if (defFile)
{
auto frameList = defFile->getEntries();
if (....)
{
....
return true;
}
}
return false;
// still here? image is missing
printError(frame, group, "LoadFrame");
images[group][frame] = std::make_shared<QImage>("DEFAULT");
}
....
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. Animation.cpp 545
В этом фрагменте не выполнится всё, что находится после return false;. Судя по комментарию в коде, разработчикам известно об этом, и такой код - лишь временная мера.
Следующий случай самый интересный на мой взгляд.
Фрагмент N11
std::string CComponent::getSubtitleInternal()
{
....
if (val)
return ....;
else
return val > 1 ? creature->getNamePluralTranslated()
: creature->getNameSingularTranslated();
....
}Предупреждение PVS-Studio: V547 Expression 'val > 1' is always false. CComponent.cpp 218
Поток управления может попасть в ветку else только если val == 0. Соответственно val > 1 никогда не вернёт true, и creature->getNamePluralTranslated() является недостижимым кодом.
Стоит отметить, что разработчики оставили комментарий, что данную функцию необходимо исправить. Однако они либо не успели это сделать (на момент написания статьи), либо забыли. В любом случае, пользуясь статическим анализатором, такую ошибку можно было бы заметить ещё при написании функции и тут же её исправить.
Фрагмент N12
void deallocate_segment(....)
{
....
if (seg_index >= first_block)
{
segment_element_allocator_traits::deallocate(....);
}
else
if (seg_index == 0)
{
elements_to_deallocate = first_block > 0 ? this->segment_size(first_block)
: this->segment_size(0);
....
}
}Предупреждение PVS-Studio: V547 Expression 'first_block > 0' is always true. concurrent_vector.h 669
Давайте разберёмся, что же тут происходит. Если первая проверка не выполняется, то seg_index < first_block. Запомним. Теперь, если seg_index == 0, то это означает, что first_block > 0. А дальше мы ещё раз проверяем, что first_block > 0, и анализатор справедливо подмечает, что это условие и так всегда истинно. Следовательно, выражение this->segment_size(0) никогда не выполнится. Человеку может быть сложно проследить такую логическую цепочку, а вот для "бездушной машины" это не представляет особой трудности.
Выявление подобных ошибок возможно благодаря использованию в анализаторе технологии символьного выполнения (symbolic execution). Если вам интересно узнать, что это такое и как работает, предлагаю вашему вниманию статью "Технологии статического анализа кода PVS-Studio".
В данном разделе мы рассмотрим более концептуальную ошибку, связанную с неправильной перегрузкой оператора присваивания.
Фрагмент N13
ObjectTemplate & ObjectTemplate::operator=(const ObjectTemplate & rhs)
{
....
width = rhs.width;
height = rhs.height;
visitable = rhs.visitable;
blockedOffsets = rhs.blockedOffsets;
blockMapOffset = rhs.blockMapOffset;
visitableOffset = rhs.visitableOffset;
usedTiles.clear();
usedTiles.resize(rhs.usedTiles.size());
for(size_t i = 0; i < usedTiles.size(); i++)
std::copy(....);
return *this;
}Предупреждение PVS-Studio: V794 The assignment operator should be protected from the case of 'this == &rhs'. ObjectTemplate.cpp 88
Анализатор говорит нам, что не предусмотрен случай, когда объект присваивается сам себе. Когда выполнение дойдёт до вызова функции usedTiles.clear(), мы сотрём поле usedTiles нашего объекта. В результате resize сделает размер контейнера равным 0, и копирование не произойдёт (да и нечего уже копировать). Получается, часть объекта будет потеряна.
Решить проблему можно одним из способов:
В данном разделе собраны срабатывания, которые нельзя назвать однозначно ошибками, ведь они не приводят к серьёзным последствиям. Часть фрагментов может являться задумкой авторов, что определить проблематично, не будучи самим автором, часть же является излишними действиями, которых можно было избежать.
Перейдём к ситуациям, где разработчики совершили избыточные проверки либо в результате неправильного вычисления логических выражений, либо, вероятно, в качестве подстраховки.
Фрагмент N14
void BattleStacksController::updateBattleAnimations(uint32_t msPassed)
{
....
if (hadAnimations && currentAnimations.empty())
{
//stackAmountBoxHidden.clear();
owner.executeStagedAnimations();
if (currentAnimations.empty())
owner.onAnimationsFinished();
}
....
}Предупреждение PVS-Studio: V547 Expression 'currentAnimations.empty()' is always true. BattleStacksController.cpp 384
Вложенная проверка всегда истинна. Вполне возможно, что надо было проверить другой контейнер, либо проверку можно убрать.
Фрагмент N15
std::vector<BattleHex> CStack::meleeAttackHexes(....)
{
....
int mask = 0;
....
if (....)
{
if ((mask & 1) == 0)
{
mask |= 1;
res.push_back(defenderPos);
}
}
....
}Предупреждение PVS-Studio: V547 Expression '(mask & 1) == 0' is always true. CStack.cpp 262
Переменная mask объявляется равной нулю и далее до момента проверки никак не меняется в коде. Возможно, ранее маска каким-либо образом модифицировалась, но после рефакторинга теперь не изменяется.
Фрагмент N16
bool CGarrisonSlot::mustForceReselection() const
{
....
if (!creature || !selection->creature)
return false;
....
if (!owner->removableUnits)
{
if (selection->upg == EGarrisonType::UP)
return true;
else
return creature || upg == EGarrisonType::UP;
}
}Предупреждение PVS-Studio: V560 A part of conditional expression is always true: creature. CGarrisonInt.cpp 284
В самом начале фрагмента кода уже была проверка if (!creature) с дальнейшим выходом из функции. Соответственно, последний return будет всегда возвращать true.
Фрагмент N17
std::optional<BattleAction> CBattleAI::considerFleeingOrSurrendering()
{
....
if (!bs.canFlee || !bs.canSurrender)
{
return std::nullopt;
}
auto result = cb->makeSurrenderRetreatDecision(bs);
if (!result && bs.canFlee && bs.turnsSkippedByDefense > 30)
{
return BattleAction::makeRetreat(bs.ourSide);
}
....
}Предупреждение PVS-Studio: V560 A part of conditional expression is always true: bs.canFlee. BattleAI.cpp 837
Похожа на предыдущую, хотя больше выглядит как подстраховка на случай, если поле bs.canFlee поменяется после первой проверки (хотя при вызове cb->makeSurrenderRetreatDecision(bs) оно не может поменяться).
Аналогичное срабатывание:
Фрагмент N18
void ApplyOnServerNetPackVisitor::visitLobbySetCampaign(LobbySetCampaign & pack)
{
....
if ( !isCurrentMapConquerable
|| (isCurrentMapConquerable && i == *pack.ourCampaign->currentMap))
{
srv.setCampaignMap(i);
}
....
}Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!isCurrentMapConquerable' and 'isCurrentMapConquerable'. NetPacksLobbyServer.cpp 225
Здесь можно заметить, что проверку можно упростить до следующей:
if (!isCurrentMapConquerable || i == *pack.ourCampaign->currentMap)В данном разделе собраны остальные интересные срабатывания анализатора, которые сложно отнести к категориям выше.
Фрагмент N19
template <typename Handler>
void serialize(Handler &h, const int version)
{
h & x;
h & y;
h & w;
h & h;
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&' operator: h & h Rect.h 158
Это функция-член класса Rect, который содержит в себе поля x, y, w, h. Очевидно, что оператор & перегружен. Однако посмотреть, как именно, не представляется возможным, поскольку функция является шаблонной. В данном случае слева и справа от оператора & находится объект типа Handler, а не поле h класса Rect, и наверняка сказать сложно, задумано так или нет.
Например, в файле AIUtility.h на 74 строке есть такая функция:
template<typename Handler>
void serialize(Handler & h, const int version)
{
h & this->h;
h & hid;
h & name;
}В ней явно указывается поле this->h. Я просмотрел ещё пару таких функций (их весьма много), и ни в одной объект типа Handler не используется с оператором & одновременно находясь слева и справа.
Фрагмент N20
void CArmedInstance::updateMoraleBonusFromArmy()
{
....
//-1 modifier for any Undead unit in army
const ui8 UNDEAD_MODIFIER_ID = -2;
....
}Предупреждение PVS-Studio: V569 Truncation of constant value -2. The value range of unsigned char type: [0, 255]. CArmedInstance.cpp 123
Константа UNDEAD_MODIFIER_ID имеет тип unsigned char. Анализатор весьма справедливо ругается, что произойдёт усечение переменной. В результате такого присваивания, переменная UNDEAD_MODIFIER_ID будет равна 254. Возможно, так и задумано автором кода. Однако для лучшей читаемости кода желательно заменить значение на более осмысленное.
Фрагмент N21
void CGameHandler::endBattleConfirm(....)
{
....
for (int i = 0; i < cs.spells.size(); i++)
{
names << "%s";
if (i < cs.spells.size() - 2)
names << ", ";
else if (i < cs.spells.size() - 1)
names << "%s";
}
....
}Предупреждение PVS-Studio: V658 A value is being subtracted from the unsigned variable. This can result in an overflow. In such a case, the '<' comparison operation can potentially behave unexpectedly. Consider inspecting the 'i < cs.spells.size() - 2' expression. CGameHandler.cpp 760
Здесь cs.spells.size() возвращает значение типа size_t. В случае, если в контейнере cs.spells содержится только 1 объект, то в выражении cs.spells.size() – 2 произойдёт переполнение, и i сравнится с максимальным числом, которое может храниться в size_t. Соответственно, в поток запишется: "%s, ".
Возможно, так и было задумано, или разработчики уверены, что в контейнере всегда больше, чем 1 объект, но код выглядит очень странно.
Последний случай:
Фрагмент N22
CGameState::CrossoverHeroesList
CGameState::getCrossoverHeroesFromPreviousScenarios()
const
{
....
crossoverHeroes.heroesFromAnyPreviousScenarios =
crossoverHeroes.heroesFromPreviousScenario = heroes;
crossoverHeroes.heroesFromPreviousScenario = heroes;
....
}Предупреждение PVS-Studio: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1203, 1204. CGameState.cpp 1204
Здесь 2 раза подряд в crossoverHeroes.heroesFromPreviousScenario присваивается heroes. Вероятно, heroes хотели сохранить в другую переменную.
В статье приведён далеко не весь список обнаруженных ошибок, некоторые были в сторонних библиотеках. Хоть такие ошибки всё равно находятся в VCMI, рассматривать их в статье я не стал, ведь непосредственно разработчики VCMI к ним не имеют отношения.
Для того чтобы такие срабатывания не мешали, в PVS-Studio есть возможность убрать из анализа выбранные каталоги или файлы, так я и поступил. Удалось выяснить, что разработчики проверяли проект с помощью анализатора Coverity, но информация о последней проверке относится к 14 апреля 2018. К сожалению, узнать, пользуются ли сейчас разработчики каким-либо статическим анализатором, не получилось.
Можно сделать вывод, что, используя решение от PVS-Studio, удалось бы избежать многих опасных и сомнительных мест в коде, тем самым обезопасив игроков от возможных багов в процессе игры в одну из лучших стратегий 20-го столетия.