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

16 Мар 2022

Теги:

#Cpp #GameDev

Проверка фреймворка Ogre3D статическим анализатором PVS-Studio

16 Мар 2022

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

Введение
Какие ошибки нашёл PVS-Studio?
Не всё так однозначно
Подозрительные места
Заключение

Обычные пользователи любят графические движки, потому что с ними удобно работать. Команда PVS-Studio любит графические движки, потому что там часто попадаются интересные фрагменты кода. По просьбе одного из читателей нашего блога в этой статье будут рассмотрены результаты анализа графического фреймворка Ogre3D. Каждому по интересному срабатыванию из проекта, и пусть никто не уйдёт обиженным!

Введение

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

Ogre3D (Object-Oriented Graphics Rendering Engine) является графическим движком с открытым исходным кодом, который можно найти на GitHub. Проект написан на языке программирования С++ и используется в играх и 3D визуализации.

Какие ошибки нашёл PVS-Studio?

Всего при анализе Ogre3D PVS-Studio выдал 562 срабатывания первого и второго уровней. Были включены только предупреждения общего назначения (GA). Про фильтрацию срабатываний можно узнать в нашей документации. Это не мало, но и не много, учитывая, что большинство срабатываний пришлось на V730. Такое диагностическое правило говорит о том, что не все члены класса инициализируются в конструкторе. Сложно определить, так и задумывалось или нет, для этого необходимо знать тонкости реализации проекта.

Не всё так однозначно

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

V1064 The '1' operand of integer division is less than the '100000' one. The result will always be zero. OgreAutoParamDataSource.cpp 1094

typedef Vector<4, Real> Vector4;

const Vector4& 
  AutoParamDataSource::getShadowSceneDepthRange(size_t index) const
{
  static Vector4 dummy(0, 100000, 100000, 1/100000);
  // ....
}

Здесь вектор dummy должен хранить числа с плавающей точкой. В данном случае конструктор принимает 4 аргумента типа float. Однако результат операции 1/100000 будет равен не дроби, а нулю, поскольку слева и справа от оператора деления находятся целочисленные значения.

Исправим ситуацию:

const Vector4& AutoParamDataSource::getShadowSceneDepthRange(size_t index) const
{
  static Vector4 dummy(0, 100000, 100000, 1.0f/100000);
  // ....
}

Теперь всё работает корректно.

V506 Pointer to local variable 'varyingName' is stored outside the scope of this variable. Such a pointer will become invalid. OgreGLES2RenderToVertexBuffer.cpp 268

typedef std::string String;

void GLES2RenderToVertexBuffer::bindVerticesOutput(Pass* pass)
{
  // ....

  const GLchar *names[64];
  for (unsigned short e = 0; e < elemCount; e++)
  {
    const VertexElement* element = declaration->getElement(e);
    String varyingName = getSemanticVaryingName(element->getSemantic(),
                                                element->getIndex());
    names[e] = varyingName.c_str(); // <=
  }

  // ....
}

В данном коде у нас есть массив из 64 указателей на тип const GLchar, куда в цикле сохраняются указатели на внутренние хранилища контейнеров типа String. Проблема заключается в том, что сами контейнеры типа String объявляются и инициализируются внутри самого цикла. После выхода из области видимости они уничтожаются вместе с внутренними хранилищами, что делает сохраненные в names указатели невалидными.

Эту ошибку можно поправить, выделив в куче память под новое хранилище, перекопировав туда строку из контейнера String и уже сохранив указатель на новое хранилище. Но не проще ли сделать массив не указателей, а типа String? Так и поступим:

void GLES2RenderToVertexBuffer::bindVerticesOutput(Pass* pass)
{
  // ....

  String names[64];
  for (unsigned short e = 0; e < elemCount; e++)
  {
    const VertexElement* element = declaration->getElement(e);
    names[e] = getSemanticVaryingName(element->getSemantic(),
                                      element->getIndex());
  }

  // ....
}

V614 Uninitialized variable 'lodLevel.reductionValue' used. main.cpp 806

Структура LodLevel:

struct _OgreLodExport LodLevel
{
  // ....
  VertexReductionMethod reductionMethod;
  Real reductionValue;
  // ....
};

И код, который использует эту структуру:

numLod = opts.numLods;
LodLevel lodLevel;            // <=
lodLevel.distance = 0.0;
for (unsigned short iLod = 0; iLod < numLod; ++iLod)
{

  lodLevel.reductionMethod = opts.usePercent
    ? LodLevel::VRM_PROPORTIONAL
    : LodLevel::VRM_CONSTANT;

  if (opts.usePercent)
  {
    lodLevel.reductionValue += opts.lodPercent * 0.01f;    // <=
  }
  else
  {
    lodLevel.reductionValue += (Ogre::Real)opts.lodFixed;  // <=
  }

  lodLevel.distance += opts.lodDist;
  lodConfig.levels.push_back(lodLevel);
}

В этом фрагменте кода объявляется структура LodLevel, которая не имеет определённого пользователем конструктора по умолчанию и инициализаторов нестатических полей классов. Соответственно, нестатическое поле не инициализируется, после чего происходит его чтение.

Если мы хотим, чтобы все поля инициализировались значениями по умолчанию, то можно воспользоваться одним из методов:

определить свой конструктор по умолчанию;
добавить инициализатор поля по умолчанию (начиная с C++11);
воспользоваться value initialization при объявлении экземпляра структуры (начиная с C++11).

Наиболее предпочтительным является третий метод, т.к. не меняет тривиальность типа, а это может быть важно:

LodLevel lodlevel {};

V595 The 'params' pointer was utilized before it was verified against nullptr. Check lines: 95, 101. OgreGpuProgramManager.cpp 95

Resource* GpuProgramManager::createImpl(...., 
                                        const NameValuePairList* params)
{
  auto langIt = params->find("language");
  auto typeIt = params->find("type");

  if (langIt == params->end())
    langIt = params->find("syntax");

  if (!params || langIt == params->end() || typeIt == params->end())
  {
    OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS,
      "You must supply 'language' or 'syntax' and 'type' parameters");
  }
}

В этом фрагменте кода переданный указатель params сначала разыменовывается, а только потом проверяется на валидность. Классическая ошибка. Код будет работать, пока в функцию кто-нибудь не толкнёт nullptr. Перенесём проверку:

Resource* GpuProgramManager::createImpl(....,
                                        const NameValuePairList* params)
{
  if (!params)
  {
    OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS,
      "Params can't be nullptr");
  }

  auto langIt = params->find("language");
  auto typeIt = params->find("type");

  if (langIt == params->end())
    langIt = params->find("syntax");

  if (langIt == params->end() || typeIt == params->end())
  {
    OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS,
      "You must supply 'language' or 'syntax' and 'type' parameters");
  }

  // ....
}

V547 Expression 'x == 0' is always true/false. OgreTerrain.cpp 3750

Terrain::NeighbourIndex Terrain::getNeighbourIndex(long x, long y)
{
  if (x < 0)
  {
    if (y < 0)
      return NEIGHBOUR_SOUTHWEST;
    else if (y > 0)
      return NEIGHBOUR_NORTHWEST;
    else
      return NEIGHBOUR_WEST;
  }
  else if (x > 0)
  {
    if (y < 0)
      return NEIGHBOUR_SOUTHEAST;
    else if (y > 0)
      return NEIGHBOUR_NORTHEAST;
    else
      return NEIGHBOUR_EAST;
  }

  if (y < 0)
  {
    if (x == 0)               // <= 
      return NEIGHBOUR_SOUTH;
  }
  else if (y > 0)
  {
    if (x == 0)               // <=
      return NEIGHBOUR_NORTH;
  }

  return NEIGHBOUR_NORTH;
}

Здесь переменная x проверяется на 0 после ложных проверок x > 0 и x < 0. Эта проверка бессмысленна, так как мы и так можем попасть в эту часть кода только при x == 0. Простая математика. Уберём лишние проверки и немного упростим код:

Terrain::NeighbourIndex Terrain::getNeighbourIndex(long x, long y)
{
  if (x < 0)
  {
    // ....
  }
  else if (x > 0)
  {
    // ....
  }
  else if (y < 0)
    return NEIGHBOUR_SOUTH;
  else if (y > 0)
    return NEIGHBOUR_NORTH;
  else
    return NEIGHBOUR_NORTH;
}

Теперь фрагмент выглядит значительно лучше и нет явно лишних проверок.

V609. Possible division or mod by zero. OgreInstanceBatchHW_VTF.cpp 392

Внимательно посмотрите на следующий код:

static const uint16 c_maxTexWidthHW = 4096;
const size_t numBones = 
  std::max<size_t>(1, baseSubMesh->blendIndexToBoneIndexMap.size());

// ....

const size_t maxUsableWidth = c_maxTexWidthHW –
                             (c_maxTexWidthHW % (numBones * mRowLength));

// ....

size_t texHeight = numWorldMatrices * mRowLength / maxUsableWidth; // <=

В строке объявления переменной maxUsableWidth её значение может быть от 0 до 4096. Следовательно, если значением maxUsableWidth внезапно окажется ноль, то мы получим деление на ноль в указанном месте. Бабах! А ведь на первый взгляд код написан без ошибок. Он даже скомпилируется и будет работать до тех пор, пока в переменную maxUsableWidth не проскочит ноль. Такое может быть, если результат произведения numBones * mRowLength будет больше 4096.

Переменная numBones инициализируется размером вектора blendIndexToBoneIndexMap. Вполне возможно, что разработчики вне класса контролируют количество элементов этого контейнера. Может быть, им просто везёт, и вектор недостаточно большой. Тем не менее может случиться, что вектор будет размером больше 4096. Тогда выстрелит деление на ноль и программа упадёт.

V557 Array overrun is possible. The 'j' index is pointing beyond array bound. OgreAnimationTrack.cpp 219

Классический выход за границы контейнера:

void AnimationTrack::_buildKeyFrameIndexMap(
  const std::vector<Real>& keyFrameTimes)
{

  // ....

  size_t i = 0, j = 0;
  while (j <= keyFrameTimes.size())                    // <=
  {
    mKeyFrameIndexMap[j] = static_cast<ushort>(i);
    while (i < mKeyFrames.size()
      && mKeyFrames[i]->getTime() <= keyFrameTimes[j]) // <=
      ++i;
    ++j;
  }
}

Индекс j, по которому мы получаем доступ к элементам контейнера keyFrameTimes, инкрементируется до значения, равного размеру контейнера.

Исправим ошибку:

while (j < keyFrameTimes.size())
{
  // ....
}

Статический анализатор нашёл несколько таких же ошибок и в других местах. Вот срабатывание на файле OgreSerializer.cpp, где в массиве 255 элементов, но мы пытаемся получить доступ к 256-му элементу:

String Serializer::readString(const DataStreamPtr& stream, size_t numChars)
{
  OgreAssert(numChars <= 255, "");
  char str[255];
  stream->read(str, numChars);
  str[numChars] = '\0';
  return str;
}

Здесь вообще очень странный код. Похоже на то, что его нигде не используют и забыли вычистить при рефакторинге, но вдруг всё же кто-то воспользуется функцией? Разберём ошибки в ней. В первую очередь в функции происходит выход за границу массива, потому что мы пытаемся присвоить значение '\0' несуществующему 256-му символу. Во-вторых, число прочитанных символов, возвращаемое функцией read, может быть меньше размера буфера str, в таком случае между символом '\0' и строкой, прочитанной функцией read, будет неинициализированная память. Такую функцию можно было бы переписать так:

String Serializer::readString(const DataStreamPtr& stream, 
                              size_t numChars)
{
  OgreAssert(numChars <= 255, "");
  String str(numChars, '\0');
  numChars = stream->read(&str[0], numChars);
  str.erase(numChars);
  return str;
}

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

V1048 The 'mVSOutPosition' variable was assigned the same value. OgreShaderExTriplanarTexturing.cpp 168

void TriplanarTexturing::copyFrom(....)
{
  const TriplanarTexturing& rhsTP =
    static_cast<const TriplanarTexturing&>(rhs);

  mPSOutDiffuse = rhsTP.mPSOutDiffuse;
  mPSInDiffuse = rhsTP.mPSInDiffuse;

  mVSInPosition = rhsTP.mVSInPosition;   // <=
  mVSOutPosition = rhsTP.mVSOutPosition; // <=

  mVSOutNormal = rhsTP.mVSOutNormal;
  mVSInNormal = rhsTP.mVSInNormal;
  mPSInNormal = rhsTP.mPSInNormal;

  mVSInPosition = rhsTP.mVSInPosition;   // <=
  mVSOutPosition = rhsTP.mVSOutPosition; // <=
}

Классическая опечатка после копипаста. Дважды присваивается одно и то же значение переменным-членам.

V560 Part of conditional expression is always true/false. OgreTerrainLodManager.cpp 62

void TerrainLodManager::open(const String& filename)
{
  if (!filename.empty() && filename.length() > 0)
       mDataStream = 
         Root::getSingleton()
              .openFileStream(filename, 
                              mTerrain->_getDerivedResourceGroup());
}

Здесь разработчик проверяет контейнер std::string на пустоту и на длину больше 0. Одну из частей условия можно убрать:

void TerrainLodManager::open(const String& filename)
{
  if (!filename.empty())
       mDataStream = 
         Root::getSingleton()
              .openFileStream(filename, 
                              mTerrain->_getDerivedResourceGroup());
}

Подозрительные места

Также я хочу поделиться некоторыми подозрительными местами, на которые ругается анализатор PVS-Studio, но трудно сказать, ошибка это или нет. Сразу отмечу, что анализатор сработал правильно, однако разработчики могли намеренно написать подобный код. Но лучше я просто покажу вам эти срабатывания.

V703 It is odd that the 'mProgramID' field in derived class 'GLGpuNvparseProgram' overwrites field in base class 'GLGpuProgram'. Check lines: OgreGLGpuNvparseProgram.h:63, OgreGLGpuProgram.h:60.

class _OgreGLExport GLGpuProgram : public GpuProgram, public GLGpuProgramBase
{
  // ....
protected:
  GLuint mProgramID; // <=
};

class _OgreGLExport GLGpuNvparseProgram : public GLGpuProgram
{
  // ....

  GLuint getProgramID(void) const
  {
    return mProgramID;            // <=
  } 

  // ....

private:
  GLuint mProgramID; // <=
};

Здесь класс-наследник объявляет переменную с таким же именем, как и protected-переменная в родительском классе. Это ведёт к скрытию и является прямым путём к ошибкам. При возврате mProgramID из функции getProgramID мы получим значение из класса-наследника, а не из базового класса. Неизвестно, задумывалось так или нет, но разработчикам лучше проверить это место.

Можно переименовать одно из полей или явно указать обращение к конкретному полю:

// Теперь обращение идёт к переменной базового класса
GLuint getProgramID(void) const
{ return GLGpuProgram::mProgramID; }

Первый способ, конечно, предпочтительнее и правильнее.

V547 Expression 'i != end' is always true. OgreScriptTranslator.cpp 787

bool ScriptTranslator::getMatrix4(
  AbstractNodeList::const_iterator i,
  AbstractNodeList::const_iterator end,
  Matrix4 *m)
{
  int n = 0;
  while (i != end && n < 16)
  {
    if (i != end)               // <=
    {
      Real r = 0;
      if (getReal(*i, &r))
        (*m)[n / 4][n % 4] = r;
      else
        return false;
    }
    else
    {
      return false;
    }
    ++i;
    ++n;
  }
  return true;
}

Очень странный код. Могу выделить в нём как минимум две проблемы:

Условие i != end проверяется дважды. Если условие в while будет true, то условие в if будет тоже всегда true. Проверка лишняя.
Ветка else недостижима. При этом возвращает false.

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

bool ScriptTranslator::getMatrix4(
  AbstractNodeList::const_iterator i,
  AbstractNodeList::const_iterator end,
  Matrix4 *m)
{
  int n = 0;
  while (i != end && n < 16)
  {
    Real r = 0;

    if (!getReal(*i, &r))
      return false;

    (*m)[n / 4][n % 4] = r;
    ++i;
    ++n;
  }
  return true;
}

V1053 Calling the 'destroyAllDeclarations' virtual function in the destructor may lead to unexpected result at runtime. OgreDefaultHardwareBufferManager.h 118

Объявление виртуальных функций класса:

class _OgreExport HardwareBufferManagerBase : public BufferAlloc
{
protected:
  // ....
  /// Internal method for destroys all vertex declarations.
  virtual void destroyAllDeclarations(void);

  /// Internal method for destroys all vertex buffer bindings.
  virtual void destroyAllBindings(void);
  // ....    
}

Объявление деструктора:

class _OgreExport DefaultHardwareBufferManager : public HardwareBufferManager
{

  // ....

  ~DefaultHardwareBufferManager()
  {
    // have to do this before mImpl is gone
    destroyAllDeclarations();
    destroyAllBindings();
  }

  // ....
}

Здесь мы вызываем в деструкторе две виртуальных функции. Пока что это ни на что не влияет. Однако если мы унаследуемся от этого класса и переопределим эти функции, то в деструкторе класса DefaultHardwareBufferManager будут по-прежнему использоваться виртуальные функции из базового класса. Это может привести к неожиданным результатам для разработчика. Использование виртуальных функций в деструкторах считается плохой практикой и опасным местом в коде. У нас есть отдельная статья, посвящённая этому случаю.

V530 The return value of function 'back' is required to be utilized. OgreGLXConfigDialog.cpp 410

class GLXConfigurator
{
public:
  // ....
  std::list<ConfigCallbackData> mConfigCallbackData
  // ....
}

void GLXConfigurator::SetRenderer(RenderSystem *r)

  // ....
  mConfigCallbackData.back();
  // ....
}

Здесь зачем-то мы вызываем функцию back контейнера std::list, чтобы получить ссылку на последний элемент, но никак её не используем и не сохраняем. Странное место. Возможно, что имелось в виду что-то другое.

V570 Variable is assigned to itself. OgreETCCodec.cpp 242

bool ETCCodec::decodePKM(const DataStreamPtr& stream,
                         DecodeResult& result) const
{
  // ....
  void *destPtr = output->getPtr();
  stream->read(destPtr, imgData->size);
  destPtr = static_cast<void*>(static_cast<uchar*>(destPtr)); // <=
  // ....
}

Указатель destPtr кастуется к другому типу указателя, потом к своему типу и присваивается самому себе. Очень странное место. Возможно, это старый код, который забыли убрать.

V1065 Expression can be simplified: check similar operands. OgrePage.cpp 117

bool Page::isHeld() const
{
  unsigned long nextFrame = Root::getSingleton().getNextFrameNumber();
  unsigned long dist;
  if (nextFrame < mFrameLastHeld)
  {
    // we must have wrapped around
    dist = mFrameLastHeld +
      (std::numeric_limits<unsigned long>::max() - mFrameLastHeld); // <=
  }
  else
    dist = nextFrame - mFrameLastHeld;

  // 5-frame tolerance
  return dist <= 5;
}

Тоже очень подозрительное место. Во-первых, выражение может быть упрощено: достаточно просто присвоить переменной dist значение из std::numeric_limits. Во-вторых, при истинном условии переменной dist присваивается значение, которое очевидно больше 5. Куда понятнее и лучше было бы сделать примерно так:

bool Page::isHeld() const
{
  unsigned long nextFrame = Root::getSingleton().getNextFrameNumber();

  if (nextFrame >= mFrameLastHeld)
  {
    // 5-frame tolerance
    return (nextFrame – mFrameLastHeld) <= 5;
  }

  return false;
}

Согласитесь, так код выглядит куда приятнее и понятнее.

Заключение

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

Также около четверти срабатываний анализатора PVS-Studio пришлось на диагностическое правило V730, о котором сложно что-то сказать однозначно. Мы не знаем подробности реализации, так может быть задумано или нет. Однозначно можно сказать лишь то, что при использовании анализатора PVS-Studio большинство перечисленных выше ошибок не попали бы в релиз и могли бы быть исправлены ещё на этапе написания кода.

#Cpp #GameDev