Checking the World of Warcraft CMaNGOS open source server

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.

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.

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

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

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

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

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:

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

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

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.

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

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

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

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.