В этой статье мне хотелось бы поделиться результатами проверки статическим анализатором PVS-Studio открытой реализации сервера игры World of Warcraft – CMaNGOS.
C(ontinued)MaNGOS является активно развивающимся ответвлением старого проекта MaNGOS (Massive Network Game Object Server), призванного создать свободный альтернативный сервер для игры World of Warcraft. Большая часть разработчиков MaNGOS продолжает работу в проекте CMaNGOS.
Как пишут сами разработчики, их цель – создать открытый "well written server in C++" для одной из лучших MMORPG. Постараюсь немного помочь им в этом, и проверю CMaNGOS при помощи статического анализатора PVS-Studio.
Примечание: Для проверки использовался сервер CMaNGOS-Classic, доступный в репозитории проекта на github.
Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. SpellEffects.cpp 473
void Spell::EffectDummy(SpellEffectIndex eff_idx)
{
....
if (uint32 roll = urand(0, 99) < 3) // <=
....
else if (roll < 6)
....
else if (roll < 9)
....
....
}Автор предполагал, что переменной roll будет присвоено случайное значение, а затем произойдет сравнение этого значения с 3. Однако, приоритет операции сравнения выше, чем приоритет операции присваивания (см. таблицу приоритетов операций), поэтому в действительности сначала случайное число будет сравниваться с 3, а затем результат этого сравнения (0 или 1) будет записан в переменную roll.
Данную ошибку можно исправить таким образом:
uint32 roll = urand(0, 99);
if (roll < 3)
{
....
}Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. SpellAuras.cpp 1537
void Aura::HandleAuraModShapeshift(bool apply, bool Real)
{
switch (form)
{
case FORM_CAT:
....
case FORM_TRAVEL:
....
case FORM_AQUA:
if (Player::TeamForRace(target->getRace()) == ALLIANCE)
modelid = 2428; // <=
else
modelid = 2428; // <=
....
}
....
}В обоих блоках переменной modelid присваивается одно и то же значение, скорее всего, это ошибка, и константу в одном из блоков нужно заменить на какую-то другую.
Предупреждение PVS-Studio: V567 Undefined behavior. The 'm_uiMovePoint' variable is modified while being used twice between sequence points. boss_onyxia.cpp 405
void UpdateAI(const uint32 uiDiff) override
{
....
switch (urand(0, 2))
{
case 0:
....
case 1:
{
// C++ is stupid, so add -1 with +7
m_uiMovePoint += NUM_MOVE_POINT - 1;
m_uiMovePoint %= NUM_MOVE_POINT;
break;
}
case 2:
++m_uiMovePoint %= NUM_MOVE_POINT; // <=
break;
}
....
}В указанной строке переменная m_uiMovePoint дважды изменяется в рамках одной точки следования, что приводит к неопределенному поведению программы. Более подробно об этом можно почитать в описании диагностики V567.
Аналогичная ошибка:
Предупреждение PVS-Studio: V547 Expression is always false. Probably the '||' operator should be used here. SpellEffects.cpp 2872
void Spell::EffectEnchantItemTmp(SpellEffectIndex eff_idx)
{
....
// TODO: Strange stuff in following code
// shaman family enchantments
if (....)
duration = 300;
else if (m_spellInfo->SpellIconID == 241 &&
m_spellInfo->Id != 7434)
duration = 3600;
else if (m_spellInfo->Id == 28891 &&
m_spellInfo->Id == 28898) // <=
duration = 3600;
....
}В указанном условии проверяется равенство переменной m_spellInfo->Id двум разным значениям одновременно. Результат такой проверки, естественно, всегда false. Скорее всего, автор ошибся и вместо оператора '||' использовал оператор '&&'.
Примечательно, что в комментариях отмечено странное поведение данного блока кода и, возможно, что оно как раз вызвано этой ошибкой.
В проекте нашлось еще несколько подобных ошибок, вот их полный список:
Предупреждение PVS-Studio: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. instance_blackrock_depths.cpp 111
void instance_blackrock_depths::OnCreatureCreate(Creature* pCreature)
{
switch (pCreature->GetEntry())
{
....
case NPC_HAMMERED_PATRON:
....
if (m_auiEncounter[11] == DONE)
pCreature->SetFactionTemporary(....);
pCreature->SetStandState(UNIT_STAND_STATE_STAND); // <=
break;
case NPC_PRIVATE_ROCKNOT:
case NPC_MISTRESS_NAGMARA:
....
}
}Здесь, вероятнее всего, автор забыл поставить фигурные скобки после оператора if, в результате чего вызов pCreature->SetStandState(UNIT_STAND_STATE_STAND) будет выполняться вне зависимости от условия в if.
Если же такое поведение и задумывалось, то стоит поправить выравнивание кода:
if (m_auiEncounter[11] == DONE)
pCreature->SetFactionTemporary(....);
pCreature->SetStandState(UNIT_STAND_STATE_STAND);Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: SAY_BELNISTRASZ_AGGRO_1. razorfen_downs.cpp 104
void AttackedBy(Unit* pAttacker) override
{
....
if (!m_bAggro)
{
DoScriptText(urand(0, 1) ?
SAY_BELNISTRASZ_AGGRO_1 : // <=
SAY_BELNISTRASZ_AGGRO_1, // <=
m_creature, pAttacker);
m_bAggro = true;
}
....
}Второй и третий операнды тернарного оператора идентичны, скорее всего, это ошибка. Судя по коду проекта, можно предположить, что один из операндов должен иметь значение SAY_BELNISTRASZ_AGGRO_2.
Предупреждение PVS-Studio: V674 The '0.1f' literal of the 'float' type is compared to a value of the 'unsigned int' type. item_scripts.cpp 44
bool ItemUse_item_orb_of_draconic_energy(....)
{
....
// If Emberstrife is already mind controled or above 10% HP:
// force spell cast failure
if (pEmberstrife && pEmberstrife->HasAura(SPELL_DOMINION_SOUL)
|| pEmberstrife->GetHealth() /
pEmberstrife->GetMaxHealth() > 0.1f) // <=
{
....
return true;
}
return false;
}Метод Unit::GetHealth() возвращает значение типа uint32_t, и метод Unit::GetMaxHealth() также возвращает значение типа uint32_t, поэтому результат их деления является целочисленным и сравнивать его с 0.1f бессмысленно.
Чтобы правильно определить 10% границу здоровья, данный код можно переписать, например, так:
// If Emberstrife is already mind controled or above 10% HP:
// force spell cast failure
if (pEmberstrife && pEmberstrife->HasAura(SPELL_DOMINION_SOUL)
|| ((float)pEmberstrife->GetHealth()) /
((float)pEmberstrife->GetMaxHealth()) > 0.1f)
{
....
return true;
}Предупреждение PVS-Studio: V612 An unconditional 'break' within a loop. Pet.cpp 1956
void Pet::InitPetCreateSpells()
{
....
for (SkillLineAbilityMap::const_iterator
_spell_idx = bounds.first; _spell_idx != bounds.second;
++_spell_idx)
{
usedtrainpoints += _spell_idx->second->reqtrainpoints;
break; // <=
}
....
}Не вполне понятно, что здесь имелось в виду, но безусловный оператор break в теле цикла for выглядит очень подозрительно. Даже если здесь нет ошибки, стоит отрефакторить код и избавиться от ненужного цикла, ведь итератор _spell_idx принимает одно единственное значение.
Аналогичное предупреждение:
Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!realtimeonly' and 'realtimeonly'. Player.cpp 10536
void Player::UpdateItemDuration(uint32 time, bool realtimeonly)
{
....
if ((realtimeonly && (....)) || !realtimeonly) // <=
item->UpdateDuration(this, time);
....
}Проверку вида (a && b) || !a можно упростить до !a || b, что наглядно видно на таблице истинности:
Таким образом, исходное выражение упростится до:
void Player::UpdateItemDuration(uint32 time, bool realtimeonly)
{
....
if (!(realtimeonly) || (....))
item->UpdateDuration(this, time);
....
}Предупреждение PVS-Studio: V704 '!this ||!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer compilers. Unit.cpp 1417
void Unit::CalculateSpellDamage(....)
{
....
if (!this || !pVictim) // <=
return;
....
}Согласно современному стандарту С++, указатель this никогда не может быть нулевым. Зачастую использование сравнения this с нулем может приводить к неожиданным ошибкам. Подробнее прочитать об этом можно в описании диагностики V704.
Аналогичные проверки:
Предупреждение PVS-Studio: V669 The 'uiHealedAmount' argument is a non-constant reference. The analyzer is unable to determine the position at which this argument is being modified. It is possible that the function contains an error. boss_twinemperors.cpp 109
void
HealedBy(Unit* pHealer, uint32& uiHealedAmount) override // <=
{
if (!m_pInstance)
return;
if (Creature* pTwin =
m_pInstance->GetSingleCreatureFromStorage(
m_creature->GetEntry() == NPC_VEKLOR ?
NPC_VEKNILASH :
NPC_VEKLOR))
{
float fHealPercent = ((float)uiHealedAmount) /
((float)m_creature->GetMaxHealth());
uint32 uiTwinHeal =
(uint32)(fHealPercent * ((float)pTwin->GetMaxHealth()));
uint32 uiTwinHealth = pTwin->GetHealth() + uiTwinHeal;
pTwin->SetHealth(uiTwinHealth < pTwin->GetMaxHealth() ?
uiTwinHealth :
pTwin->GetMaxHealth());
}
}Переменная uiHealedAmount передается по ссылке, но в теле функции не изменяется. Это может вводить в заблуждение, ведь создается впечатление, что функция HealedBy() что-то записывает в uiHealedAmount. Лучше передать переменную по константной ссылке или по значению.
Предупреждение PVS-Studio: V519 The 'stat' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1776, 1781. DetourNavMeshQuery.cpp 1781
dtStatus dtNavMeshQuery::findStraightPath(....) const
{
....
if (....)
{
stat = appendPortals(apexIndex, i, closestEndPos, // <=
path, straightPath, straightPathFlags,
straightPathRefs, straightPathCount,
maxStraightPath, options);
}
stat = appendVertex(closestEndPos, 0, path[i], // <=
straightPath, straightPathFlags,
straightPathRefs, straightPathCount,
maxStraightPath);
....
}Анализатор обнаружил подозрительное место, где переменной stat два раза подряд присваиваются разные значения. Определенно стоит проверить этот участок кода на предмет ошибок.
Предупреждение PVS-Studio: V668 There is no sense in testing the 'pmmerge' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. MapBuilder.cpp 553
void MapBuilder::buildMoveMapTile(....)
{
....
rcPolyMesh** pmmerge =
new rcPolyMesh*[TILES_PER_MAP * TILES_PER_MAP];
if (!pmmerge) // <=
{
printf("%s alloc pmmerge FIALED! \r", tileString);
return;
}
....
}Проверка указателя на ноль после использования оператора new бессмысленна. При невозможности выделить память, оператор new генерирует исключение std::bad_alloc(), а не возвращает nullptr. Соответственно, программа никогда не зайдет в блок после условия.
Чтобы исправить эту ошибку можно сделать выделение памяти в блоке try {....} catch(const std::bad_alloc &) {....}, или использовать для выделения памяти конструкцию new(std::nothrow), которая не будет генерировать исключение в случае неудачи.
Аналогичные проверки указателей:
Предупреждение PVS-Studio: V764 Possible incorrect order of arguments passed to 'loadVMap' function: 'tileY' and 'tileX'. MapBuilder.cpp 279
void MapBuilder::buildTile(uint32 mapID,
uint32 tileX, uint32 tileY,
dtNavMesh* navMesh, uint32 curTile,
uint32 tileCount)
{
....
// get heightmap data
m_terrainBuilder->loadMap(mapID,
tileX, tileY,
meshData);
// get model data
m_terrainBuilder->loadVMap(mapID,
tileY, tileX, // <=
meshData);
....
}Анализатор обнаружил подозрительную передачу аргументов в функцию - были перепутаны местами аргументы tileX и tileY.
Если взглянуть на прототип функции loadVMap(), то становится очевидно, что это действительно ошибка.
bool loadVMap(uint32 mapID,
uint32 tileX, uint32 tileY,
MeshData& meshData);Предупреждение PVS-Studio: V760 Two identical blocks of text were found. The second block begins from line 213. BattleGround.cpp 210
BattleGround::BattleGround()
: m_BuffChange(false),
m_StartDelayTime(0),
m_startMaxDist(0)
{
....
m_TeamStartLocO[TEAM_INDEX_ALLIANCE] = 0;
m_TeamStartLocO[TEAM_INDEX_HORDE] = 0;
m_BgRaids[TEAM_INDEX_ALLIANCE] = nullptr;
m_BgRaids[TEAM_INDEX_HORDE] = nullptr;
m_PlayersCount[TEAM_INDEX_ALLIANCE] = 0; // <=
m_PlayersCount[TEAM_INDEX_HORDE] = 0; // <=
m_PlayersCount[TEAM_INDEX_ALLIANCE] = 0; // <=
m_PlayersCount[TEAM_INDEX_HORDE] = 0; // <=
m_TeamScores[TEAM_INDEX_ALLIANCE] = 0;
m_TeamScores[TEAM_INDEX_HORDE] = 0;
....
}Здесь два раза подряд выполняются одни и те же действия. Скорее всего, такой код появился в результате Copy-Paste.
Предупреждение PVS-Studio: V571 Recurring check. The 'isDirectory' condition was already verified in line 166. FileSystem.cpp 169
FileSystem::Dir&
FileSystem::getContents(const std::string& path,
bool forceUpdate)
{
// Does this path exist on the real filesystem?
if (exists && isDirectory) // <=
{
// Is this path actually a directory?
if (isDirectory) // <=
{
....
}
....
}
....
}Условие isDirectory проверяется дважды. Можно удалить дублирующую проверку.
Предупреждение PVS-Studio: V616 The 'SPELL_DAMAGE_CLASS_NONE' named constant with the value of 0 is used in the bitwise operation. Spell.cpp 674
void Spell::prepareDataForTriggerSystem()
{
....
if (IsPositiveSpell(m_spellInfo->Id))
{
if (m_spellInfo->DmgClass & SPELL_DAMAGE_CLASS_NONE) // <=
{
m_procAttacker = PROC_FLAG_DONE_SPELL_NONE_DMG_CLASS_POS;
m_procVictim = PROC_FLAG_TAKEN_SPELL_NONE_DMG_CLASS_POS;
}
}
....
}Константа SPELL_DAMAGE_CLASS_NONE имеет нулевое значение, а побитовое И любого числа и нуля, является нулем, следовательно условие будет всегда иметь значение false, а блок, следующий за ним никогда не выполнится.
Аналогичная ошибка:
Предупреждение PVS-Studio: V595 The 'model' pointer was utilized before it was verified against nullptr. Check lines: 303, 305. MapTree.cpp 303
bool StaticMapTree::InitMap(const std::string& fname,
VMapManager2* vm)
{
....
WorldModel* model =
vm->acquireModelInstance(iBasePath, spawn.name);
model->setModelFlags(spawn.flags); // <=
....
if (model) // <=
{
....
}
....
}Указатель model проверяется на null, т.е. допускается его равенство нулю, однако, выше указатель уже используется и без всяких проверок. Налицо потенциальное разыменовывание нулевого указателя.
Чтобы исправить эту ошибку, следует проверить значение указателя model до того, как вызывать метод model->setModelFlags(spawn.flags).
Аналогичные предупреждения:
PVS-Studio как всегда нашел много подозрительных мест и ошибок в коде. Надеюсь, разработчики CMaNGOS поправят все недочеты, а так же начнут постоянно использовать статический анализ в своем проекте, поскольку разовая проверка не так уж эффективна.
Также напомню, что теперь любой желающий может воспользоваться возможностью бесплатного использования PVS-Studio при соблюдении описанных по ссылке условий.
P.S. Вы можете предложить для проверки нашим анализатором любой интересный для вас проект через форму обратной связи или с помощью GitHub. Подробности по ссылке.