Некоторое время назад где-то в Интернете я наткнулся на физический движок под названием Newton Game Dynamics. Зная, что проекты движка обычно большие и сложные, я решил проверить его код с помощью PVS-Studio на наличие интересных дефектов. Я был особенно воодушевлен этим, потому что мой коллега Андрей Карпов уже проверял его в 2014 году, и повторная проверка была бы хорошей возможностью продемонстрировать эволюцию нашего анализатора за последние шесть лет. На момент написания этой статьи последняя версия Newton Game Dynamics датирована 27 февраля 2020 года, а это значит, что она также активно развивалась последние шесть лет. Так что, надеюсь, эта статья будет интересна не только нам, но и разработчикам движка — для них это шанс исправить некоторые ошибки и улучшить свой код.
Аналитический отчет
В 2014 году PVS-Studio выпустила:
- 48 предупреждений первого уровня;
- 79 предупреждений второго уровня;
- 261 предупреждение третьего уровня.
В 2020 году выпущено:
- 124 предупреждения первого уровня;
- 272 предупреждения второго уровня;
- 787 предупреждений третьего уровня (некоторые из них тоже весьма интересны).
На этот раз интересных предупреждений гораздо больше, чем в статье Андрея, так что давайте их проверим.
Диагностические сообщения
Предупреждение 1
V519 Переменной ‘tmp[i][2]’ два раза подряд присваиваются значения. Возможно, это ошибка. Строки проверки: 468, 469. dgCollisionConvexHull.cpp 469
bool dgCollisionConvexHull::Create (dgInt32 count,....)
{
....
dgStack<dgVector> tmp(3 * count);
for (dgInt32 i = 0; i < count; i ++)
{
tmp[i][0] = dgFloat32 (buffer[i*3 + 0]);
tmp[i][1] = dgFloat32 (buffer[i*3 + 1]);
tmp[i][2] = dgFloat32 (buffer[i*3 + 2]);
tmp[i][2] = dgFloat32 (0.0f);
}
....
}Элемент массива tmp[i][2] инициализируется дважды подряд. Подобные дефекты обычно являются признаком неправильного использования копипасты. Это можно исправить, либо удалив вторую инициализацию, если ее там не должно быть, либо изменив номер индекса на 3 — все зависит от значения переменной count. Теперь я хочу показать вам еще одно предупреждение V519, отсутствующее в статье Андрея, но записанное в нашей базе ошибок:
V519 Дважды подряд объекту «влажный» присваиваются значения. Возможно, это ошибка. физика dgbody.cpp 404
void dgBody::AddBuoyancyForce (....)
{
....
damp = (m_omega % m_omega) * dgFloat32 (10.0f) *
fluidAngularViscousity;
damp = GetMax (GetMin ((m_omega % m_omega) *
dgFloat32 (1000.0f) *
fluidAngularViscousity, dgFloat32(0.25f)),
dgFloat32(2.0f));
....
}На самом деле, эта ошибка не отображалась в отчете. Я также не нашел функцию AddBuoyancyForce в файле dgbody.cpp. И это прекрасно: если способность выявлять новые ошибки — признак эволюции нашего анализатора, то отсутствие более ранних ошибок в последних версиях проекта — признак эволюции самого проекта.
Небольшое предположение не по теме
Я не тот, кто может судить, содержат ли приведенные ниже фрагменты ошибок или их поведение не соответствует ожиданиям программиста, но они выглядят подозрительно.
Этот фрагмент вызвал сразу два предупреждения:
V621 Рассмотрите возможность проверки оператора for. Возможно, что цикл будет выполняться некорректно или вообще не будет выполняться. MultiBodyCar.cpp 942
V654 Условие цикла ‘i ‹ count’ всегда ложно. MultiBodyCar.cpp 942
void MultibodyBodyCar(DemoEntityManager* const scene)
{
....
int count = 10;
count = 0;
for (int i = 0; i < count; i++)
{
for (int j = 0; j < count; j++)
{
dMatrix offset(location);
offset.m_posit += dVector (j * 5.0f + 4.0f, 0.0f, i * 5.0f, 0.0f);
//manager->CreateSportCar(offset, viperModel.GetData());
manager->CreateOffRoadCar(offset, monsterTruck.GetData());
}
}
....
}Этот код можно использовать для целей отладки — если это так, отключение цикла является обычным приемом. Таких случаев было еще несколько:
V519 Переменной «ret» дважды подряд присваиваются значения. Возможно это ошибка. Проверьте строки: 325, 326. dString.cpp 326
void dString::LoadFile (FILE* const file)
{
....
size_t ret = fread(m_string, 1, size, file);
ret = 0;
....
}V519 Переменной «ret» дважды подряд присваиваются значения. Возможно, это ошибка. Строки проверки: 1222, 1223. DemoEntityManager.cpp 1223
void DemoEntityManager::DeserializeFile (....)
{
....
size_t ret = fread(buffer, size, 1, (FILE*) serializeHandle);
ret = 0;
....
}V560 Часть условного выражения всегда истинна: (количество ‹ 10). dMathDefines.h 726
bool dCholeskyWithRegularizer(....)
{
....
int count = 0;
while (!pass && (count < 10))
{
....
}
....
}V654 Условие цикла ‘ptr != edge’ всегда ложно. dgPolyhedra.cpp 1571
void dgPolyhedra::Triangulate (....)
{
....
ptr = edge;
....
while (ptr != edge);
....
}V763 Параметр count всегда перезаписывается в теле функции перед использованием. ConvexCast.cpp 31
StupidComplexOfConvexShapes (...., int count)
{
count = 40;
//count = 1;
....
}V547 Выражение «axisCount» всегда ложно. MultiBodyCar.cpp 650
void UpdateDriverInput(dVehicle* const vehicle, dFloat timestep)
{
....
int axisCount = scene->GetJoystickAxis(axis);
axisCount = 0;
if (axisCount)
{
....
}
....
}Многие из вас могут возразить, что такие изменения в общедоступном коде должны быть, по крайней мере, прокомментированы. Что ж, я с вами в этом вопросе. Я считаю, что некоторые функции, которые хороши для домашнего проекта, не должны быть разрешены в проекте, предназначенном для использования многими людьми. Но выбор все же за авторами.
Предупреждение 2
V769 Указатель «результат» в выражении «результат + i» равен nullptr. Полученное значение бессмысленно и не должно использоваться. win32_monitor.c 286
GLFWvidmode* _glfwPlatformGetVideoModes(_GLFWmonitor* monitor, int* count)
{
GLFWvidmode* result = NULL;
....
for (i = 0; i < *count; i++)
{
if (_glfwCompareVideoModes(result + i, &mode) == 0)
break;
}
}Проблема здесь в том, что result не меняется после инициализации. Результирующий указатель бессмыслен; вы не можете его использовать.
Предупреждения 3, 4, 5
V778 Найдено два похожих фрагмента кода. Возможно, это опечатка и вместо m_binormalChannel нужно использовать переменную m_colorChannel. dgMeshEffect1.cpp 1887
void dgMeshEffect::EndBuildFace ()
{
....
if (m_attrib.m_binormalChannel.m_count) <=
{
attibutes.m_binormalChannel.
PushBack(m_attrib.m_binormalChannel[m_constructionIndex + i]);
}
if (m_attrib.m_binormalChannel.m_count) <=
{
attibutes.m_colorChannel.
PushBack(m_attrib.m_colorChannel[m_constructionIndex + i]);
}
}Второе условие, похоже, является клоном первого и должно было выглядеть так:
if (m_attrib.m_colorChannel.m_count) <=
{
attibutes.m_colorChannel.
PushBack(m_attrib.m_colorChannel[m_constructionIndex + i]);
}Вот еще очень похожий баг:
V524 Странно, что тело функции «EnabledAxis1» полностью эквивалентно телу функции «EnabledAxis0». dCustomDoubleHingeActuator.cpp 88
void dCustomDoubleHingeActuator::EnabledAxis0(bool state)
{
m_axis0Enable = state; <=
}
void dCustomDoubleHingeActuator::EnabledAxis1(bool state)
{
m_axis0Enable = state; <=
}Это должно быть исправлено следующим образом:
void dCustomDoubleHingeActuator::EnabledAxis1(bool state)
{
m_axis1Enable = state;
}Еще одна ошибка копирования-вставки:
V525 Код содержит набор похожих блоков. Проверьте элементы «m_x», «m_y», «m_y» в строках 73, 74, 75. dWoodFracture.cpp 73
WoodVoronoidEffect(....)
{
....
for (int i = 0; i < count; i ++)
{
dFloat x = dGaussianRandom(size.m_x * 0.1f);
dFloat y = dGaussianRandom(size.m_y * 0.1f); <=
dFloat z = dGaussianRandom(size.m_y * 0.1f); <=
....
}
....
}Я предполагаю, что переменная z должна быть инициализирована следующим образом:
dFloat z = dGaussianRandom(size.m_z * 0.1f);Предупреждения 6, 7
Как и любой другой крупный проект на C или C++, Newton Game Dynamics не удалось избежать ошибок, связанных с небезопасной обработкой указателей. Их обычно трудно найти и отладить, и они вызывают сбои программ, то есть они очень опасны и непредсказуемы. К счастью, многие из них легко обнаруживаются нашим анализатором. Кажется не очень оригинальной идеей, что написать проверку указателя и двигаться дальше с легким сердцем гораздо лучше, чем тратить время на воспроизведение ошибки, отслеживание проблемного места и его отладку, не так ли? Во всяком случае, вот некоторые из предупреждений этого типа:
V522 Возможно, произошло разыменование потенциального нулевого указателя «лицо». dgContactSolver.cpp 351
DG_INLINE dgMinkFace* dgContactSolver::AddFace(dgInt32 v0,dgInt32 v1,
dgInt32 v2)
{
dgMinkFace* const face = NewFace();
face->m_mark = 0;
....
}Реализация функции NewFace невелика, поэтому приведу ее полностью:
DG_INLINE dgMinkFace* dgContactSolver::NewFace()
{
dgMinkFace* face = (dgMinkFace*)m_freeFace;
if (m_freeFace)
{
m_freeFace = m_freeFace->m_next;
} else
{
face = &m_facePool[m_faceIndex];
m_faceIndex++;
if (m_faceIndex >= DG_CONVEX_MINK_MAX_FACES)
{
return NULL;
}
}
#ifdef _DEBUG
memset(face, 0, sizeof (dgMinkFace));
#endif
return face;
}В одной из своих точек выхода функция NewFace возвращает NULL, что, в свою очередь, приведет к разыменованию нулевого указателя с неопределенным поведением в результате.
Вот похожий случай разыменования нулевого указателя, но более опасный:
V522 Возможно, произошло разыменование потенциального нулевого указателя «периметр». dgPolyhedra.cpp 2541
bool dgPolyhedra::PolygonizeFace(....)
{
....
dgEdge* const perimeter = flatFace.AddHalfEdge
(edge1->m_next->m_incidentVertex,
edge1->m_incidentVertex);
perimeter->m_twin = edge1;
....
}Вот реализация AddHalfEdge:
dgEdge* dgPolyhedra::AddHalfEdge (dgInt32 v0, dgInt32 v1)
{
if (v0 != v1)
{
dgPairKey pairKey (v0, v1);
dgEdge tmpEdge (v0, -1);
dgTreeNode* node = Insert (tmpEdge, pairKey.GetVal());
return node ? &node->GetInfo() : NULL;
} else
{
return NULL;
}
}На этот раз NULL возвращается в двух точках выхода из трех.
Всего анализатор выдал 48 предупреждений V522. Они похожи по большей части, поэтому я не вижу смысла обсуждать здесь больше.
Предупреждение 8
V668 Проверять указатель pBits на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. TargaToOpenGl.cpp 166
char* const pBits = new char [width * height * 4];
if(pBits == NULL)
{
fclose(pFile);
return 0;
}Значение указателя, возвращаемое оператором new, сравнивается с нулем. Обычно это означает, что вы получите неожиданное поведение в случае сбоя выделения памяти. Когда оператору new не удается выделить необходимое хранилище, должно быть выдано исключение std::bad_alloc(), как предписано стандартом C++. . В данном конкретном случае это означает, что условие никогда не будет выполнено, что явно отличается от поведения, на которое рассчитывал программист. Они хотели, чтобы программа закрывала файл в случае сбоя выделения памяти. Но программа этого не сделает и вместо этого получит утечку ресурсов.
Предупреждения 9, 10, 11
- V764 Возможно, в функцию «CreateWheel» передается неправильный порядок аргументов: «высота» и «радиус». StandardJoints.cpp 791
- V764 Возможно, в функцию «CreateWheel» передается неправильный порядок аргументов: «высота» и «радиус». StandardJoints.cpp 833
- V764 Возможно, в функцию «CreateWheel» передается неправильный порядок аргументов: «высота» и «радиус». StandardJoints.cpp 884
Это вызовы функции:
NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);А это его декларация:
static NewtonBody* CreateWheel (DemoEntityManager* const scene,
const dVector& location, dFloat radius, dFloat height)Эта диагностика обнаруживает вызовы функций с предположительно переставленными аргументами.
Предупреждения 12, 13
Анализатор выдал предупреждения о двух похожих методах с разными именами:
V621 Рассмотрите возможность проверки оператора for. Возможно, что цикл будет выполняться некорректно или вообще не будет выполняться. dgCollisionUserMesh.cpp 161
V621 Рассмотрите возможность проверки оператора for. Возможно, что цикл будет выполняться некорректно или вообще не будет выполняться. dgCollisionUserMesh.cpp 236
void dgCollisionUserMesh::GetCollidingFacesContinue
(dgPolygonMeshDesc* const data) const
{
....
data->m_faceCount = 0; <=
data->m_userData = m_userData;
data->m_separationDistance = dgFloat32(0.0f);
m_collideCallback(&data->m_p0, NULL);
dgInt32 faceCount0 = 0;
dgInt32 faceIndexCount0 = 0;
dgInt32 faceIndexCount1 = 0;
dgInt32 stride = data->m_vertexStrideInBytes / sizeof(dgFloat32);
dgFloat32* const vertex = data->m_vertex;
dgInt32* const address = data->m_meshData.m_globalFaceIndexStart;
dgFloat32* const hitDistance = data->m_meshData.m_globalHitDistance;
const dgInt32* const srcIndices = data->m_faceVertexIndex;
dgInt32* const dstIndices = data->m_globalFaceVertexIndex;
dgInt32* const faceIndexCountArray = data->m_faceIndexCount;
for (dgInt32 i = 0; (i < data->m_faceCount)&&
(faceIndexCount0 < (DG_MAX_COLLIDING_INDICES - 32));
i++)
{
....
}
....
}
void dgCollisionUserMesh::GetCollidingFacesDescrete
(dgPolygonMeshDesc* const data) const
{
....
data->m_faceCount = 0; <=
data->m_userData = m_userData;
data->m_separationDistance = dgFloat32(0.0f);
m_collideCallback(&data->m_p0, NULL);
dgInt32 faceCount0 = 0;
dgInt32 faceIndexCount0 = 0;
dgInt32 faceIndexCount1 = 0;
dgInt32 stride = data->m_vertexStrideInBytes / sizeof(dgFloat32);
dgFloat32* const vertex = data->m_vertex;
dgInt32* const address = data->m_meshData.m_globalFaceIndexStart;
dgFloat32* const hitDistance = data->m_meshData.m_globalHitDistance;
const dgInt32* const srcIndices = data->m_faceVertexIndex;
dgInt32* const dstIndices = data->m_globalFaceVertexIndex;
dgInt32* const faceIndexCountArray = data->m_faceIndexCount;
for (dgInt32 i = 0; (i < data->m_faceCount)&&
(faceIndexCount0 < (DG_MAX_COLLIDING_INDICES - 32));
i++)
{
....
}
....
}Проблемным местом является i ‹ data-›m_faceCountчасть условия.Поскольку data-›m_faceCountприсваивается значение 0, этот цикл не будет выполняться ни разу. Я предполагаю, что программист забыл повторно инициализировать поле m_faceCount и просто клонировал тело метода.
Предупреждения 14, 15
Анализатор выдал два предупреждения на двух одинаковых соседних строках:
V630 Функция «_alloca» используется для выделения памяти для массива объектов, которые являются классами, содержащими конструкторы. dgSkeletonContainer.cpp 1341
V630 Функция «_alloca» используется для выделения памяти для массива объектов, которые являются классами, содержащими конструкторы. dgSkeletonContainer.cpp 1342
#define alloca _alloca
....
#define dAlloca(type,size) (type*) alloca ((size) * sizeof (type))
....
dgSpatialMatrix::dgSpatialMatrix();
dgSpatialMatrix::dgSpatialMatrix(dgFloat32 val);
....
dgSpatialMatrix* const bodyMassArray = dgAlloca(dgSpatialMatrix,
m_nodeCount);
dgSpatialMatrix* const jointMassArray = dgAlloca(dgSpatialMatrix,
m_nodeCount);Проблема с этим кодом заключается в том, что выделенный блок памяти обрабатывается так, как если бы он был массивом объектов, имеющих конструктор или деструктор. Но когда память распределяется так, как здесь, конструктор вызываться не будет. Деструктор также не будет вызываться при освобождении памяти. Этот код очень подозрительный. Программа может в конечном итоге обрабатывать неинициализированные переменные и столкнуться с другими проблемами. Еще одна проблема с этим подходом заключается в том, что, в отличие от метода malloc/free, вы не получите явного сообщения об ошибке, если попытаетесь выделить больше памяти, чем может предоставить компьютер. Вместо этого вы получите ошибку сегментации при попытке доступа к этой памяти. Еще несколько сообщений такого типа:
- V630 Функция «_alloca» используется для выделения памяти для массива объектов, которые являются классами, содержащими конструкторы. dVehicleSolver.cpp 498
- V630 Функция «_alloca» используется для выделения памяти для массива объектов, которые являются классами, содержащими конструкторы. dVehicleSolver.cpp 499
- V630 Функция «_alloca» используется для выделения памяти для массива объектов, которые являются классами, содержащими конструкторы. dVehicleSolver.cpp 1144
- Еще около 10 таких предупреждений.
Вывод
Как обычно, PVS-Studio не подвела и нашла несколько интересных багов. А это значит, что у него все отлично и он помогает сделать мир лучше. Если вы хотите попробовать PVS-Studio на собственном проекте, вы можете получить его здесь.
