Обычные пользователи любят графические движки, потому что с ними удобно работать. Команда 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 большинство перечисленных выше ошибок не попали бы в релиз и могли бы быть исправлены ещё на этапе написания кода.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: ссылка.
