In this article, I would like to share the results of our analysis of the open source implementation of the World of Warcraft server, CMaNGOS, as done by the PVS-Studio static analyzer.
Introduction
C(ontinued)MaNGOS is an actively developing offshoot of an old project: MaNGOS (Massive Network Game Object Server), which was made in order to create an alternative server for the World of Warcraft game. The majority of MaNGOS developers continue working in CMaNGOS.
According to the developers themselves, their goal is to create a "well written server in C++" for one of the best MMORPGs. I'll try to help them with this, by checking CMaNGOS using the static analyzer, PVS-Studio.
Note: To do the analysis we used CMaNGOS-Classic server, available in the project repository on GitHub.
The analysis results
An error in the operation precedence
PVS-Studio warning: 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)
....
....
}
The author assumed that the roll variable would be assigned with a random value, and then this value would be compared with 3. However, the priority of the comparison operation is higher than of the assignment operation (see the table "Operation priorities in C/C++", so the random number will be compared with 3 first, and then the result of the comparison (0 or 1) will be written to the roll variable.
This error can be corrected by placing parentheses thusly:
if ((uint32 roll = urand(0, 99)) < 3)Similar actions in the if and else blocks
PVS-Studio warning: 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; // <=
....
}
....
}
In both blocks the variable modelid is assigned with the same value; most likely, this is an error and the constant in one of the blocks should be replaced with another.
Undefined behavior
PVS-Studio warning: 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;
}
....
}
In the specified string the variable m_uiMovePoint is modified twice within one sequence point, which leads to undefined behavior of the program. You may find more information in the description of the V567 diagnostic.
A similar error:
- V567 Undefined behavior. The 'm_uiCrystalPosition' variable is modified while being used twice between sequence points. boss_ossirian.cpp 150
An error in the condition
PVS-Studio warning: 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;
....
}
In the specified condition, the variable m_spellInfo->Id is verified against two different values at the same time. The result of this check is always false, of course. The author most likely made a mistake and instead of '||' operator used '&&'.
It should be noted that the program commented on strange code behavior, and perhaps, it was caused by this error exactly.
There were several errors like this, here is the full list:
- 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
Suspicious formatting
PVS-Studio warning: 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:
....
}
}
The author may have forgotten to put curly braces after the if statement, which caused the call pCreature->SetStandState(UNIT_STAND_STATE_STAND) to be executed regardless of the if condition.
If this behavior was meant intentionally, then the formatting should be corrected:
if (m_auiEncounter[11] == DONE)
pCreature->SetFactionTemporary(....);
pCreature->SetStandState(UNIT_STAND_STATE_STAND);Similar operands in the ternary operator
PVS-Studio warning: 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;
}
....
}
The second and third operands of the ternary operator are identical; this is most likely an error. Judging by the code of the project, we can assume that one of the operands should have the value SAY_BELNISTRASZ_AGGRO_2.
Integer division
PVS-Studio warning: 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;
}
The method Unit::GetHealth() returns the value of the uint32_t type, and the method Unit::GetMaxHealth() also returns the value of the uint32_t type, so the result of the division is an integer, and it's pointless comparing it with 0.1f.
To correctly identify 10% of the health, this code can be rewritten like this:
// 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;
}Unconditional exit from the for loop
PVS-Studio warning: 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; // <=
}
....
}
It was not clear what was meant here, but an unconditional break statement in the body of the for loop looks very suspicious. Even if there is no error here, it's better to refactor the code, and get rid of the unnecessary loop, because the iterator _spell_idx takes a single value.
The same warning:
- V612 An unconditional 'break' within a loop. Pet.cpp 895
Redundant condition
PVS-Studio warning: 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);
....
}
The check (a && b) || !a can be simplified to !a || b, which can be seen in the truth table:
Thus, the original expression can be simplified to:
void Player::UpdateItemDuration(uint32 time, bool realtimeonly)
{
....
if (!(realtimeonly) || (....))
item->UpdateDuration(this, time);
....
}Testing this for null
PVS-Studio warning: 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;
....
}
According to modern C++ standards, the "this" pointer can never be null. Often, comparing this with zero can cause unexpected errors. You may find more information about this in the description of the V704 diagnostic.
Similar checks:
- 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
Unjustified passing by reference
PVS-Studio warning: 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());
}
}
The variable uiHealedAmount is passed by reference, but is not changed in the body of the function. This can be misleading, because we get the impression that the HealedBy() function writes something to the uiHealedAmount. It would be better to pass the variable by a constant reference or by value.
Repeated assignment
PVS-Studio warning: 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);
....
}
The analyzer detected a suspicious fragment, where the stat variable is assigned with different values twice. This code is definitely worth reviewing.
Verifying a pointer against null after new
PVS-Studio warning: 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;
}
....
}
Verification of a pointer against null is pointless after the new operator. If it is impossible to allocate memory, the new operator throws an exception std::bad_alloc(), it doesn't return nullptr. Which means that the program will never enter the block after the condition.
To correct this error, we could allocate the memory in the try {....} catch(const std::bad_alloc &) {....} block or use the new(std::nothrow) construction for allocation of the memory, which won't throw exceptions in the case of a failure.
Similar checks of the pointers:
- 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
Incorrect order of arguments
PVS-Studio warning: 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);
....
}
The analyzer detected suspicious passing of arguments to the function - the arguments tileX and tileY swapped places.
If we have a look at the prototype of the function loadVMap(), then we can clearly see that this is an error.
bool loadVMap(uint32 mapID,
uint32 tileX, uint32 tileY,
MeshData& meshData);Two identical blocks of code
PVS-Studio warning: 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;
....
}
The same actions are performed twice in this fragment. This code is most likely as a result of using Copy-Paste.
Duplicate condition
PVS-Studio warning: 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 condition is checked twice. We can remove the duplicate check.
Bit wise AND with a null constant
PVS-Studio warning: 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;
}
}
....
}
The constant SPELL_DAMAGE_CLASS_NONE has a null value, and the bitwise AND of any number and null is a null, therefore the condition will always be false, and the next block will never be executed.
A similar error:
- V616 The 'SPELL_DAMAGE_CLASS_NONE' named constant with the value of 0 is used in the bitwise operation. Spell.cpp 692
Potential dereference of a null pointer
PVS-Studio warning: 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) // <=
{
....
}
....
}
The pointer model is verified against null; i.e. it can be equal to zero, however the pointer is used earlier without any checks. It's clear that it is a potential null pointer dereference.
To correct this error, you should check the value of the model pointer before calling a method model->setModelFlags(spawn.flags).
Similar warnings:
- 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
Conclusion
As always, PVS-Studio found a large number of suspicious places and errors in the code. I hope the CMaNGOS developers will fix all these flaws, and will also start using static analysis on a regular basis, because a one-time check is not that effective.
Also, I should remind you that everyone can use PVS-Studio static analyzer for free, upon certain conditions, described on the site.
P.S. You can offer to check any interesting project with our analyzer using feedback form or GitHub. You can find all the details here.
No comments:
Post a Comment