PVS-Studio checks OpenMW: everything is not going smoothly in the Morrowind universe / PVS-Studio Blog

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

Openmw

OpenMW is an attempt to recreate the popular RPG Morrowind, a full-fledged implementation of all the features of an open source game. To run OpenMW you will need the original Morrowind disc.

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

Suspicious Locations 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 should most likely 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 parentheses or intended to use the '&&' operator. openmw spellcasting.cpp 717 The

error is related to the priority of operations. At the beginning, the action (! MagicEffect-> mData.mFlags) is executed. Result: 0 or 1. Then the action is performed: 0 & 4 or 1 & 4. This makes no practical sense. 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

Two times, the 'mIcon' object is cleared. The second cleaning is unnecessary or something else had to be cleaned.

Fragment N4

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

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

When working with the class 'std :: istream', it is not enough to call the function 'eof ()' to complete the loop. In the event of a failure while reading data, calling the 'eof ()' function will always return the value 'false'. To complete the loop in this case, an additional check of the value returned by the function 'fail ()' 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 'getWriteSourceCache' function is fully equivalent to the body of 'getReadSourceCache' function. components factory.hpp 209

It seems to me that the getWriteSourceCache () function 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 be exceeded.

Correctly:

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 utilized before it was verified against nullptr. Check lines: 234, 245. openmw worldimp.cpp 234

Similarly: V595 The 'mBody' pointer was utilized 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 created but it is not being used. The 'throw' keyword could be missing: throw logic_error (FOO); components exprparser.cpp 101

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

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

Conclusion

The acquisition of PVS-Studio and the introduction of this tool in the development process will speed up the development process, detect errors at an early stage 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 .