To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
A Second Check of Newton Game Dynamics …

A Second Check of Newton Game Dynamics with PVS-Studio

Apr 20 2020

Some time ago, somewhere on the Internet, I stumbled upon a physics engine called Newton Game Dynamics. Knowing that engine projects are usually big and complex, I decided to check its code with PVS-Studio for any interesting defects. I was especially enthusiastic about this one because my co-worker Andrey Karpov already checked it in 2014 and a second check would be a good opportunity to demonstrate our analyzer's evolution over the past six years. As of this writing, the latest version of Newton Game Dynamics is dated February 27, 2020, which means it has been actively developing for the past six years too. So, hopefully, this article will be interesting not only to us but to the engine's developers as well – and for them it's a chance to fix some bugs and improve their code.

0729_Newton/image1.png

Analysis report

In 2014, PVS-Studio issued:

  • 48 first-level warnings;
  • 79 second-level warnings;
  • 261 third-level warnings.

In 2020, it issued:

  • 124 first-level warnings;
  • 272 second-level warnings;
  • 787 third-level warnings (some of them are pretty interesting too).

This time, there are many more interesting warnings than in Andrey's article, so let's check them out.

Diagnostic messages

Warning 1

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

An element of the tmp[i][2] array is initialized twice in a row. Defects like that are usually a sign of misused copy-paste. This can be fixed by either removing the second initialization if it's not meant to be there or changing the index number to 3 – it all depends on the value of the count variable. Now, I'd like to show you another V519 warning absent in Andrey's article but recorded in our bug 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));
  ....
}

Actually, this bug didn't show up in the report. Neither did I find the AddBuoyancyForce function in the dgbody.cpp file. And that's just fine: while the ability to detect new bugs is a sign of our analyzer's evolution, the absence of earlier bugs in recent project versions is a sign of the project's own evolution.

A bit of off-topic speculation

I'm not the one to judge if snippets below contain bugs or if their behavior fails the programmer's expectations, but they do look suspicious.

This snippet triggered two warnings at once:

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

This code might be used for debugging purposes – if so, turning off the loop is a normal trick. There were a few more cases like that:

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

Many of you might argue that changes like that to publicly available code should be, at the very least, commented. Well, I'm with you on this one. I believe that certain features that are fine for a pet project shouldn't be allowed in a project intended for use by many people. But the choice is still up to the authors.

Warning 2

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 here is that result doesn't change once it's initialized. The resulting pointer is pointless; you can't use it.

Warnings 3, 4, 5

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

The second condition seems to be a clone of the first one and was meant to look like this:

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

Here's another very similar bug:

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

This one should be fixed as follows:

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

Another copy-paste error:

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

I guess the z variable should be initialized as follows:

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

Warnings 6, 7

Like any other big C or C++ project, Newton Game Dynamics failed to steer clear of unsafe pointer handling bugs. These are typically hard to find and debug and they cause programs to crash – that is, they are highly dangerous and unpredictable. Luckily, many of them are easily detected by our analyzer. It doesn't seem to be a very original idea that writing a check for a pointer and moving on with a light heart is way better than wasting time trying to reproduce the bug, tracing the problem spot, and debugging it, does it? Anyway, here are some of the warnings of this type:

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 implementation of the NewFace function isn't big, so I'll include 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;
}

In one of its exit points, the NewFace function returns NULL, which will in its turn lead to null pointer dereferencing with undefined behavior as a result.

Here's a similar case of null pointer dereferencing, but more dangerous:

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's the implementation 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;
  }
}

This time, NULL is returned at two exit points out of three.

In total, the analyzer issued 48 V522 warnings. They are similar for the most part, so I don't see any point in discussing more here.

Warning 8

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 value of the pointer returned by the new operator is compared with zero. This usually means that you'll get unexpected behavior if memory allocation fails. When the new operator fails to allocate the required storage, a std::bad_alloc() exception should be thrown, as prescribed by the C++ standard. In this particular case, it means the condition will never execute, which is obviously different from the behavior the programmer counted on. They wanted the program to close the file in the case of memory allocation failure. But the program won't do that and will instead end up with a resource leak.

Warnings 9, 10, 11

  • 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

These are the calls to the function:

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

And this is its declaration:

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

This diagnostic detects function calls with presumably swapped arguments.

Warnings 12, 13

The analyzer issued warnings on two similar methods of 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 spot is the i < data->m_faceCount part of the condition. Since data->m_faceCount is assigned the value 0, this loop won't execute even once. I guess the programmer forgot to reinitialize the m_faceCount field and simply cloned the method's body.

Warnings 14, 15

The analyzer issued two warnings on two similar adjacent lines:

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 is that the allocated memory block is being handled as if it were an array of objects that have a constructor or destructor. But when memory is allocated the way it's done here, the constructor won't be called. Neither will the destructor be called when freeing the memory. This code is very suspicious. The program may end up handling uninitialized variables and running into other troubles. Another problem with this approach is that, unlike with the malloc/free technique, you won't get an explicit error message if you try to have more memory allocated than the machine could provide. Instead, you'll get a segmentation error when trying to access that memory. A few more messages of this type:

  • 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
  • About 10 more warnings like that.

Conclusion

As usual, PVS-Studio didn't let us down and found a few interesting bugs. And that means it's doing great and helps make the world a better place. If you want to try PVS-Studio on your own project, you can get it here.

Popular related articles
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept