Проверка фреймворка Ogre3D статическим анализатором 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; } 

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

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

2. Ветка 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 большинство перечисленных выше ошибок не попали бы в релиз и могли бы быть исправлены ещё на этапе написания кода.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: ссылка.