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

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

![0927_ogre_ru/image1.png](https://import.viva64.com/docx/blog/0927_ogre_ru/image1.png)

## Введение

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

[Ogre3D](https://www.ogre3d.org/) \(Object\-Oriented Graphics Rendering Engine\) является графическим движком с открытым исходным кодом, который можно найти на [GitHub](https://github.com/OGRECave/ogre)\. Проект написан на языке программирования С\+\+ и используется в играх и 3D визуализации\.

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

Всего при анализе Ogre3D PVS\-Studio выдал 562 срабатывания первого и второго уровней\. Были включены только предупреждения общего назначения \(GA\)\. Про фильтрацию срабатываний можно узнать в нашей [документации](https://pvs-studio.ru/ru/docs/manual/6532/)\. Это не мало, но и не много, учитывая, что большинство срабатываний пришлось на [V730](https://pvs-studio.ru/ru/docs/warnings/v730/)\. Такое диагностическое правило говорит о том, что не все члены класса инициализируются в конструкторе\. Сложно определить, так и задумывалось или нет, для этого необходимо знать тонкости реализации проекта\.

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

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

[V1064](https://pvs-studio.ru/ru/docs/warnings/v1064/) The '1' operand of integer division is less than the '100000' one\. The result will always be zero\. OgreAutoParamDataSource\.cpp 1094

```cpp
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_ будет равен не дроби, а нулю, поскольку слева и справа от оператора деления находятся целочисленные значения\.

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

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

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

[V506](https://pvs-studio.ru/ru/docs/warnings/v506/) Pointer to local variable 'varyingName' is stored outside the scope of this variable\. Such a pointer will become invalid\. OgreGLES2RenderToVertexBuffer\.cpp 268

```cpp
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_? Так и поступим:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized variable 'lodLevel\.reductionValue' used\. main\.cpp 806

Структура _LodLevel_:

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

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

```cpp
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](https://en.cppreference.com/w/cpp/language/value_initialization) при объявлении экземпляра структуры \(начиная с C\+\+11\)\.

Наиболее предпочтительным является третий метод, т\.к\. не меняет [тривиальность](https://en.cppreference.com/w/cpp/named_req/TrivialType) типа, а это может быть важно:

```cpp
LodLevel lodlevel {};
```

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'params' pointer was utilized before it was verified against nullptr\. Check lines: 95, 101\. OgreGpuProgramManager\.cpp 95

```cpp
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_\. Перенесём проверку:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'x \=\= 0' is always true/false\. OgreTerrain\.cpp 3750

```cpp
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\._ Простая математика\. Уберём лишние проверки и немного упростим код:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v609/)\. Possible division or mod by zero\. OgreInstanceBatchHW\_VTF\.cpp 392

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The 'j' index is pointing beyond array bound\. OgreAnimationTrack\.cpp 219

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

```cpp
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_, инкрементируется до значения, равного размеру контейнера\.

![0927_ogre_ru/image2.png](https://import.viva64.com/docx/blog/0927_ogre_ru/image2.png)

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

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

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

```cpp
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_, будет неинициализированная память\. Такую функцию можно было бы переписать так:

```cpp
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_](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1072r10.html)\.

[V1048](https://pvs-studio.ru/ru/docs/warnings/v1048/) The 'mVSOutPosition' variable was assigned the same value\. OgreShaderExTriplanarTexturing\.cpp 168

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v560/) Part of conditional expression is always true/false\. OgreTerrainLodManager\.cpp 62

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

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

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

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

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

[V703](https://pvs-studio.ru/ru/docs/warnings/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\.

```cpp
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_ мы получим значение из класса\-наследника, а не из базового класса\. Неизвестно, задумывалось так или нет, но разработчикам лучше проверить это место\.

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

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

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

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'i \!\= end' is always true\. OgreScriptTranslator\.cpp 787

```cpp
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;
}
```

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

1. Условие _i \!\= end_ проверяется дважды\. Если условие в _while_ будет _true_, то условие в _if_ будет тоже всегда _true_\. Проверка лишняя\.
1. Ветка _else_ недостижима\. При этом возвращает _false_\.

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v1053/) Calling the 'destroyAllDeclarations' virtual function in the destructor may lead to unexpected result at runtime\. OgreDefaultHardwareBufferManager\.h 118

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

```cpp
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);
  // ....    
}
```

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

```cpp
class _OgreExport DefaultHardwareBufferManager : public HardwareBufferManager
{

  // ....

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

  // ....
}
```

Здесь мы вызываем в деструкторе две виртуальных функции\. Пока что это ни на что не влияет\. Однако если мы унаследуемся от этого класса и переопределим эти функции, то в деструкторе класса _DefaultHardwareBufferManager_ будут по\-прежнему использоваться виртуальные функции из базового класса\. Это может привести к неожиданным результатам для разработчика\. Использование виртуальных функций в деструкторах считается плохой практикой и опасным местом в коде\. У нас есть [отдельная статья](https://pvs-studio.ru/ru/blog/posts/cpp/0891/), посвящённая этому случаю\.

[V530](https://pvs-studio.ru/ru/docs/warnings/v530/) The return value of function 'back' is required to be utilized\. OgreGLXConfigDialog\.cpp 410

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

void GLXConfigurator::SetRenderer(RenderSystem *r)

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

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

[V570](https://pvs-studio.ru/ru/docs/warnings/v570/) Variable is assigned to itself\. OgreETCCodec\.cpp 242

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v1065/) Expression can be simplified: check similar operands\. OgrePage\.cpp 117

```cpp
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\. Куда понятнее и лучше было бы сделать примерно так:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v730/), о котором сложно что\-то сказать однозначно\. Мы не знаем подробности реализации, так может быть задумано или нет\. Однозначно можно сказать лишь то, что при использовании анализатора [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) большинство перечисленных выше ошибок не попали бы в релиз и могли бы быть исправлены ещё на этапе написания кода\.