🤜 👭 ❤️ Rechecking Newton Game Dynamics with PVS-Studio static analyzer ⚜️ 🅿️ 🤲

Recently, on the Internet, I discovered the physics engine Newton Game Dynamics. Knowing that such projects usually have a large amount of complex code, I thought it would be interesting to check it with the PVS-Studio static analyzer. My enthusiasm was further spurred by the fact that my colleague Andrei Karpov already tested this project in 2014, which means it is also a good opportunity to demonstrate the development of our analyzer over the past six years. It is also worth noting that at the time of writing, the latest release of Newton Game Dynamics is dated February 27, 2020, that is, this project is also actively developing for the past 6 years. Thus, I hope that in addition to our team, this article will be of interest to the developers of the engine, who will be able to get rid of some bugs and fix their code.

Analyzer output

In 2014, PVS-Studio issued:

48 first level alerts;
79 second level;
261 third level.

For 2020, however:

124 first level warnings;
272 second level;
787 of the third level (among which there are also interesting ones).

There are much more interesting warnings than in Andrey’s article , but we’ll study them in more detail.

Description of warnings

Warning 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);
  }
  ....
}

The array element tmp [i] [2] is initialized twice in a row. Most often, such a code speaks of copy-paste. You can fix this by removing unnecessary initialization if it is not needed, or by replacing the array index with a subsequent one — this already depends on the value of the count variable . Further, I would like to describe another warning V519 , which Andrei does not have in the article, but has our error database :

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));
  ....
}

I admit, I did not find this error in the analyzer log. Also, I did not find the AddBuoyancyForce function in dgbody.cpp . This is normal: if new examples of errors found using the warnings of our analyzer are an indicator of the development of PVS-Studio, then the absence of errors found earlier in the project is an indicator of the development of the project.

A small offtopic

I can’t pretend to say that the following code fragments contain errors or do not work as expected by the programmer, but they behave rather suspiciously.

The analyzer issued two warnings for this code fragment:

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());
    }
  }
  ....
}

Perhaps this code is used for debugging, then turning off the loop seems like a normal move. Several other similar points were also discovered:

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)
  {
    ....
  }
  ....
}

Probably, many will say that when making such a change in the code stored in the public domain, you must at least write a comment. Well, I agree. Some things that could be painlessly used in a pet project, in my opinion, are unacceptable in the code that a large number of people will use. However, the choice is up to the authors.

Warning 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;
    }
}

The problem is that result has not changed since initialization. The resulting pointer will not make sense, it can not be used.

Warning 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]);
  }
}

It seems that the programmer has copied two conditions. The second one should look like this:

if (m_attrib.m_colorChannel.m_count) <= 
{
  attibutes.m_colorChannel.
  PushBack(m_attrib.m_colorChannel[m_constructionIndex + i]);
}

Another very similar error was also found:

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;  <=
}

Here the code should be fixed like this:

void dCustomDoubleHingeActuator::EnabledAxis1(bool state)
{
  m_axis1Enable = state;
}

Another copy-paste:

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);  <=
  ....
  }
  ....
}

Most likely, the initialization of the variable z should look like this:

dFloat z = dGaussianRandom(size.m_z * 0.1f);

Warnings N6, N7

In Newton Game Dynamics, as in almost any large C or C ++ project, there were some warnings about unsafe work with pointers. Such errors are often very difficult to find and debug; they cause program crashes; in general, they are very dangerous and unpredictable. Fortunately, our analyzer is able to detect many of these errors. It seems obvious that it’s better to write a check once and not take a steam bath, than spend a lot of time reproducing the problem, finding the problem spot in the code and debugging it. Here are some of the warnings:

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; 
  ....
}

The definition of the NewFace function is small, so I will give it in full:

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;
}

One of the functions of the exit points NewFace returns NULL , if returned null pointer dereferencing occur and there will be an undefined behavior of the program.

There is also a similar warning to a more dangerous piece of code:

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;
  ....
}

Here is the definition of 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;
  }
}

Here, NULL is the return value at two points of the function's three possible exit points.

In total, I received 48 V522 warnings . For the most part, they are of the same type, so I don’t see any reason to describe any more in the framework of this article.

Warning 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;
}

The analyzer detected a situation where the value of the pointer returned by the new operator is compared to zero. As a rule, this means that the program, if it is impossible to allocate memory, will behave differently than the programmer expects. If the new operator could not allocate memory, then, according to the C ++ language standard , an exception std :: bad_alloc () is thrown. Thus, the condition will never be fulfilled. This is clearly not the behavior the programmer was counting on. He planned to close the file in case of a memory allocation error. This will not happen and a resource leak will occur.

Warnings 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

This is how the function calls look:

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

This is how the function declaration looks:

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

This diagnostics suggests that when calling functions, perhaps the arguments were mixed up.

Warnings N12, N13

The analyzer issued warnings for two similar methods with different names:

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++)
  {
    ....
  }
  ....
}

The problem in this part of the condition is: i <data-> m_faceCount. Since data-> m_faceCount is assigned 0, this loop will not be executed even once. Probably, when writing this piece of code, the programmer forgot to reinitialize the m_faceCount field , and then simply copied the method body.

Warning N14, N15

The analyzer issued two warnings for similar lines of code in succession:

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);

The problem with this code fragment is that they work with allocated memory as an array of objects that have a constructor or destructor. With this allocation of memory for the class, the constructor will not be called. When freeing memory, the destructor will not be called. This is extremely suspicious. Such code can lead to work with uninitialized variables and other errors. In addition, compared to the approach using malloc / free , such code is bad in that if you try to allocate more memory than the machine can provide, you will not receive a clear error message. Instead, you get a segmentation error when you try to access this memory. Here are some more analyzer messages:

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 .

In general, PVS-Studio failed again, during the verification, new interesting errors were discovered. And this means that the analyzer does its job well, allowing us to make the world around us a little more perfect. In order to try the PVS-Studio static analyzer on your project, you can click on the link .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Vladislav Stolyarov. A Second Check of Newton Game Dynamics with PVS-Studio .

Rechecking Newton Game Dynamics with PVS-Studio static analyzer

Analyzer output

Description of warnings

More articles: