PVS-Studio checks OpenMW: everything is not smooth in the Morrowind universe

I checked the OpenMW project with PVS-Studio and wrote this tiny article. There were too few errors. But I was asked to write about the verification of this project article, and here it is.

Openmw

OpenMW is an attempt to recreate the popular RPG Morrowind, a complete implementation of all the features of the game with open source. To run OpenMW, you need the original Morrowind CD.

Source code available at: https://code.google.com/p/openmw/

Suspicious places found

Fragment N1

std::string getUtf8(unsigned char c, ToUTF8::Utf8Encoder& encoder, ToUTF8::FromType encoding) { .... conv[0xa2] = 0xf3; conv[0xa3] = 0xbf; conv[0xa4] = 0x0; conv[0xe1] = 0x8c; conv[0xe1] = 0x8c; <<<<==== conv[0xe3] = 0x0; .... } 

PVS-Studio warning: V519 The 'conv [0xe1]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 103, 104. openmw fontloader.cpp 104
')
This is probably a typo. In the highlighted line, the index 0xe2 is most likely to be used.

Fragment N2

 enum Flags { .... NoDuration = 0x4, .... } bool CastSpell::cast (const ESM::Ingredient* ingredient) { .... if (!magicEffect->mData.mFlags & ESM::MagicEffect::NoDuration) .... } 

PVS-Studio warning: V564 The '&' operator is applied to bool type value. You've probably forgotten to include the operator. openmw spellcasting.cpp 717

The error is related to the priority of operations. In the beginning, the action is executed (! MagicEffect-> mData.mFlags). Result: 0 or 1. Then the action is performed: 0 & 4 or 1 & 4. This has no practical meaning. Most likely, the code should look like this:

 if ( ! (magicEffect->mData.mFlags & ESM::MagicEffect::NoDuration) ) 

Fragment N3

 void Clothing::blank() { mData.mType = 0; mData.mWeight = 0; mData.mValue = 0; mData.mEnchant = 0; mParts.mParts.clear(); mName.clear(); mModel.clear(); mIcon.clear(); mIcon.clear(); mEnchant.clear(); mScript.clear(); } 

PVS-Studio warning: V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 48, 49. components loadclot.cpp 49

The object 'mIcon' is cleared twice. The second cleaning is superfluous or it was necessary to clean something else.

Fragment N4

 void Storage::loadDataFromStream( ContainerType& container, std::istream& stream) { std::string line; while (!stream.eof()) { std::getline( stream, line ); .... } .... } 

PVS-Studio warning: V663 Infinite loop is possible. The 'cin.eof ()' condition is not a loop from the loop. Consider adding the 'cin.fail ()' function call to the conditional expression. components translation.cpp 45

When working with the 'std :: istream' class, it is not enough to call the 'eof ()' function to end the loop. In case of a failure in reading data, the call to the 'eof () function will always return the value' false '. To complete the loop in this case, additional verification of the value returned by the 'fail ()' function is necessary.

Fragment N5

 class Factory { .... bool getReadSourceCache() { return mReadSourceCache; } bool getWriteSourceCache() { return mReadSourceCache; } .... bool mReadSourceCache; bool mWriteSourceCache; .... }; 

PVS-Studio warning: V524 It is odd that the body of the getWriteSourceCache function is fully equivalent to the body of the getReadSourceCache function. components factory.hpp 209

It seems to me that the function getWriteSourceCache () should be like this:

 bool getWriteSourceCache() { return mWriteSourceCache; } 

Fragment N6, N7, N8

 std::string rangeTypeLabel(int idx) { const char* rangeTypeLabels [] = { "Self", "Touch", "Target" }; if (idx >= 0 && idx <= 3) return rangeTypeLabels[idx]; else return "Invalid"; } 

PVS-Studio warning: V557 Array overrun is possible. The value of 'idx' index could reach 3. esmtool labels.cpp 502

Incorrect array index check. If the variable 'idx' is equal to 3, then the array will overrun.

Right:

 if (idx >= 0 && idx < 3) 

A similar situation occurred in two more places:

• V557 Array overrun is possible. The value of 'idx' index could reach 143. esmtool labels.cpp 391
• V557 Array overrun is possible. The value of 'idx' index could reach 27. esmtool labels.cpp 475

Fragment N9

 enum UpperBodyCharacterState { UpperCharState_Nothing, UpperCharState_EquipingWeap, UpperCharState_UnEquipingWeap, .... }; bool CharacterController::updateWeaponState() { .... if((weaptype != WeapType_None || UpperCharState_UnEquipingWeap) && animPlaying) .... } 

PVS-Studio Warning: V560 A part of conditional expression is always true: UpperCharState_UnEquipingWeap. openmw character.cpp 949

This condition is very suspicious. Now it can be simplified to: "if (animPlaying)". Obviously something is wrong.

Fragment N10, N11

 void World::clear() { mLocalScripts.clear(); mPlayer->clear(); .... if (mPlayer) .... } 

PVS-Studio warning: V595 The mPlayer 'pointer was used against nullptr. Check lines: 234, 245. openmw worldimp.cpp 234

Similarly: V595 The mBody pointer was used before it was verified against nullptr. Check lines: 95, 99. openmw physic.cpp 95

Fragment N12

 void ExprParser::replaceBinaryOperands() { .... if (t1==t2) mOperands.push_back (t1); else if (t1=='f' || t2=='f') mOperands.push_back ('f'); else std::logic_error ("failed to determine result operand type"); } 

PVS-Studio warning: V596 The object was not created. The 'throw' keyword could be missing: throw logic_error (FOO); components exprparser.cpp 101

Forgot the 'throw' keyword. The code should be:

 throw std::logic_error ("failed to determine result operand type"); 

Conclusion

The acquisition of PVS-Studio and the implementation of this tool in the development process will speed up the development process, detect errors early and, as a result, make the project better.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. PVS-Studio Checks OpenMW: Not All is Fine in the Morrowind Universe .