# Герои Кода и Магии: анализ игрового движка VCMI Порой хочется поностальгировать и поиграть в любимую старую игру, но некоторые вещи в таких играх могут показаться устаревшими\. Для того чтобы вдохнуть новую жизнь в старый проект, некоторые энтузиасты ставят себе задачу воссоздать и улучшить его исходный код\. Мы решили проверить с помощью статического анализатора PVS\-Studio, насколько хорошо справляются с этой задачей разработчики VCMI\. ![1058_VCMI_ru/image1.png](https://import.viva64.com/docx/blog/1058_VCMI_ru/image1.png) ## Коротко о проекте VCMI Project – игровой движок с открытым исходным кодом для Героев Меча и Магии 3\. Движок VCMI является кроссплатформенным и работает на устройствах под управлением Windows, Linux, Android, macOS и iOS\. Привнесены следующие изменения: улучшены анимации, повышена производительность, обновлен графический интерфейс, исправлены баги, улучшена система модов и многое другое\. Подробную информацию о проекте можно почитать [здесь](https://vcmi.eu/)\. Над проектом трудится большая команда разработчиков, проект регулярно дорабатывается, а количество новых строк кода неизменно увеличивается\. В таких условиях возможное появление ошибок в коде не является чудом\. Любой программист может допустить ошибку вне зависимости от его опыта, причиной может стать банальная невнимательность или усталость, а разработчики VCMI трудятся в свободное время на некоммерческой основе\. Целью статьи не является каким\-либо образом задеть разработчиков\. Мы хотим показать, насколько могут быть полезны инструменты статического анализа\. Они могут значительно упростить жизнь разработчикам и избавить пользователей от возможных багов\. Это не только сократит время, затрачиваемое на разработку и тестирование, но также позволит избежать ошибок в конечном продукте, которые могут испортить удовольствие от его использования\. Начать пользоваться статическим анализатором проще, чем может показаться\. Например, у PVS\-Studio есть [бесплатная версия](https://pvs-studio.ru/ru/blog/posts/0614/) для open\-source проектов, а в [документации](https://pvs-studio.ru/ru/docs/) можно посмотреть, как быстро внедрить его в процесс разработки\. Любителей серии Героев Меча и Магии может также заинтересовать наша [статья о проверке Free Heroes of Might and Magic II](https://pvs-studio.ru/ru/blog/posts/cpp/0804/)\. Давайте же проверим эффективность статического анализа, рассмотрев некоторые фрагменты кода из данного проекта, на которые нам указал анализатор\. ![1058_VCMI_ru/image2.png](https://import.viva64.com/docx/blog/1058_VCMI_ru/image2.png) ## Результаты проверки Перед тем как мы перейдём непосредственно к разбору, стоит упомянуть, что код весьма качественный\. Местами разработчики даже перестраховывались, вставляя лишние проверки\. Тем не менее в код всё равно закралось немало ошибок\. Проверка проводилась на коммите [10fc6ce](https://github.com/vcmi/vcmi/tree/10fc6cecef54d3f6a952e4f6c5d794160322f61e)\. ### Утечки памяти Вопрос с памятью в играх стоит достаточно остро\. Конечно, Герои Меча и Магии 3 сегодня сложно назвать требовательной игрой, работающей на пределе доступной оперативной памяти\. Однако крайне неприятно видеть картину, когда через пару часов какая\-нибудь игра съела несколько лишних гигабайт оперативной памяти\. Что уж тут говорить, никто не любит утечки памяти\. Рассмотрим следующие фрагменты кода и предупреждения анализатора\. **Фрагмент N1** ```cpp 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_, но гораздо лучше воспользоваться "умными" указателями\. Один из вариантов исправленного кода: ```cpp std::unique_ptr CMapLoaderH3M::readDwellingRandom(....) { std::unique_ptr spec{ nullptr }; switch(objectTemplate->id) { case Obj::RANDOM_DWELLING: spec = std::make_unique(); break; case Obj::RANDOM_DWELLING_LVL: spec = std::make_unique(); break; case Obj::RANDOM_DWELLING_FACTION: spec = std::make_unique(); break; default: throw std::runtime_error("Invalid random dwelling format"); } auto object = std::make_unique(); spec->owner = object.get(); .... object->info = std::move(spec); return object; } ``` **Фрагмент N2** ```cpp 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_, а в деструкторе освобождают память только по одному указателю\. В простейшем случае исправленный код выглядит так: ```cpp CTownHandler::~CTownHandler() { delete randomFaction; delete randomTown; } ``` Но можно заменить "простые" указатели на "умные" \(например, на _std::unique\_ptr_\) и навсегда забыть о подобных проблемах\. ![1058_VCMI_ru/image3.png](https://import.viva64.com/docx/blog/1058_VCMI_ru/image3.png) **Фрагмент N3** ```cpp template 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_: ```cpp template void addModificator() { modificators.push_back(std::make_unique(*this, map, generator)); } ``` Да, в таком коде будет вызван лишний конструктор перемещения\. Зато мы устранили вероятность утечки памяти\. У читателя также может возникнуть вопрос: "Зачем было заменять _emplace\_back_ на _push\_back_"? Аргумент в пользу использования _push\_back_ в данном случае – меньше работы для компилятора, следовательно, меньше времени уходит на компиляцию\. Когда вы вызываете _push\_back_, компилятор должен лишь выбрать перегрузку\. Когда вы вызываете _emplace\_back_, компилятор, помимо выбора перегрузки, ещё выполнит инстанцирование шаблона функции\. При этом обе функции сделают абсолютно то же самое\. Но это не значит, что приоритет всегда стоит отдавать _push\_back_\. Используйте _emplace\_back_ тогда, когда вы можете идеально передать аргументы в конструктор типа элемента контейнера\. В ином случае, если ваш объект уже был создан или идеальная передача аргументов конструктора невозможна, отдайте предпочтение функции _push\_back_\. ### Undefined behavior В данном разделе мы рассмотрим фрагменты кода, приводящие к неопределённому поведению\. Кому интересна тема UB, рекомендую также ознакомиться со следующей [статьёй](https://pvs-studio.ru/ru/blog/posts/cpp/1024/)\. Перейдём к рассмотрению срабатываний анализатора\. **Фрагмент N4** ```cpp 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** ```cpp 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_, залоггировав ошибку\. Однако после этого программа продолжит выполнение, и произойдёт разыменование нулевого указателя, что ведёт к [неопределённому поведению](https://pvs-studio.ru/ru/blog/posts/cpp/0306/)\. Возможно, после логгирования стоило прервать выполнение функции\. **Фрагмент N6** ```cpp void CCallback::trade(....) { .... pack.marketId = dynamic_cast(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 * \.\.\. ### Недостижимый код В этом разделе я бы хотел рассмотреть недостижимый код – код, который не выполнится ни при каких условиях в программе\. Чаще всего такие места появляются из\-за нарушений в логике условных операторов, но бывают случаи, когда причиной становится невнимательность\. ![1058_VCMI_ru/image4.png](https://import.viva64.com/docx/blog/1058_VCMI_ru/image4.png) Итак, вот они ~~слева направо~~\. **Фрагмент N7** ```cpp 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** ```cpp 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** ```cpp 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** ```cpp 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("DEFAULT"); } .... } ``` Предупреждение PVS\-Studio: V779 Unreachable code detected\. It is possible that an error is present\. Animation\.cpp 545 В этом фрагменте не выполнится всё, что находится после _return false;_\. Судя по комментарию в коде, разработчикам известно об этом, и такой код \- лишь временная мера\. Следующий случай самый интересный на мой взгляд\. **Фрагмент N11** ```cpp 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** ```cpp 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](https://pvs-studio.ru/ru/blog/posts/0908/)"\. ### Ошибка перегрузки оператора В данном разделе мы рассмотрим более концептуальную ошибку, связанную с неправильной перегрузкой оператора присваивания\. **Фрагмент N13** ```cpp 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, и копирование не произойдёт \(да и нечего уже копировать\)\. Получается, часть объекта будет потеряна\. Решить проблему можно одним из способов: * Добавить проверку _this \=\= &rhs_ и досрочно выйти из функции\. * Реализовать _operator\=_ при помощи идиомы [copy\-and\-swap](https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Copy-and-swap)\. ## Сомнительные фрагменты кода В данном разделе собраны срабатывания, которые нельзя назвать однозначно ошибками, ведь они не приводят к серьёзным последствиям\. Часть фрагментов может являться задумкой авторов, что определить проблематично, не будучи самим автором, часть же является излишними действиями, которых можно было избежать\. ### Лишние проверки условий Перейдём к ситуациям, где разработчики совершили избыточные проверки либо в результате неправильного вычисления логических выражений, либо, вероятно, в качестве подстраховки\. **Фрагмент N14** ```cpp 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** ```cpp std::vector 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** ```cpp 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** ```cpp std::optional 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\)_ оно не может поменяться\)\. Аналогичное срабатывание: * V547 Expression 'hero\-\>movement \> 0' is always true\. ExecuteHeroChain\.cpp 140 **Фрагмент N18** ```cpp 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 Здесь можно заметить, что проверку можно упростить до следующей: ```cpp if (!isCurrentMapConquerable || i == *pack.ourCampaign->currentMap) ``` ### Разное В данном разделе собраны остальные интересные срабатывания анализатора, которые сложно отнести к категориям выше\. **Фрагмент N19** ```cpp template 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 строке есть такая функция: ```cpp template void serialize(Handler & h, const int version) { h & this->h; h & hid; h & name; } ``` В ней явно указывается поле _this\-\>h_\. Я просмотрел ещё пару таких функций \(их весьма много\), и ни в одной объект типа _Handler_ не используется с оператором _&_ одновременно находясь слева и справа\. **Фрагмент N20** ```cpp 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** ```cpp 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** ```cpp 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 есть возможность [убрать из анализа](https://pvs-studio.ru/ru/docs/manual/6640/) выбранные каталоги или файлы, так я и поступил\. Удалось выяснить, что разработчики проверяли проект с помощью анализатора Coverity, но информация о последней проверке относится к 14 апреля 2018\. К сожалению, узнать, пользуются ли сейчас разработчики каким\-либо статическим анализатором, не получилось\. Можно сделать вывод, что, используя решение от [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/), удалось бы избежать многих опасных и сомнительных мест в коде, тем самым обезопасив игроков от возможных багов в процессе игры в одну из лучших стратегий 20\-го столетия\.