Виктория Ханиева

30 Июл 2019

Теги:

#Cpp #GameDev

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

30 Июл 2019

Автор: Виктория Ханиева

Для разогрева
Копипаста
За пределами дозволенного: выход за границы массива
Один раз отмерь - семь раз отрежь
Утечка
Тут свои законы
Ошибки, найденные до нас
Заключение

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

Эта библиотека свободно распространяется, так что все могут при желании использовать её и в своих проектах. Кроме Red Dead Redemption этот физический движок также используется и в киноиндустрии для создания спецэффектов. Например, он был задействован при съемках "Шерлока Холмса" Гая Ричи для расчета столкновений.

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

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

Пример 1:

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

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

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

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

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

Копипаста

Пример 2:

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

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

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:

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

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

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

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

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

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

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

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

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

float m_fJacCoeffInv[2];      
static inline void b3SolveFriction(b3ContactConstraint4& cs, ....)
{
  if (cs.m_fJacCoeffInv[0] == 0 && cs.m_fJacCoeffInv[0] == 0)
  {
    return;
  }
  ....
}

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

Пример 5:

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

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

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, но условие проверки было продублировано. Скорее всего это также ошибка копипасты.

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

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

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

Пример 6:

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

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

V557 Array overrun is possible. The value of 'dofIndex + dof' index could reach 130. 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, но это не устраняет ошибку, а просто делает её проявление редким.

V557 Array overrun is possible. The value of 'len' index could reach 1024. 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 мест в коде, где можно внести такую поправку.

V807 Decreased performance. Consider creating a pointer to avoid using the 'm_app->m_renderer->getActiveCamera()' expression repeatedly. 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:

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

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

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:

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

V773 Visibility scope of the 'importer' pointer was exited without releasing the memory. A memory leak is possible. 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;
    ....
  }
....
}

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

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

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

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

Пример 12:

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

V806 Decreased performance. The expression of strlen(MyStr.c_str()) kind can be rewritten as MyStr.length(). 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 = "";
  ....
}

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

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

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

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

В репозитории оказалось не так уж много 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 на исходный вид проверки:

V600 Consider inspecting the condition. The 'info.m_deviceExtensions' pointer is always not equal to NULL. 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;
}

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

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. LinearR4.h 627

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

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 также мог бы найти и выдать следующее предупреждение:

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

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

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

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

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

Заключение

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

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

#Cpp #GameDev