# Проверка open\-source сервера World of Warcraft CMaNGOS

В этой статье мне хотелось бы поделиться результатами проверки статическим анализатором PVS\-Studio открытой реализации сервера игры World of Warcraft – CMaNGOS\.

![0476_CMaNGOS_ru/image1.png](https://import.viva64.com/docx/blog/0476_CMaNGOS_ru/image1.png)

## Введение

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\.](https://github.com/cmangos/mangos-classic)

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

## Ошибка в приоритете операций

**Предупреждение PVS\-Studio:** [V593](https://pvs-studio.ru/ru/docs/warnings/v593/) Consider reviewing the expression of the 'A \= B < C' kind\. The expression is calculated as following: 'A \= \(B < C\)'\. SpellEffects\.cpp 473

```cpp
void Spell::EffectDummy(SpellEffectIndex eff_idx)
{
  ....
  if (uint32 roll = urand(0, 99) < 3) // <=
    ....
  else if (roll < 6)
    ....
  else if (roll < 9)
    ....
  ....
}
```

Автор предполагал, что переменной _roll_ будет присвоено случайное значение, а затем произойдет сравнение этого значения с _3_\. Однако, приоритет операции сравнения выше, чем приоритет операции присваивания \(см\. [таблицу приоритетов операций](https://pvs-studio.ru/ru/blog/terms/0064/)\), поэтому в действительности сначала случайное число будет сравниваться с _3_, а затем результат этого сравнения \(_0_ или _1_\) будет записан в переменную _roll_\.

Данную ошибку можно исправить таким образом:

```cpp
uint32 roll = urand(0, 99);
if (roll < 3)
{
  ....
}
```

## Одинаковые действия в блоках if и else

**Предупреждение PVS\-Studio:** [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. SpellAuras\.cpp 1537

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'm\_uiMovePoint' variable is modified while being used twice between sequence points\. boss\_onyxia\.cpp 405

```cpp
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_ дважды изменяется в рамках одной [точки следования](https://en.wikipedia.org/wiki/Sequence_point), что приводит к неопределенному поведению программы\. Более подробно об этом можно почитать в описании диагностики [V567](https://pvs-studio.ru/ru/docs/warnings/v567/)\.

Аналогичная ошибка:

* V567 Undefined behavior\. The 'm\_uiCrystalPosition' variable is modified while being used twice between sequence points\. boss\_ossirian\.cpp 150

## Ошибка в условии

**Предупреждение PVS\-Studio:** [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression is always false\. Probably the '\|\|' operator should be used here\. SpellEffects\.cpp 2872

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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_\.

Если же такое поведение и задумывалось, то стоит поправить выравнивание кода:

```cpp
if (m_auiEncounter[11] == DONE)
  pCreature->SetFactionTemporary(....);
pCreature->SetStandState(UNIT_STAND_STATE_STAND);
```

## Одинаковые операнды в тернарном операторе

**Предупреждение PVS\-Studio:** [V583](https://pvs-studio.ru/ru/docs/warnings/v583/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: SAY\_BELNISTRASZ\_AGGRO\_1\. razorfen\_downs\.cpp 104

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v674/) The '0\.1f' literal of the 'float' type is compared to a value of the 'unsigned int' type\. item\_scripts\.cpp 44

```cpp
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% границу здоровья, данный код можно переписать, например, так:

```cpp
// 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;
}
```

## Безусловный выход из цикла for

**Предупреждение PVS\-Studio:** [V612](https://pvs-studio.ru/ru/docs/warnings/v612/) An unconditional 'break' within a loop\. Pet\.cpp 1956

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v728/) An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions '\!realtimeonly' and 'realtimeonly'\.  Player\.cpp 10536

```cpp
void Player::UpdateItemDuration(uint32 time, bool realtimeonly)
{
  ....
  if ((realtimeonly && (....)) || !realtimeonly) // <=
    item->UpdateDuration(this, time);
  ....
}
```

Проверку вида \(_a_ && _b_\) \|\| \!_a_ можно упростить до \!_a_ \|\| _b_, что наглядно видно на таблице истинности:

![0476_CMaNGOS_ru/image2.png](https://import.viva64.com/docx/blog/0476_CMaNGOS_ru/image2.png)

Таким образом, исходное выражение упростится до:

```cpp
void Player::UpdateItemDuration(uint32 time, bool realtimeonly)
{
  ....
  if (!(realtimeonly) || (....))
    item->UpdateDuration(this, time);
  ....
}
```

## Проверка this на null

**Предупреждение PVS\-Studio:** [V704](https://pvs-studio.ru/ru/docs/warnings/v704/) '\!this \|\|\!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. Unit\.cpp 1417

```cpp
void Unit::CalculateSpellDamage(....)
{
  ....
  if (!this || !pVictim) // <=
    return;
  ....
}
```

Согласно современному стандарту С\+\+, указатель _this_ никогда не может быть нулевым\. Зачастую использование сравнения _this_ с нулем может приводить к неожиданным ошибкам\. Подробнее прочитать об этом можно в описании диагностики [V704](https://pvs-studio.ru/ru/docs/warnings/v704/)\.

Аналогичные проверки:

* V704 '\!this \|\|\!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. Unit\.cpp 1476
* V704 '\!this \|\|\!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. Unit\.cpp 1511
* V704 '\!this \|\|\!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. Unit\.cpp 1797

## Неоправданная передача по ссылке

**Предупреждение PVS\-Studio:** [V669](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'stat' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1776, 1781\. DetourNavMeshQuery\.cpp 1781

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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 in testing 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 in testing 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 in testing 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](https://pvs-studio.ru/ru/docs/warnings/v764/) Possible incorrect order of arguments passed to 'loadVMap' function: 'tileY' and 'tileX'\. MapBuilder\.cpp 279

```cpp
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\(\)_, то становится очевидно, что это действительно ошибка\.

```cpp
bool loadVMap(uint32 mapID, 
              uint32 tileX, uint32 tileY,
              MeshData& meshData);
```

## Два идентичных блока кода

**Предупреждение PVS\-Studio:** [V760](https://pvs-studio.ru/ru/docs/warnings/v760/) Two identical blocks of text were found\. The second block begins from line 213\. BattleGround\.cpp 210

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v571/) Recurring check\. The 'isDirectory' condition was already verified in line 166\. FileSystem\.cpp 169

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v616/) The 'SPELL\_DAMAGE\_CLASS\_NONE' named constant with the value of 0 is used in the bitwise operation\. Spell\.cpp 674

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'model' pointer was utilized before it was verified against nullptr\. Check lines: 303, 305\. MapTree\.cpp 303

```cpp
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 поправят все недочеты, а так же начнут постоянно использовать статический анализ в своем проекте, поскольку разовая проверка не так уж эффективна\.

Также напомню, что теперь любой желающий может воспользоваться возможностью [бесплатного](https://pvs-studio.ru/ru/blog/posts/0457/) использования PVS\-Studio при соблюдении описанных по ссылке условий\.

P\.S\. Вы можете предложить для проверки нашим анализатором любой интересный для вас проект через форму обратной связи или с помощью GitHub\. Подробности по [ссылке](https://pvs-studio.ru/ru/blog/posts/0473/)\.