Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
В мире антропоморфных животных: PVS-Stu…

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

23 Июн 2022

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

0957_overgrowth_ru/image1-47817e652e091584381add51b0d46c80.png

О проекте

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. Это безусловно поможет замечательной игре и дальше радовать своих фанатов в уже новых, менее подверженных багам сборках :)

Популярные статьи по теме
Нереальный baselining или доработки PVS-Studio для Unreal Engine проектов

Дата: 26 Апр 2022

Автор: Валерий Комаров

Статический анализатор PVS-Studio постоянно развивается: улучшаются различные механизмы, происходит интеграция с игровыми движками, IDE, CI/CD и другими системами и сервисами. Благодаря этому несколь…
Проверка фреймворка Ogre3D статическим анализатором PVS-Studio

Дата: 16 Мар 2022

Автор: Григорий Семенчев

Обычные пользователи любят графические движки, потому что с ними удобно работать. Команда PVS-Studio любит графические движки, потому что там часто попадаются интересные фрагменты кода. По просьбе од…
Спасибо, Марио, но код стоит поправить – проверка TheXTech

Дата: 22 Ноя 2021

Автор: Алексей Говоров

Здорово, когда энтузиастам-разработчикам удаётся сделать работающий клон известной игры. Ещё лучше, когда находятся люди, готовые продолжить развитие таких проектов! В этой статье с помощью PVS-Studi…
Прокачка статического анализа проектов на Unreal Engine 4 и проверка автосимулятора Carla

Дата: 19 Ноя 2021

Автор: Константин Кочкин

Одним из механизмов статического анализа является аннотирование методов популярных библиотек. Аннотации позволяют обладать большей информацией при диагностировании ошибок в коде. Впечатляющий свободн…
Йо-хо-хо, и бутылка рому! Разбираем ошибки игрового движка Storm Engine

Дата: 25 Июн 2021

Автор: Александр Куренев

PVS-Studio – это инструмент статического анализа, позволяющий находить ошибки в исходном коде программ. В качестве знакомства с возможностями анализатора вашему вниманию предлагается результат провер…

Комментарии (0)

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