Проверка сервера с открытым исходным кодом World of Warcraft CMaNGOS

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

Введение

C (продолжение) MaNGOS - это активно развивающееся ответвление старого проекта MaNGOS (Massive Network Game Object Server), который был создан для создания альтернативного сервера для игры World of Warcraft. Большинство разработчиков MaNGOS продолжают работать в CMaNGOS.

По словам самих разработчиков, их цель - создать «хорошо написанный сервер на C ++» для одной из лучших MMORPG. Я постараюсь им в этом помочь, проверив CMaNGOS с помощью статического анализатора PVS-Studio.

Примечание: для анализа мы использовали сервер CMaNGOS-Classic, доступный в репозитории проекта на GitHub.

Результаты анализа

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

Предупреждение PVS-Studio: V593 Рассмотрите возможность просмотра выражения типа A = B‹ C . Выражение рассчитывается следующим образом: 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. Однако приоритет операции сравнения выше, чем у операции присваивания (см. таблица Приоритеты операций в C / C ++, поэтому случайное число сначала будет сравниваться с 3, а затем результат сравнения (0 или 1) будет записан в переменную roll.

Эту ошибку можно исправить, разместив круглые скобки следующим образом:

if ((uint32 roll = urand(0, 99)) < 3)

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

Предупреждение PVS-Studio: V523 Оператор then эквивалентен оператору else. 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 Неопределенное поведение. Переменная m_uiMovePoint модифицируется, когда используется дважды между точками последовательности. 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 Неопределенное поведение. Переменная m_uiCrystalPosition изменяется, когда используется дважды между точками последовательности. boss_ossirian.cpp 150

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

Предупреждение PVS-Studio: Выражение V547 всегда ложно. Вероятно, здесь следует использовать оператор ||. 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 проверяется на соответствие двум различным значениям одновременно. Конечно, результат этой проверки всегда ложный. Скорее всего, автор ошибся и вместо оператора «||» использовал «&&».

Следует отметить, что программа прокомментировала странное поведение кода, и, возможно, оно было вызвано именно этой ошибкой.

Таких ошибок было несколько, вот полный список:

• V547 Выражение всегда ложно. Вероятно, здесь следует использовать оператор «||». SpellEffects.cpp 2872
• V547 Выражение всегда верно. Вероятно, здесь следует использовать оператор «&&». genrevision.cpp 261
• V547 Выражение всегда верно. Вероятно, здесь следует использовать оператор «&&». vmapexport.cpp 361
• V547 Выражение всегда верно. Вероятно, здесь следует использовать оператор «&&». MapTree.cpp 125
• V547 Выражение всегда верно. Вероятно, здесь следует использовать оператор «&&». MapTree.cpp 234

Подозрительное форматирование

Предупреждение PVS-Studio: V640. Логика работы кода не соответствует его форматированию. Оператор смещен вправо, но выполняется всегда. Возможно, что фигурные скобки отсутствуют. 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 Оператор ?:, независимо от его условного выражения, всегда возвращает одно и то же значение: 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 Литерал 0.1f типа float сравнивается со значением типа unsigned int. 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 Безусловный разрыв внутри цикла. 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 Безусловный «разрыв» внутри цикла. Pet.cpp 895

Резервное состояние

Предупреждение PVS-Studio: V728 Излишнюю проверку можно упростить. Оператор || окружен противоположными выражениями ! Realtimeonly и 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’ следует избегать: указатель ‘this’ никогда не может иметь значение NULL на новых компиляторах. Unit.cpp 1417

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

Согласно современным стандартам C ++, указатель this никогда не может быть нулевым. Часто сравнение this с нулем может вызвать непредвиденные ошибки. Подробнее об этом читайте в описании диагностики V704.

Подобные проверки:

• V704 следует избегать выражения «! This ||! PVictim»: указатель «this» никогда не может иметь значение NULL в новых компиляторах. Unit.cpp 1476
• V704 следует избегать выражения «! This ||! PVictim»: указатель «this» никогда не может иметь значение NULL в новых компиляторах. Unit.cpp 1511
• V704 следует избегать выражения «! This ||! PVictim»: указатель «this» никогда не может иметь значение NULL в новых компиляторах. Unit.cpp 1797

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

Предупреждение PVS-Studio: V669 Аргумент uiHealedAmount - это непостоянная ссылка. Анализатор не может определить позицию, в которой этот аргумент изменяется. Возможно, функция содержит ошибку. 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 переменной stat дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 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 Нет смысла проверять указатель pmmerge на нуль, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. 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 генерирует исключение std :: bad_alloc (), он не возвращает nullptr. Это означает, что программа никогда не войдет в кадр после условия.

Чтобы исправить эту ошибку, мы могли бы выделить память в блоке try {….} Catch (const std :: bad_alloc &) {….} или использовать new (std :: nothrow ) конструкция для выделения памяти, которая не будет генерировать исключения в случае сбоя.

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

• V668 Нет смысла проверять указатель «данные» на нуль, так как память была выделена с помощью оператора «новый». Исключение будет сгенерировано в случае ошибки выделения памяти. loadlib.cpp 36
• V668 Нет смысла проверять указатель «dmmerge» на нуль, так как память была выделена с помощью оператора «new». Исключение будет сгенерировано в случае ошибки выделения памяти. MapBuilder.cpp 560
• V668 Нет смысла проверять указатель «m_session» на нуль, так как память была выделена с помощью оператора «new». Исключение будет сгенерировано в случае ошибки выделения памяти. WorldSocket.cpp 426

Неправильный порядок аргументов

Предупреждение PVS-Studio: V764 Возможный неправильный порядок аргументов, переданных в функцию loadVMap: tileY и 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 Обнаружены два идентичных блока текста. Второй блок начинается со строки 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 Периодическая проверка. Условие isDirectory уже было проверено в строке 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 Именованная константа SPELL_DAMAGE_CLASS_NONE со значением 0 используется в побитовой операции. 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 имеет нулевое значение, а побитовое И любого числа и null является нулевым, поэтому условие всегда будет ложным, и следующий блок никогда не будет выполнен.

Похожая ошибка:

• V616 Именованная константа SPELL_DAMAGE_CLASS_NONE со значением 0 используется в побитовой операции. Spell.cpp 692

Возможное разыменование нулевого указателя

Предупреждение PVS-Studio: V595 Указатель модель использовался до того, как он был проверен на соответствие nullptr. Проверьте строки: 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 проверяется на нулевое значение; т.е. может быть равен нулю, однако указатель используется ранее без каких-либо проверок. Понятно, что это потенциальное разыменование нулевого указателя.

Чтобы исправить эту ошибку, вы должны проверить значение указателя model перед вызовом метода model- ›setModelFlags (spawn.flags).

Подобные предупреждения:

• V595 Указатель «модель» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 374, 375. MapTree.cpp 374
• V595 Указатель «единицы» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 272, 290. Object.cpp 272
• V595 Указатель «updateMask» использовался до проверки на nullptr. Проверить строки: 351, 355. Object.cpp 351
• V595 Указатель dbcEntry1 использовался до того, как он был проверен на соответствие nullptr. Проверьте строки: 7123, 7128. ObjectMgr.cpp 7123

Вывод

Как всегда, PVS-Studio обнаружил в коде большое количество подозрительных мест и ошибок. Я надеюсь, что разработчики CMaNGOS исправят все эти недостатки, а также начнут регулярно использовать статический анализ, потому что разовая проверка не так эффективна.

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

P.S. Вы можете предложить проверить любой интересный проект с помощью нашего анализатора, используя форму обратной связи или GitHub. Все подробности вы можете найти здесь.