World of Warcraft CMaNGOSのオープンソースサーバーを確認する

この記事では、World of Warcraftゲームサーバー-CMaNGOSのオープン実装のPVS-Studioアナライザーで検証の結果を共有したいと思います。

はじめに

C（継続）MaNGOSは、World of Warcraftの無料の代替サーバーを作成するために設計された、古いMaNGOS（Massive Network Game Object Server）プロジェクトの積極的な開発ブランチです。 ほとんどのMaNGOS開発者は、CMaNGOSプロジェクトに引き続き取り組みます。



開発者自身が書いているように、彼らの目標は、最高のMMORPGの1つに対してオープンな「C ++で書かれたサーバー」を作成することです。 この点について少し手助けをして、PVS-Studio静的アナライザーを使用してCMaNGOSをチェックします。



注：検証のために、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) .... .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

著者は、ランダム変数がロール変数に割り当てられ、この値が3と比較されると想定しました。 ただし、比較操作の優先順位は割り当て操作の優先順位よりも高いため（ 操作優先順位の表を参照）、したがって、実際には、最初に乱数が3と比較され、この比較の結果（ 0または1 ）がロール変数に書き込まれます。



このエラーは次の方法で修正できます。

  uint32 roll = urand(0, 99); if (roll < 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変数に同じ値が割り当てられます。これはおそらくエラーであり、ブロックの1つの定数を他のブロックに置き換える必要があります。

未定義の動作

警告PVS-Studio： V567未定義の動作。 'm_uiMovePoint'変数は、シーケンスポイント間で2回使用されている間に変更されます。 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変数が 2回変化し、プログラムの動作が未定義になります。 詳細については、 V567診断の説明を参照してください 。



同様のエラー：

• V567未定義の動作。 'm_uiCrystalPosition'変数は、シーケンスポイント間で2回使用されている間に変更されます。 boss_ossirian.cpp 150

状態エラー

PVS-Studio警告： V547式は常にfalseです。 おそらく '||' ここで演算子を使用する必要があります。 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の等価性が2つの異なる値について同時にチェックされます。 このようなチェックの結果は、もちろん常にfalseです。 最も可能性が高いのは、作者が間違っていて、演算子「||」ではなく 演算子「&&」を使用しました。



コメントがこのコードブロックの奇妙な動作に言及していることは注目に値します。おそらくこのエラーが原因である可能性があります。



プロジェクトでは、さらにいくつかの同様のエラーが見つかりました。それらの完全なリストを以下に示します。

• 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; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

三項演算子の2番目と3番目のオペランドは同じです。これはおそらくエラーです。 プロジェクトコードから判断すると、オペランドの1 つに値SAY_BELNISTRASZ_AGGRO_2が必要であると想定できます。

整数除算

PVS-Studio警告： V674 「float」タイプの「0.1f」リテラルは、「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; // <= } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここで何が意味されているかは完全には明らかではありませんが、 forループの本体の無条件のbreakステートメントは非常に疑わしく見えます。 _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 ）|| ！ に簡単にできます！ || b 、真理値表に明確に表示されます：









したがって、元の式は次のように単純化されます。

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

nullをチェックする

PVS-Studioの 警告 ： V704 '！This ||！PVictim'式は避ける必要があります。新しいコンパイラーでは 'this'ポインターがNULLになることはありません。 Unit.cpp 1417

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

最新のC ++標準によれば、 thisポインターは決してnullにできません。 多くの場合、 この比較をゼロで使用すると、予期しないエラーが発生する可能性があります。 詳細については、 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」変数には連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：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); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは、変数variableに2つの異なる値が連続して割り当てられている疑わしい場所を検出しました。 このセクションのエラーをチェックすることは間違いなく価値があります。

新しい後のnullへのポインタを確認する

PVS-Studio警告： V668メモリが「新しい」演算子を使用して割り当てられたため、「pmmerge」ポインタをnullに対してテストしても意味がありません。 メモリ割り当てエラーの場合、例外が生成されます。 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 {....} block catch（const std :: bad_alloc＆）{....}でメモリを割り当てるか、 新しい（std :: nothrow）コンストラクトを使用します。失敗した場合に例外をスローします。



同様のポインターチェック：

• V668「new」演算子を使用してメモリが割り当てられたため、「data」ポインターをnullに対してテストしても意味がありません。 メモリ割り当てエラーの場合、例外が生成されます。 loadlib.cpp 36
• V668「new」演算子を使用してメモリが割り当てられたため、「dmmerge」ポインターをnullに対してテストしても意味がありません。 メモリ割り当てエラーの場合、例外が生成されます。 MapBuilder.cpp 560
• V668メモリは「new」演算子を使用して割り当てられたため、「m_session」ポインターをnullに対してテストする意味はありません。 メモリ割り当てエラーの場合、例外が生成されます。 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);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

2つの同一のコードブロック

PVS-Studio警告： V760 2つの同一のテキストブロックが見つかりました。 2番目のブロックは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; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここでは、同じアクションが連続して2回実行されます。 ほとんどの場合、このようなコードはコピーペーストの結果として現れました。

重複条件

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条件は2回チェックされます。 重複する検証を削除できます。

ビット単位およびゼロ定数付き

PVS-Studio警告： V616値「0」の「SPELL_DAMAGE_CLASS_NONE」という名前の定数がビット演算で使用されます。 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はゼロであり、任意の数とゼロのビット単位のANDはゼロであるため、条件は常にfalseになり、それに続くブロックは実行されません。



同様のエラー：

• V616値が0の「SPELL_DAMAGE_CLASS_NONE」という名前の定数がビット演算で使用されます。 Spell.cpp 692

潜在的なNULLポインターの逆参照

PVS-Studio警告： V595 nullmodelに対して検証される前に、「モデル」ポインターが使用されました。 行をチェック：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) // <= { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

モデルポインターのnullがチェックされます 。 ゼロへの等値は許可されますが、上記のポインターはチェックなしですでに使用されています。 nullポインターの逆参照が発生する可能性があります。



このエラーを修正するには、 model-> setModelFlags（spawn.flags）メソッドを呼び出す前に、 モデルポインターの値を確認する必要があります。



同様の警告：

• V595 nullptrに対して検証される前に、「モデル」ポインターが使用されました。 行を確認してください：374、375。MapTree.cpp 374
• V595 nullptrに対して検証される前に、「ユニット」ポインターが使用されました。 行を確認してください：272、290。Object.cpp 272
• V595 nullptrに対して検証される前に、「updateMask」ポインターが使用されました。 行を確認してください：351、355。Object.cpp 351
• V595 nullptrに対して検証される前に、「dbcEntry1」ポインターが使用されました。 行を確認してください：7123、7128。ObjectMgr.cpp 7123

おわりに

PVS-Studioは常に、コード内に多くの疑わしい場所とエラーを発見しました。 CMaNGOSの開発者がすべての欠点を修正し、1回限りのチェックはそれほど効果的ではないため、プロジェクトで常に静的分析を使用し始めることを望みます。



また、参照によって説明されている条件に従って、誰でもPVS-Studioの無料使用を利用できるようになったことを思い出します。



PSフィードバックフォームまたはGitHubを使用してアナライザーで検証することにより、興味のあるプロジェクトを提供できます。 リンクの詳細。





英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Bredikhin Egor。 World of Warcraft CMaNGOSオープンソースサーバーの確認 。