Перевірка open-source сервера World of Warcraft CMaNGOS

У цій статті мені хотілося б поділитися результатами перевірки статичним аналізатором 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.

Цю помилку можна виправити, просто поставивши дужки:
if ((uint32 roll = urand(0, 99)) < 3)

Однакові дії в блоках if та else
Попередження 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.

Аналогічна помилка:
  • V567 Undefined behavior. The 'm_uiCrystalPosition' variable is modified while being used twice between sequence points. boss_ossirian.cpp 150
Помилка в умові
Попередження 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. Швидше за все, автор помилився і замість оператора '||' використовував оператор '&&'.

Примітно, що в коментарях зазначено дивна поведінка даного блоку коду і, можливо, що воно викликане цією помилкою.

У проекті знайшлося ще кілька подібних помилок, ось їх повний список:
  • V547 Expression is always false. Probably the '||' operator should be used here. SpellEffects.cpp 2872
  • V547 Expression is always true. Probably the '&&' operator should be used here. genrevision.cpp 261
  • V547 Expression is always true. Probably the '&&' operator should be used here. vmapexport.cpp 361
  • V547 Expression is always true. Probably the '&&' operator should be used here. MapTree.cpp 125
  • V547 Expression is always true. Probably the '&&' operator should be used here. MapTree.cpp 234
Підозріле форматування
Попередження 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.1 f' 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.1 f) // <=
{
....
return true;
}
return false;
}

Метод Unit::GetHealth() повертає значення типу uint32_t, і метод Unit::GetMaxHealth() також повертає значення типу uint32_t, тому результат їх поділу є цілочисельним і порівнювати його з 0.1 f безглуздо.

Щоб правильно визначити 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.1 f)
{
....
return true;
}

Безумовний вихід з циклу for
Попередження 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 приймає одне єдине значення.

Аналогічне попередження:
  • V612 An unconditional 'break' within a loop. Pet.cpp 895
Надлишкове умова
Попередження 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);
....
}

Перевірка this null
Попередження PVS-Studio: V704 '!this ||!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer компілятори. Unit.cpp 1417
void Unit::CalculateSpellDamage (....)
{
....
if (!this || !pVictim) // <=
return;
....
}

Згідно з сучасним стандартом С++, покажчик this ніколи не може бути нульовим. Найчастіше використання порівняння this з нулем може призводити до несподіваних помилок. Детальніше прочитати про це можна в описі діагностики V704.

Аналогічні перевірки:
  • V704 '!this ||!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer компілятори. Unit.cpp 1476
  • V704 '!this ||!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer компілятори. Unit.cpp 1511
  • V704 '!this ||!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer компілятори. Unit.cpp 1797
Невиправдана передача за посиланням
Попередження 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 два рази поспіль присвоюються різні значення. Безперечно варто перевірити цю ділянку коду на предмет помилок.

Перевірка покажчика на null після new
Попередження PVS-Studio: V668 There is no sense testing in 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), яка не буде генерувати виключення у випадку невдачі.

Аналогічні перевірки покажчиків:
  • V668 There is no sense testing in the 'data' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. loadlib.cpp 36
  • V668 There is no sense testing in the 'dmmerge' 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 560
  • V668 There is no sense testing in the 'm_session' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. WorldSocket.cpp 426
Неправильний порядок аргументів
Попередження 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 data heightmap
m_terrainBuilder->loadMap(mapID, 
tileX, tileY,
meshData);

// get data model
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, а блок, наступний за ним ніколи не виконається.

Аналогічна помилка:
  • V616 The 'SPELL_DAMAGE_CLASS_NONE' named constant with the value of 0 is used in the bitwise operation. Spell.cpp 692
Потенційне розіменування нульового покажчика
Попередження 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).

Аналогічні попередження:
  • V595 The 'model' pointer was utilized before it was verified against nullptr. Check lines: 374, 375. MapTree.cpp 374
  • V595 The 'unit' pointer was utilized before it was verified against nullptr. Check lines: 272, 290. Object.cpp 272
  • V595 The 'updateMask' pointer was utilized before it was verified against nullptr. Check lines: 351, 355. Object.cpp 351
  • V595 The 'dbcEntry1' pointer was utilized before it was verified against nullptr. Check lines: 7123, 7128. ObjectMgr.cpp 7123
Висновок
PVS-Studio як завжди знайшов багато підозрілих місць і помилок в коді. Сподіваюся, розробники CMaNGOS поправлять всі недоліки, а так само почнуть постійно використовувати статичний аналіз у своєму проекті, оскільки разова перевірка не так вже ефективна.

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

P. S. Ви можете запропонувати для перевірки нашим аналізатором будь-який цікавий для вас проект через форму зворотного зв'язку або за допомогою GitHub. Подробиці посилання.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Bredikhin Egor. Checking the World of Warcraft CMaNGOS open source server.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.