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

• Введение
• Результаты проверки
• Ошибка в приоритете операций
• Одинаковые действия в блоках if и else
• Неопределенное поведение
• Ошибка в условии
• Подозрительное форматирование
• Одинаковые операнды в тернарном операторе
• Целочисленное деление
• Безусловный выход из цикла for
• Избыточное условие
• Проверка this на null
• Неоправданная передача по ссылке
• Повторное присваивание
• Проверка указателя на null после new
• Неправильный порядок аргументов
• Два идентичных блока кода
• Дублирующее условие
• Побитовое И с нулевой константой
• Потенциальное разыменование нулевого указателя
• Заключение

В этой статье мне хотелось бы поделиться результатами проверки статическим анализатором 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)
{
  ....
}

Одинаковые действия в блоках 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.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;
}

Безусловный выход из цикла 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 compilers. 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 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 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 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), которая не будет генерировать исключение в случае неудачи.

Аналогичные проверки указателей:

• 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 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, а блок, следующий за ним никогда не выполнится.

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

• 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. Подробности по ссылке.