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