PVS-Studio проверяет OpenMW: во вселенной Morrowind не всё гладко

Я проверил проект OpenMW с помощью PVS-Studio и написал эту крошечную статью. Нашлось слишком мало ошибок. Но меня просили написать про проверку этого проекта статью, и вот она.

OpenMW

OpenMW — это попытка воссоздать популярную RPG Morrowind, полноценная реализация всех особенностей игры с открытым исходным кодом. Для запуска OpenMW понадобится оригинальный диск Morrowind.

Исходный код доступен по адресу: https://code.google.com/p/openmw/

Найденные подозрительные места

Фрагмент 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: V519 The 'conv[0xe1]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 103, 104. openmw fontloader.cpp 104

Наверное, это опечатка. В выделенной строке, скорее всего должен использоваться индекс 0xe2.

Фрагмент N2

enum Flags
{
  ....
  NoDuration = 0x4,
  ....
}

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

Предупреждение PVS-Studio: 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

Ошибка связана с приоритетом операций. В начале, выполняется действие (!magicEffect->mData.mFlags). Результат: 0 или 1. Затем выполняется действие: 0 & 4 или 1 & 4. Это не имеет никакого практического смысла. Скорее всего, код должен выглядеть так:

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

Фрагмент 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: V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 48, 49. components loadclot.cpp 49

Два раза очищается объект 'mIcon'. Вторая очистка лишняя или надо было очистить что-то ещё.

Фрагмент N4

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

Предупреждение 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

При работе с классом 'std::istream' недостаточно вызова функции 'eof()' для завершения цикла. В случае возникновения сбоя при чтении данных, вызов функции 'eof()' будет всегда возвращать значение 'false'. Для завершения цикла в этом случае необходима дополнительная проверка значения, возвращаемого функцией 'fail()'.

Фрагмент N5

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

Предупреждение PVS-Studio: V524 It is odd that the body of 'getWriteSourceCache' function is fully equivalent to the body of 'getReadSourceCache' function. components factory.hpp 209

Мне кажется, функция getWriteSourceCache() должна быть такой:

bool getWriteSourceCache() { return mWriteSourceCache; }

Фрагмент 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: V557 Array overrun is possible. The value of 'idx' index could reach 3. esmtool labels.cpp 502

Неправильная проверка индекса массива. Если переменная 'idx' будет равна 3, то произойдёт выход за границу массива.

Правильно:

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

Аналогичная ситуация встретилась ещё в двух местах:

• 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

Фрагмент N9

enum UpperBodyCharacterState
{
  UpperCharState_Nothing,
  UpperCharState_EquipingWeap,
  UpperCharState_UnEquipingWeap,
  ....
};

bool CharacterController::updateWeaponState()
{
  ....
  if((weaptype != WeapType_None ||
      UpperCharState_UnEquipingWeap) && animPlaying)
  ....
}

Предупреждение PVS-Studio: V560 A part of conditional expression is always true: UpperCharState_UnEquipingWeap. openmw character.cpp 949

Это условие очень подозрительное. Сейчас его можно упростить до: «if (animPlaying)». Явно что-то не так.

Фрагмент N10, N11

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

Предупреждение PVS-Studio: V595 The 'mPlayer' pointer was utilized before it was verified against nullptr. Check lines: 234, 245. openmw worldimp.cpp 234

Аналогично: V595 The 'mBody' pointer was utilized before it was verified against nullptr. Check lines: 95, 99. openmw physic.cpp 95

Фрагмент 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: 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

Забыли ключевое слово 'throw'. Код должен быть таким:

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

Заключение

Приобретение PVS-Studio и внедрение этого инструмента в процесс разработки позволит ускорить процесс разработки, обнаруживать ошибки на ранних этапах и как следствие – сделать проект качественнее.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio Checks OpenMW: Not All is Fine in the Morrowind Universe.

Прочитали статью и есть вопрос?

Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.