Владислав Столяров

20 Апр 2020

Теги:

#Cpp #GameDev

Повторная проверка Newton Game Dynamics статическим анализатором PVS-Studio

20 Апр 2020

Автор: Владислав Столяров

Вывод анализатора
Описание предупреждений
Заключение

Недавно на просторах интернета мной был обнаружен физический движок Newton Game Dynamics. Зная, что в таких проектах обычно большой объём сложного кода, я подумал, что будет интересно проверить его статическим анализатором PVS-Studio. Мой энтузиазм ещё больше подстегнуло то, что мой коллега Андрей Карпов уже проверял данный проект в 2014 году, а значит, это ещё и хорошая возможность продемонстрировать развитие нашего анализатора за последние шесть лет. Также стоит отметить, что на момент написания статьи последний релиз Newton Game Dynamics датирован 27 февраля 2020 года, то есть данный проект тоже активно развивается последние 6 лет. Таким образом, надеюсь, что помимо нашей команды, данная статья будет интересна и разработчикам движка, которые смогут избавиться от некоторых багов и исправить свой код.

Вывод анализатора

В 2014 году PVS-Studio выдал:

48 предупреждений первого уровня;
79 второго уровня;
261 третьего уровня.

На 2020 год же:

124 предупреждения первого уровня;
272 второго уровня;
787 третьего уровня (среди которых тоже есть интересные).

Интересных предупреждений существенно больше, чем в статье Андрея, изучим же их поподробнее.

Описание предупреждений

Предупреждение N1

V519 The 'tmp[i][2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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] инициализируется два раза подряд. Чаще всего такой код говорит о копипасте. Исправить это можно, удалив лишнюю инициализацию, если она не нужна, или заменив индекс массива на последующий, – это уже зависит от значения переменной count. Далее я бы хотел описать ещё одно предупреждение V519, которого нет в статье у Андрея, но есть в нашей базе ошибок:

V519 The 'damp' object is assigned values twice successively. Perhaps this is a mistake. physics 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. Это нормально: если новые примеры ошибок, найденные с помощью предупреждений нашего анализатора, - показатель развития PVS-Studio, то отсутствие найденных ранее в проекте ошибок – показатель развития проекта.

Небольшой оффтоп

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

На данный фрагмент кода анализатор выдал два предупреждения:

V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. MultiBodyCar.cpp 942

V654 The condition 'i < count' of loop is always false. 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 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake.Check lines: 325, 326. dString.cpp 326

void dString::LoadFile (FILE* const file)
{
  ....
  size_t ret = fread(m_string, 1, size, file);
  ret = 0;
  ....
}

V519 The 'ret' variable is assigned values twice successively.Perhaps this is a mistake. Check lines: 1222, 1223. DemoEntityManager.cpp 1223

void DemoEntityManager::DeserializeFile (....)
{
  ....
  size_t ret = fread(buffer, size, 1, (FILE*) serializeHandle);
  ret = 0;
  ....
}

V560 A part of conditional expression is always true: (count < 10). dMathDefines.h 726

bool dCholeskyWithRegularizer(....)
{
  ....
  int count = 0;
  while (!pass && (count < 10))
  {
    ....
  }
  ....
}

V654 The condition 'ptr != edge' of loop is always false. dgPolyhedra.cpp 1571

void dgPolyhedra::Triangulate (....)
{
  ....
  ptr = edge;
  ....
  while (ptr != edge);
  ....
}

V763 Parameter 'count' is always rewritten in function body before being used. ConvexCast.cpp 31

StupidComplexOfConvexShapes (...., int count)
{
  count = 40;
  //count = 1;
  ....
}

V547 Expression 'axisCount' is always false. MultiBodyCar.cpp 650

void UpdateDriverInput(dVehicle* const vehicle, dFloat timestep) 
{
  ....
  int axisCount = scene->GetJoystickAxis(axis);
  axisCount = 0;
  if (axisCount)
  {
    ....
  }
  ....
}

Наверное, многие скажут, что при внесении изменения такого рода в код, хранящийся в открытом доступе, надо, как минимум, написать комментарий. Что же, я согласен. Некоторые вещи, которые могли бы безболезненно быть использованы в pet-проекте, по моему мнению, недопустимы в коде, которым будет пользоваться большое количество людей. Однако выбор остаётся за авторами.

Предупреждение N2

V769 The 'result' pointer in the 'result + i' expression equals nullptr. The resulting value is senseless and it should not be used. 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 не меняется с момента инициализации. Полученный указатель не будет иметь смысла, его нельзя использовать.

Предупреждение N3, N4, N5

V778 Two similar code fragments were found. Perhaps, this is a typo and 'm_colorChannel' variable should be used instead of 'm_binormalChannel'. 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 It is odd that the body of 'EnabledAxis1' function is fully equivalent to the body of 'EnabledAxis0' function. 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 The code contains the collection of similar blocks. Check items 'm_x', 'm_y', 'm_y' in lines 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);

Предупреждения N6, N7

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

V522 There might be dereferencing of a potential null pointer 'face'. 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 There might be dereferencing of a potential null pointer 'perimeter'. 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. В большинстве своём они однотипны, поэтому описывать ещё какие-то в рамках этой статьи не вижу смысла.

Предупреждение N8

V668 There is no sense in testing the 'pBits' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. TargaToOpenGl.cpp 166

char* const pBits = new char [width * height * 4];
if(pBits == NULL) 
{
  fclose(pFile);
  return 0;
}

Анализатор обнаружил ситуацию, когда значение указателя, возвращаемого оператором new, сравнивается с нулём. Как правило, это значит, что программа при невозможности выделить память будет вести себя не так, как ожидает программист. Если оператор new не смог выделить память, то, согласно стандарту языка С++, генерируется исключение std::bad_alloc(). Таким образом, условие никогда не выполнится. Это явно не то поведение, на которое рассчитывал программист. Он планировал в случае ошибки выделения памяти закрыть файл. Этого не произойдёт и возникнет утечка ресурса.

Предупреждения N9, N10, N11

V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 791
V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 833
V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 884

Так выглядят вызовы функции:

NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);

Так выглядит объявление функции:

static NewtonBody* CreateWheel (DemoEntityManager* const scene,
  const dVector& location, dFloat radius, dFloat height)

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

Предупреждения N12, 13

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

V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. dgCollisionUserMesh.cpp 161

V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. 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, а потом просто скопипастил тело метода.

Предупреждение N14,N15

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

V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. dgSkeletonContainer.cpp 1341

V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. 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 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. dVehicleSolver.cpp 498
V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. dVehicleSolver.cpp 499
V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. dVehicleSolver.cpp 1144
И ещё около 10 подобных предупреждений.

Заключение

В общем, PVS-Studio снова не подвёл, в ходе проверки были обнаружены новые интересные ошибки. А это значит, что анализатор отлично справляется со своей работой, позволяя делать мир вокруг нас чуть более совершенным. Для того, чтобы попробовать статический анализатор PVS-Studio на своём проекте, вы можете перейти по ссылке.

#Cpp #GameDev