Хабрахабр

PVS-Studio заглянул в движок Red Dead Redemption — Bullet

Picture 4

В наши дни для, например, разработки игр уже нет нужды самостоятельно с нуля реализовывать физику объектов, так как для этого существует большое число библиотек. Bullet в свое время активно использовался во многих ААА играх, проектах виртуальной реальности, различных симуляциях и машинном обучении. Да и используется до сих пор, являясь, например, одним из движков Red Dead Redemption и Red Dead Redemption 2. Так что почему бы не проверить Bullet с помощью PVS-Studio, чтобы посмотреть, какие ошибки сможет выявить статический анализ в таком масштабном проекте, связанном с симуляцией физики.
Эта библиотека свободно распространяется, так что все могут при желании использовать её и в своих проектах. Кроме Red Dead Redemption этот физический движок также используется и в киноиндустрии для создания спецэффектов. Например, он был задействован при съемках «Шерлока Холмса» Гая Ричи для расчета столкновений.

PVS-Studio — это статический анализатор кода, который помогает находить ошибки, недочеты и потенциальные уязвимости в исходном коде программ на С, C++, C#, Java. Если вы впервые встречаетесь со статьей, где PVS-Studio проверяет проекты, то сделаю небольшое отступление. Статический анализ является своего рода автоматизированным процессом обзора кода.

Для разогрева

Пример 1:

Начнем с забавной ошибки:

141592538' constant. V624 There is probably a misprint in '3. PhysicsClientC_API.cpp 4109 Consider using the M_PI constant from <math.h>.

B3_SHARED_API void b3ComputeProjectionMatrixFOV(float fov, ....)
{ float yScale = 1.0 / tan((3.141592538 / 180.0) * fov / 2); ....
}

Небольшая опечатка в числе Пи (3,141592653...), пропущено число «6» на 7-ой позиции в дробной части.

Picture 1

Возможно, ошибка в десятимиллионной доле после запятой и не приведет к ощутимым последствиям, но все-таки стоит пользоваться уже существующими библиотечными константами без опечаток. Для числа Пи есть константа M_PI из заголовка math.h.

Копипаста

Пример 2:

Так, например, здесь в функцию передаются три родственных аргумента halfExtentsX, halfExtentsY, halfExtentsZ, но последний нигде в функции не используется. Иногда анализатор позволяет найти ошибку косвенным путем. Так что возможно, это ошибка копипасты и здесь и должен использоваться забытый аргумент. Можно заметить, что при вызове метода addVertex дважды используется переменная halfExtentsY.

TinyRenderer.cpp 375 V751 Parameter 'halfExtentsZ' is not used inside function body.

void TinyRenderObjectData::createCube(float halfExtentsX, float halfExtentsY, float halfExtentsZ, ....)
{ .... m_model->addVertex(halfExtentsX * cube_vertices_textured[i * 9], halfExtentsY * cube_vertices_textured[i * 9 + 1], halfExtentsY * cube_vertices_textured[i * 9 + 2], cube_vertices_textured[i * 9 + 4], ....); ....
}

Пример 3:

Также анализатор обнаружил следующий интересный фрагмент, приведу его сначала в исходном виде.

Picture 11

Видите эту длииииинную строчку?

Picture 12

Крайне странно, что программист решил записать такое длинное условие в одну строку. А вот то, что в неё скорее всего закралась ошибка, совсем не удивительно.

Анализатор выдал на эту строку следующие предупреждения.

Column1(). V501 There are identical sub-expressions 'rotmat. 0001' to the left and to the right of the '&&' operator. Norm() < 1. LinearR4.cpp 351

9999 < rotmat. V501 There are identical sub-expressions '0. Norm()' to the left and to the right of the '&&' operator. Column1(). LinearR4.cpp 351

Из последних двух сравнений видно, что есть Column1 и Column2. Если мы выпишем все в наглядном «табличном» виде, то станет видно, что к Column1 применяются одни и те же проверки. Скорее всего третье и четвёртое сравнение должны были проверять значение именно Column2.

Column1().Norm() < 1.0001 && 0.9999 < Column1().Norm()
&& Column1().Norm() < 1.0001 && 0.9999 < Column1().Norm()
&&(Column1() ^ Column2()) < 0.001 && (Column1() ^ Column2()) > -0.001

В таком виде одинаковые сравнения становятся куда более заметными.

Пример 4:

Ошибка аналогичного вида:

b3CpuRigidBodyPipeline.cpp 169 V501 There are identical sub-expressions 'cs.m_fJacCoeffInv[0] == 0' to the left and to the right of the '&&' operator.

float m_fJacCoeffInv[2]; static inline void b3SolveFriction(b3ContactConstraint4& cs, ....)
....
}

В этом случае дважды проверяется один и тот же элемент массива. Скорее всего условие должно было выглядеть так: cs.m_fJacCoeffInv[0] == 0 && cs.m_fJacCoeffInv[1] == 0. Это классический пример ошибки копипасты.

Пример 5:

Еще был обнаружен такой недочет:

There is a probability of logical error presence. V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. main.cpp 79 Check lines: 79, 112.

int main(int argc, char* argv[])
{ .... while (serviceResult > 0) { serviceResult = enet_host_service(client, &event, 0); if (serviceResult > 0) { .... } else if (serviceResult > 0) { puts("Error with servicing the client"); exit(EXIT_FAILURE); } .... } ....
}

Функция enet_host_service, результат которой присваивается serviceResult, возвращает единицу в случае удачного завершения и -1 в случае неудачи. Скорее всего ветка else if как раз и должна была среагировать на отрицательное значение serviceResult, но условие проверки было продублировано. Скорее всего это также ошибка копипасты.

Аналогичное предупреждение анализатора, которое нет смысла описывать в статье:

There is a probability of logical error presence. V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. PhysicsClientUDP.cpp 151 Check lines: 151, 190.

За пределами дозволенного: выход за границы массива

Пример 6:

Зачастую эта ошибка происходит из-за сложного индексирования в цикле. Одной из неприятных для поиска ошибок является выход за пределы массива.

Но m_desiredState также содержит только 128 элементов. Здесь в условии цикла переменная dofIndex ограничивается сверху значением 128, а dof значением 4 включительно. В результате индекс [dofIndex+dof] может привести к выходу за границы массива.

The value of 'dofIndex + dof' index could reach 130. V557 Array overrun is possible. PhysicsClientC_API.cpp 968

#define MAX_DEGREE_OF_FREEDOM 128 double m_desiredState[MAX_DEGREE_OF_FREEDOM]; B3_SHARED_API int b3JointControl(int dofIndex, double* forces, int dofCount, ....)
{ .... if ( (dofIndex >= 0) && (dofIndex < MAX_DEGREE_OF_FREEDOM ) && dofCount >= 0 && dofCount <= 4) { for (int dof = 0; dof < dofCount; dof++) { command->m_sendState.m_desiredState[dofIndex+dof] = forces[dof]; .... } } ....
}

Пример 7:

Если имя файла будет максимально длинным, то терминальный ноль будет записан за границей массива (Off-by-one Error). Схожая ошибка, но теперь к ней приводит суммирование не при индексировании массива, а в условии. Естественно, переменная len лишь в исключительных случаях будет равна MAX_FILENAME_LENGTH, но это не устраняет ошибку, а просто делает её проявление редким.

The value of 'len' index could reach 1024. V557 Array overrun is possible. PhysicsClientC_API.cpp 5223

#define MAX_FILENAME_LENGTH MAX_URDF_FILENAME_LENGTH 1024
struct b3Profile
{ char m_name[MAX_FILENAME_LENGTH]; int m_durationInMicroSeconds;
}; int len = strlen(name);
if (len >= 0 && len < (MAX_FILENAME_LENGTH + 1))
{ command->m_type = CMD_PROFILE_TIMING; strcpy(command->m_profile.m_name, name); command->m_profile.m_name[len] = 0;
}

Один раз отмерь — семь раз отрежь

Пример 8:

Анализатор нашел более 100 мест в коде, где можно внести такую поправку. В случаях, когда нужно много раз использовать результат работы какой-либо функции или многократно использовать переменную, доступ к которой нужно получить, пройдя по целой цепочке вызовов, для оптимизации и лучшей читаемости кода следует использовать временные переменные.

Consider creating a pointer to avoid using the 'm_app->m_renderer->getActiveCamera()' expression repeatedly. V807 Decreased performance. InverseKinematicsExample.cpp 315

virtual void resetCamera()
{ .... if (....) { m_app->m_renderer->getActiveCamera()->setCameraDistance(dist); m_app->m_renderer->getActiveCamera()->setCameraPitch(pitch); m_app->m_renderer->getActiveCamera()->setCameraYaw(yaw); m_app->m_renderer->getActiveCamera()->setCameraPosition(....); }
}

Здесь многократно используется одна и та же цепочка вызовов, которую можно заменить одним указателем.

Пример 9:

The 'btCos(euler_out.pitch)' function was called several times with identical arguments. V810 Decreased performance. btMatrix3x3.h 576 The result should possibly be saved to a temporary variable, which then could be used while calling the 'btAtan2' function.

The 'btCos(euler_out2.pitch)' function was called several times with identical arguments. V810 Decreased performance. btMatrix3x3.h 578 The result should possibly be saved to a temporary variable, which then could be used while calling the 'btAtan2' function.

void getEulerZYX(....) const
{ .... if (....) { .... } else { .... euler_out.roll = btAtan2(m_el[2].y() / btCos(euler_out.pitch), m_el[2].z() / btCos(euler_out.pitch)); euler_out2.roll = btAtan2(m_el[2].y() / btCos(euler_out2.pitch), m_el[2].z() / btCos(euler_out2.pitch)); euler_out.yaw = btAtan2(m_el[1].x() / btCos(euler_out.pitch), m_el[0].x() / btCos(euler_out.pitch)); euler_out2.yaw = btAtan2(m_el[1].x() / btCos(euler_out2.pitch), m_el[0].x() / btCos(euler_out2.pitch)); } ....
}

В этом случае можно создать две переменные и сохранить в них значения, возвращаемые функцией btCos для euler_out.pitch и euler_out2.pitch, вместо того, чтобы вызывать функцию по четыре раза для каждого аргумента.

Утечка

Пример 10:

В проекте было обнаружено множество ошибок следующего вида:

A memory leak is possible. V773 Visibility scope of the 'importer' pointer was exited without releasing the memory. SerializeSetup.cpp 94

void SerializeSetup::initPhysics()
{ .... btBulletWorldImporter* importer = new btBulletWorldImporter(m_dynamicsWorld); .... fclose(file); m_guiHelper->autogenerateGraphicsObjects(m_dynamicsWorld);
}

Здесь не было произведено освобождение памяти от указателя importer. Это может привести к утечке памяти. А для физического движка это может быть плохой тенденцией. Чтобы избежать утечки, достаточно после того, как переменная станет не нужна, добавить delete importer. Но, конечно, лучше использовать умные указатели.

Тут свои законы

Пример 11:

Заметите сами, где в небольшом отрывке кода содержится ошибка? Следующая ошибка появляется в коде из-за того, что правила С++ не всегда совпадают с математическими правилами или «здравым смыслом».

btAlignedObjectArray<btFractureBody*> m_fractureBodies; void btFractureDynamicsWorld::fractureCallback()
{ for (int i = 0; i < numManifolds; i++) { .... int f0 = m_fractureBodies.findLinearSearch(....); int f1 = m_fractureBodies.findLinearSearch(....); if (f0 == f1 == m_fractureBodies.size()) continue; .... }
....
}

Анализатор выдает следующее предупреждение:

Remember that 'a == b == c' is not equal to 'a == b && b == c'. V709 Suspicious comparison found: 'f0 == f1 == m_fractureBodies.size()'. btFractureDynamicsWorld.cpp 483

Похоже, что это сравнение должно было проверить, находятся ли f0 и f1 в конце массива m_fractureBodies, поскольку они содержат найденную методом findLinearSearch() позицию объекта. Казалось бы, условие проверяет, что f0 равно f1 и равно количеству элементов в m_fractureBodies. В итоге третий операнд здесь сравнивается с 0 или 1. Однако на самом деле это выражение превращается в проверку равны ли f0 и f1, а затем в проверку равно ли m_fractureBodies.size() результату f0 == f1.

И, к счастью, достаточно редкая. Красивая ошибка! Пока мы встречали её только в двух открытых проектах, и что интересно все они были как раз игровыми движками.

Пример 12:

Так, для следующих двух случаев лучше заменить strlen(MyStr.c_str()) и val = "" на MyStr.length() и val.clear() соответственно. При работе со строками зачастую лучше использовать возможности, предоставляемые классом string.

The expression of strlen(MyStr.c_str()) kind can be rewritten as MyStr.length(). V806 Decreased performance. RobotLoggingUtil.cpp 213

FILE* createMinitaurLogFile(const char* fileName, std::string& structTypes, ....)
{ FILE* f = fopen(fileName, "wb"); if (f) { .... fwrite(structTypes.c_str(), strlen(structTypes.c_str()), 1, f); .... } ....
}

V815 Decreased performance. Consider replacing the expression 'val = ""' with 'val.clear()'. b3CommandLineArgs.h 40

void addArgs(int argc, char **argv)
{ .... std::string val; .... val = ""; ....
}

Были и другие предупреждения, но, думаю, можно остановиться. Как видите, статический анализ кода может выявить широкий спектр разнообразнейших ошибок.

И про это мы поговорим ниже. Читать про разовые проверки проектов интересно, но это не является правильным способом использования статических анализаторов кода.

Ошибки, найденные до нас

Было интересно в духе недавней статьи "Ошибки, которые не находит статический анализ кода, потому, что он не используется" попробовать найти баги или недочеты, которые уже были исправлены, но которые был бы способен обнаружить статический анализатор.

Picture 2

В репозитории оказалось не так уж много pull requests и многие из них связаны с внутренней логикой работы движка. Но нашлись и ошибки, которые мог бы обнаружить анализатор.

Пример 13:

char m_deviceExtensions[B3_MAX_STRING_LENGTH]; void b3OpenCLUtils_printDeviceInfo(cl_device_id device)
{ b3OpenCLDeviceInfo info; b3OpenCLUtils::getDeviceInfo(device, &info); .... if (info.m_deviceExtensions != 0) { .... }
}

Комментарий к правке говорит о том, что необходимо было проверить массив на то, что он не пустой, но вместо этого производилась бессмысленная проверка указателя, которая всегда возвращала true. Об этом и говорит предупреждение PVS-Studio на исходный вид проверки:

The 'info.m_deviceExtensions' pointer is always not equal to NULL. V600 Consider inspecting the condition. b3OpenCLUtils.cpp 551

Пример 14:

Сможете сходу найти, в чем проблема в следующей функции?

inline void Matrix4x4::SetIdentity()
{ m12 = m13 = m14 = m21 = m23 = m24 = m13 = m23 = m41 = m42 = m43 = 0.0; m11 = m22 = m33 = m44 = 1.0;
}

Анализатор выдает следующие предупреждения:

LinearR4.h 627 V570 The same value is assigned twice to the 'm23' variable.

LinearR4.h 627 V570 The same value is assigned twice to the 'm13' variable.

Эта ошибка была исправлена табличной формой записи присвоения: Повторные присвоения при такой форме записи сложно отследить невооруженным глазом и в итоге часть элементов матрицы не получила исходного значения.

m12 = m13 = m14 =
m21 = m23 = m24 =
m31 = m32 = m34 =
m41 = m42 = m43 = 0.0;

Пример 15:

Согласно комментарию в пулл реквесте, силы применялись к объектам с неверной стороны. Следующая ошибка в одном из условий функции btSoftBody::addAeroForceToNode() приводила к явному багу.

struct eAeroModel
{ enum _ { V_Point, V_TwoSided, .... END };
}; void btSoftBody::addAeroForceToNode(....)
{ .... if (....) { if (btSoftBody::eAeroModel::V_TwoSided) { .... } .... }
....
}

Эту ошибку PVS-Studio также мог бы найти и выдать следующее предупреждение:

btSoftBody.cpp 542 V768 The enumeration constant 'V_TwoSided' is used as a variable of a Boolean-type.

Исправленная проверка выглядит следующим образом:

if (m_cfg.aeromodel == btSoftBody::eAeroModel::V_TwoSided)
{ ....
}

Вместо эквивалентности свойства объекта одному из перечислителей проверялся сам перечислитель V_TwoSided.

Я просто хотела показать, что регулярное использование статического анализатора кода может выявлять ошибки на самом раннем этапе. Понятно, что я просмотрела не все пулл реквесты, так как такую задачу я себе и не ставила. Статический анализ должен быть встроен в DevOps процесс и являться первичным фильтром багов. Это и есть правильный способ использования статического анализа кода. Всё это хорошо описано в статье "Внедряйте статический анализ в процесс, а не ищите с его помощью баги".

Заключение

Picture 6

Судя по некоторым пулл реквестам, проект иногда проверяется через различные инструменты анализа кода, однако правки вносятся не постепенно, а группами и с большими промежутками во времени. В некоторых реквестах комментарий указывает на то, что изменения были внесены лишь для того, чтобы подавить предупреждения. Такой подход к использованию анализа существенно снижает его пользу, поскольку именно постоянная проверка проекта позволяет исправлять ошибки сразу же, а не ждать, пока проявятся какие-либо явные баги.

сети: Инстаграм, Твиттер, Вконтакте, Телеграм. Если хотите всегда быть в курсе новостей и событий нашей команды, подписывайтесь на наши соц.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: PVS-Studio Looked into the Red Dead Redemption's Bullet Engine

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть