Владислав Столяров

23 Июн 2022

Теги:

#Cpp #GameDev

В мире антропоморфных животных: PVS-Studio проверил Overgrowth

23 Июн 2022

Автор: Владислав Столяров

О проекте
Результаты проверки
Заключение

Недавно в сети появилась новость о том, что был открыт исходный код игры Overgrowth. Мы не смогли пройти мимо и проверили его качество с помощью PVS-Studio. Давайте же вместе посмотрим, где больше интересного экшена: в игре или в её исходном коде!

О проекте

Overgrowth – вышедшая 14 лет назад игра от компании Wolfire Games. Это экшен с видом от 3-го лица, действие которого происходит в мрачном средневековом мире животных с повадками людей. В игре очень интересная система управления и довольно продвинутый ИИ. В ходе прохождения игроку даётся полная свобода передвижений и организации своих действий. Присутствует многопользовательский режим.

Overgrowth построен на движке Phoenix. Он содержит улучшенную модель движения. Бег, прыжки, перекаты, повороты происходят плавно, а также все позы и анимация изменяются в зависимости от окружения, настроения и даже индивидуальности каждого персонажа. На окружающую среду игры действует погода, даже деревья будут расти быстрее под солнечными лучами.

Анонс Overgrowth произошёл 17 сентября 2008 года, окончательная версия игры вышла 16 октября 2017 года.

Так как с момента публикации исходного кода в проект непрерывно коммитят, я зафиксировал его версией: f2a67f7.

Итак, давайте разберём самые интересные предупреждения, которые удалось найти при помощи проверки проекта PVS-Studio.

Результаты проверки

Предупреждения N1, N2

Начнём, пожалуй, с функции, в которой PVS-Studio выдал два предупреждения на соседние строки кода.

V611 [CERT-MEM51-CPP] The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] heightfieldData;'. PhysicsServerCommandProcessor.cpp 4741
V773 [CERT-MEM31-C, CERT-MEM51-CPP] The function was exited without releasing the 'worldImporter' pointer. A memory leak is possible. PhysicsServerCommandProcessor.cpp 4742

bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
  ....
  const unsigned char* heightfieldData = 0;
  ....
  heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
  ....
  delete heightfieldData;
  return ....;
}

Похоже, что программист, который писал данную функцию, не очень знаком с принципами работы с динамической памятью в С++.

Начнём с предупреждения V773, как наиболее тривиального. Память под указатель worldImporter была выделена при помощи оператора new и по выходу из функции не была освобождена. Это плохая практика, которая приводит к утечке ресурсов. Поправить данный фрагмент кода можно было бы, позвав оператор delete по завершению работы с этим указателем.

Перейдём к предупреждению V611 и буферу heightfieldData. Тут разработчик очистил динамически выделенную память, однако сделал это неправильно. Вместо того, чтоб позвать оператор delete[] для освобождения аллоцированной ранее памяти при помощи оператора new[], он позвал простой delete. Согласно стандарту, такой код явно приведёт к неопределённому поведению, вот ссылка на соответствующий пункт.

Поправленный фрагмент кода:

bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
  ....
  const unsigned char* heightfieldData = 0;
  ....
  heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
  ....

  delete   worldImporter;
  delete[] heightfieldData;
  return ....;
}

Также проблем с ручной очисткой памяти можно избежать, написав код посовременнее. Например, использовав std::unique_ptr для автоматического освобождения памяти. Такой код будет короче и надёжнее. Он защитит от ошибок неосвобождённой памяти при досрочном выходе из функции:

bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  auto worldImporter = std::make_unique<btMultiBodyWorldImporter> ();
  ....
  std::unique_ptr<unsigned char[]> heightfieldData;
  ....
  heightfieldData = std::make_unique_for_overwrite<unsigned char[]>
                                (width * height * sizeof(btScalar));
  ....
  return ....;
}

Предупреждение N3

V772 [CERT-MSC15-C] Calling a 'delete' operator for a void pointer will cause undefined behavior. OVR_CAPI_Util.cpp 380

typedef struct ovrHapticsClip_
{
  const void* Samples;
  ....
} ovrHapticsClip;
....

OVR_PUBLIC_FUNCTION(void) ovr_ReleaseHapticsClip(ovrHapticsClip* hapticsClip)
{
  if (hapticsClip != NULL && hapticsClip->Samples != NULL) 
  {
    delete[] hapticsClip->Samples;
  ....
  }
}

Применение операторов delete и delete[] для указателя на void ведёт к неопределённому поведению. Чтобы избежать ошибки, нужно явно привести указатель к его фактическому типу при очистке памяти.

Чтобы понять, как глубока проблема, я провёл небольшое исследование. Поле Samples инициализируется только в одном месте и типом uint8_t*. Вот пруф:

.... ovr_GenHapticsFromAudioData(ovrHapticsClip* outHapticsClip, ....)
{
  ....
  uint8_t* hapticsSamples = new uint8_t[hapticsSampleCount];
  ....

  outHapticsClip->Samples = hapticsSamples;

  ....
}

Это говорит об архитектурной ошибке при проектировании класса. Возможно, раньше оно инициализировалось разными типами и это убрали вследствие рефакторинга, а изменить тип поля Samples с void* на uint8_t* забыли.

Разработчикам в любом случае стоит обратить внимание на этот участок кода и поправить его, он странный и ведёт к UB.

Предупреждение N4

V595 [CERT-EXP12-C] The 'ctx' pointer was utilized before it was verified against nullptr. Check lines: 130, 131. ascontext.cpp 130

class ASContext
{
public:
  asIScriptContext *ctx;
}

ASContext::ASContext(....)
{
  ctx = ....;
  ctx->SetUserData(this, 0);
  if( ctx == 0 ) 
  {
    FatalError("Error","Failed to create the context.");
    return;
  }
  ....
}

В данном фрагменте кода указатель ctx сначала разыменовывается, а потом проверяется на 0. Это выглядит довольно подозрительно. Если программист опасается, что ctx может быть равен nullptr, то, возможно, его стоит разыменовывать уже после проверки:

ASContext::ASContext(....)
{
  ctx = ....;
  if( !ctx )
  {
    FatalError("Error","Failed to create the context.");
    return;
  }

  ctx->SetUserData(this, 0);
  ....
}

Предупреждение N5

V547 Expression 'connect_id_ == - 1' is always true. placeholderobject.cpp 342

class PlaceholderObject
{
private:
  int connect_id_;
  ....
};

ObjectSanityState PlaceholderObject::GetSanity()
{
  ....
  if( .... && connect_id_ == -1) 
  {
    if( connect_id_ == -1) 
    {
      ....
    } 
  } 
  ....
}

В данном фрагменте кода анализатор заметил избыточную проверку connect_id_ == -1. Она уже была выполнена в условии верхнего уровня, и переменная connect_id_ с этого момента не менялась.

Возможно, в условии, на которое указывает анализатор, должна была проверяться какая-нибудь другая переменная или данная проверка просто избыточна и код можно упростить:

ObjectSanityState PlaceholderObject::GetSanity()
{
  ....
  if( .... && connect_id_ == -1 ) 
  {
      ....
  } 
  ....
}

Предупреждение N6

V791 The initial value of the index in the nested loop equals 'i'. Perhaps, 'i + 1' should be used instead. navmeshhintobject.cpp 65

NavmeshHintObject::NavmeshHintObject()
{
  ....
  for( int i = 0; i < 8; i++ )
  {
    for( int k = i; k < 8; k++ )
    {
      if( i != k )
      {
        if( 
            corners[i][0] == corners[k][0] ||
            corners[i][1] == corners[k][1] ||
            corners[i][2] == corners[k][2] 
          )
          {
            cross_marking.push_back(corners[i]);   
            cross_marking.push_back(corners[k]);   
          }
      }
    }
  }
  ....
}

Здесь анализатор выявил неоптимальный цикл. Используется паттерн кода с выполнением некоторых операций для пар элементов массива. При этом анализатор понял, что нет смысла выполнять операцию для пары, состоящей из одного и того же элемента i == j. Данный фрагмент кода можно упростить:

NavmeshHintObject::NavmeshHintObject()
{
  ....
  for( int i = 0; i < 8; i++ )
  {
    for( int k = i + 1; k < 8; k++ )
    {
      if( 
          corners[i][0] == corners[k][0] ||
          corners[i][1] == corners[k][1] ||
          corners[i][2] == corners[k][2] 
        )
        {
          cross_marking.push_back(corners[i]);   
          cross_marking.push_back(corners[k]);   
        }
    }
  }
  ....
}

Предупреждение N7

V561 [CERT-DCL01-C] It's probably better to assign value to 'other_radius_sq' variable than to declare it anew. Previous declaration: scenegraph.cpp, line 2006. scenegraph.cpp 2010

bool SceneGraph::AddDynamicDecal(....)
{
  ....
  float other_radius_sq = ....;
  if(....)
  {
    ....
    float other_radius_sq = ....;
  }
  ....
}

Анализатор обнаружил подозрительный фрагмент кода. Здесь происходит переопределение переменной other_radius_sq. Зачастую появление сущностей с одинаковыми именами — следствие неудачной копипасты.

Предупреждения N8, N9

V547 Expression 'imageBits == 8' is always false. texture_data.cpp 305
V547 Expression 'imageBits == 24' is always false. texture_data.cpp 313

void TextureData::GetUncompressedData(unsigned char* data) 
{
  int imageBits = 32;
  ....
  if (imageBits == 8)
  {
    ....
  }
  else if (imageBits == 24)
  {
    ....
  }
  ....
}

Значение переменной imageBits не меняется между инициализацией и проверками. Скорее всего, это не ошибка, просто анализатор выявил странный недописанный или избыточный фрагмент кода. Разработчикам стоит обратить на него внимание!

Предупреждения N10, N11

V769 [CERT-EXP08-C] The 'idx_buffer_offset' pointer in the 'idx_buffer_offset += pcmd->ElemCount' expression equals nullptr. The resulting value is senseless and it should not be used. imgui_impl_sdl_gl3.cpp 138

void ImGui_ImplSdlGL3_RenderDrawLists(ImDrawData* draw_data)
{
  const ImDrawIdx* idx_buffer_offset = 0;
  ....
  idx_buffer_offset += pcmd->ElemCount;
  ....
}

Анализатор обнаружил подозрительную операцию сложения, применяемую к нулевому указателю. Указатель далее не используется. Да и нельзя его использовать. В общем, это какой-то бессмысленный код.

Ещё одно похожее срабатывание:

V769 [CERT-EXP08-C] The 'cp' pointer in the 'cp ++' expression equals nullptr. The resulting value is senseless and it should not be used. crn_file_utils.cpp 547

int file_utils::wildcmp(...., const char* pString)
{
  const char* cp = NULL;
  ....
  pString = cp++;
  ....
}

Данный фрагмент также похож на последствия рефакторинга или криво реализованный алгоритм. Что имели в виду разработчики — остаётся только гадать...

Предупреждение N12

V523 The 'then' statement is equivalent to the 'else' statement. skeleton.cpp 152

void Skeleton::SetGravity( bool enable ) 
{
  if(enable)
  {
    for(unsigned i=0; i<physics_bones.size(); i++)
    {
      if(!physics_bones[i].bullet_object)
      {
        continue;
      }
      physics_bones[i].bullet_object->SetGravity(true);
      //physics_bones[i].bullet_object->SetDamping(0.0f);
    }
  } 
  else 
  {
    for(unsigned i=0; i<physics_bones.size(); i++)
    {
      if(!physics_bones[i].bullet_object)
      {
        continue;
      }
      physics_bones[i].bullet_object->SetGravity(true);
      //physics_bones[i].bullet_object->SetDamping(1.0f);
    }
  }
}

В продолжение обзора странного кода. Анализатор обнаружил if с одинаковыми then и else. Скорее всего, разработчик просто не дописал второй фрагмент кода. Об этом можно судить по разным закомментированным фрагментам в ветках кода.

Предупреждение N13

V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. as_compiler.cpp 4317

void asCCompiler::CompileIfStatement(....)
{
  bool constructorCall1 = ....;
  bool constructorCall2 = ....;
  ....
  if (  (constructorCall1 && !constructorCall2) 
      ||(constructorCall2 && !constructorCall1) )
  {
    ....
  }
}

Давайте рассмотрим фрагмент кода, который сам по себе не является ошибкой. На самом деле, мне просто очень нравится эта диагностика. Она проста и изящна.

PVS-Studio обнаружил паттерн в проверяемом условии, который можно упростить — и сделать код существенно читабельнее. Программист пытается понять, что произошёл вызов одного или другого конструктора. Операция, которую он применяет очень похожа на XOR. Однако в C++ нет исключающего "ИЛИ" для типа bool, и порой это выливается в гораздо более громоздкий код. Переписать его можно, например, таким образом:

void asCCompiler::CompileIfStatement(....)
{
  bool constructorCall1 = ....;
  bool constructorCall2 = ....;
  ....
  if (constructorCall1 != constructorCall2)
  {
    ....
  }
}

Предупреждения N14, N15, N16

V610 [CERT-INT34-C] Undefined behavior. Check the shift operator '<<'. The right operand ('i' = [0..63]) is greater than or equal to the length in bits of the promoted left operand. bitarray.cpp 77

class Bitarray 
{
private:
  uint64_t *arr;
  ....
};

void Bitarray::SetBit( size_t index )
{
  size_t p = index/64;
  size_t i = index%64;

  arr[p] |= (1UL << i);
}

PVS-Studio обнаружил опасный код со сдвигом влево беззнакового числа. Согласно стандарту, это неопределённое поведение, если правый операнд больше или равен разрядности левого операнда. Литерал 1UL под MSVC представлен 32 битами, правый операнд же лежит в диапазоне от 0 до 63.

Так как код рассчитан в том числе и для сборки под Windows, разработчикам стоит обратить внимание на него. Вот ещё несколько предупреждений с такой же проблемой:

V610 [CERT-INT34-C] Undefined behavior. Check the shift operator '<<'. The right operand ('i' = [0..63]) is greater than or equal to the length in bits of the promoted left operand. bitarray.cpp 85
V610 [CERT-INT34-C] Undefined behavior. Check the shift operator '<<'. The right operand ('i' = [0..63]) is greater than or equal to the length in bits of the promoted left operand. bitarray.cpp 93

Предупреждение N17

V751 [CERT-MSC13-C] Parameter 'rayTo' is not used inside function body. btSoftBody.cpp 2148

btScalar btSoftBody::RayFromToCaster::rayFromToTriangle(
  const btVector3& rayFrom,
  const btVector3& rayTo,
  const btVector3& rayNormalizedDirection,
  const btVector3& a,
  const btVector3& b,
  const btVector3& c,
  btScalar maxt)
{
  static const btScalar ceps = -SIMD_EPSILON * 10;
  static const btScalar teps = SIMD_EPSILON * 10;

  const btVector3 n = btCross(b - a, c - a);
  const btScalar d = btDot(a, n);
  const btScalar den = btDot(rayNormalizedDirection, n);
  if (!btFuzzyZero(den))
  {
    const btScalar num = btDot(rayFrom, n) - d;
    const btScalar t = -num / den;
    if ((t > teps) && (t < maxt))
    {
      const btVector3 hit = rayFrom + rayNormalizedDirection * t;
      if ((btDot(n, btCross(a - hit, b - hit)) > ceps) &&
          (btDot(n, btCross(b - hit, c - hit)) > ceps) &&
          (btDot(n, btCross(c - hit, a - hit)) > ceps))
      {
        return (t);
      }
    }
  }
  return (-1);
}

Здесь анализатор говорит о формальном параметре rayTo, который не используется в теле функции. При этом параметр rayFrom используется несколько раз, что может свидетельствовать об ошибке или не очень удачном рефакторинге кода.

Заключение

В результате проверки анализатор нашёл в проекте ошибки разного рода: традиционные опечатки, довольно много неправильной работы с памятью, логические ошибки. Мы надеемся, что благодаря данной статье, разработчики Overgrowth смогут поправить некоторые недочёты, а может быть, захотят самостоятельно перепроверить свою кодовую базу PVS-Studio. Это безусловно поможет замечательной игре и дальше радовать своих фанатов в уже новых, менее подверженных багам сборках :)

#Cpp #GameDev