Герои Кода и Магии: анализ игрового движка VCMI

Порой хочется поностальгировать и поиграть в любимую старую игру, но некоторые вещи в таких играх могут показаться устаревшими. Для того чтобы вдохнуть новую жизнь в старый проект, некоторые энтузиасты ставят себе задачу воссоздать и улучшить его исходный код. Мы решили проверить с помощью статического анализатора PVS-Studio, насколько хорошо справляются с этой задачей разработчики VCMI.

VCMI Project – игровой движок с открытым исходным кодом для Героев Меча и Магии 3. Движок VCMI является кроссплатформенным и работает на устройствах под управлением Windows, Linux, Android, macOS и iOS. Привнесены следующие изменения: улучшены анимации, повышена производительность, обновлен графический интерфейс, исправлены баги, улучшена система модов и многое другое. Подробную информацию о проекте можно почитать здесь.

Над проектом трудится большая команда разработчиков, проект регулярно дорабатывается, а количество новых строк кода неизменно увеличивается. В таких условиях возможное появление ошибок в коде не является чудом. Любой программист может допустить ошибку вне зависимости от его опыта, причиной может стать банальная невнимательность или усталость, а разработчики VCMI трудятся в свободное время на некоммерческой основе.

Целью статьи не является каким-либо образом задеть разработчиков. Мы хотим показать, насколько могут быть полезны инструменты статического анализа. Они могут значительно упростить жизнь разработчикам и избавить пользователей от возможных багов. Это не только сократит время, затрачиваемое на разработку и тестирование, но также позволит избежать ошибок в конечном продукте, которые могут испортить удовольствие от его использования.

Любителей серии Героев Меча и Магии может также заинтересовать наша статья о проверке 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. Похожее срабатывание:

V595 The 'gs' pointer was utilized before it was verified against nullptr. Check lines: 628, 629. Client.cpp 628

Фрагмент 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. Цена ошибки в таком случае будет стоить гораздо больше, чем лишняя явно написанная проверка.

Вот лишь часть подобных срабатываний:

V522 There might be dereferencing of a potential null pointer 'boat'. MapRendererContext.cpp 47
V522 There might be dereferencing of a potential null pointer 'hero'. MapRendererContext.cpp 134
V522 There might be dereferencing of a potential null pointer 'hero'. MapViewController.cpp 291
V522 There might be dereferencing of a potential null pointer. CArtifactsOfHeroAltar.cpp 102
V522 There might be dereferencing of a potential null pointer 'boat'. MapViewController.cpp 323
V522 There might be dereferencing of a potential null pointer 'hero'. MapViewController.cpp 333
V522 There might be dereferencing of a potential null pointer 'dst'. NetPacksClient.cpp 902
V522 There might be dereferencing of a potential null pointer 'questObj'. AIMovementAfterDestinationRule.cpp 130
V522 There might be dereferencing of a potential null pointer 'quest'. AIUtility.cpp 258
V522 There might be dereferencing of a potential null pointer 'd'. AIUtility.cpp 397
...

В этом разделе я бы хотел рассмотреть недостижимый код – код, который не выполнится ни при каких условиях в программе. Чаще всего такие места появляются из-за нарушений в логике условных операторов, но бывают случаи, когда причиной становится невнимательность.

Итак, вот они слева направо.

Фрагмент 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() является недостижимым кодом.

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

Полная версия статьи – тык

#gameengine #opensourcegames #VCMI #staticanalysis

6 комментариев

I'm Qugurun

18.07.2023

Сайт про игры, пост про игру, не вижу никаких противоречий. Считаю такого контента должно быть больше, достойное для прочтения. Спасибо.

Ответить

Вадим

Пост не про игру, а про пользу статических анализаторов. Программисты о них и так знают. Суть в том что для людей далёких от темы технические детали идут лесом. Среднестатистический юзер понятия не имеет что такое указатель и чем он отличается от умного. Не говоря уже о более сложных понятиях

max.mustermann

20.07.2023

Я бы сказал, что это просто реклама PVS-Studio. :)

А вы уверены что этой статье тут место? Профессионалы тут вряд-ли обитают. А для мимо крокодилов это не особо понятно и интересно.

Денис Кадыков

Раздел геймдев, кто мимо проходит пусть идут в игры

Герои Кода и Магии: анализ игрового движка VCMI

Коротко о проекте

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

Утечки памяти

Undefined behavior

Недостижимый код