Get best of the week

Überprüfen der Newton-Spieldynamik mit dem statischen Analysator PVS-Studio

Bild 1

Kürzlich habe ich im Internet die Physik-Engine Newton Game Dynamics entdeckt. Da ich wusste, dass solche Projekte normalerweise eine große Menge an komplexem Code enthalten, hielt ich es für interessant, ihn mit dem statischen Analysator PVS-Studio zu überprüfen. Meine Begeisterung wurde durch die Tatsache weiter beflügelt, dass mein Kollege Andrei Karpov dieses Projekt bereits 2014 getestet hat. Dies ist auch eine gute Gelegenheit, die Entwicklung unseres Analysegeräts in den letzten sechs Jahren zu demonstrieren. Es ist auch erwähnenswert, dass zum Zeitpunkt des Schreibens die neueste Version von Newton Game Dynamics vom 27. Februar 2020 datiert ist, dh dieses Projekt wird auch in den letzten 6 Jahren aktiv weiterentwickelt. Daher hoffe ich, dass dieser Artikel zusätzlich zu unserem Team für die Entwickler der Engine von Interesse sein wird, die in der Lage sein werden, einige Fehler zu beseitigen und ihren Code zu beheben.

Analysatorausgabe


Im Jahr 2014 veröffentlichte PVS-Studio:

  • 48 Warnungen der ersten Ebene;
  • 79 zweite Ebene;
  • 261 dritte Ebene.

Für 2020 jedoch:

  • 124 Warnungen der ersten Ebene;
  • 272 zweite Ebene;
  • 787 der dritten Ebene (unter denen es auch interessante gibt).

Es gibt viel interessantere Warnungen als in Andreys Artikel , aber wir werden sie genauer untersuchen.

Beschreibung der Warnungen


Warnung N1

V519 Der Variablen 'tmp [i] [2]' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 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);
  }
  ....
}

Das Array-Element tmp [i] [2] wird zweimal hintereinander initialisiert. Meistens spricht ein solcher Code von Copy-Paste. Sie können dieses Problem beheben , indem unnötige Initialisierung zu entfernen , wenn sie nicht benötigt wird, oder durch den Array - Index mit einem nachfolgenden ersetzt wird - das bereits auf dem Wert der abhängig Zahl variabel . Als nächstes würde Ich mag eine weitere Warnung beschreiben , V519 , die Andrei, nicht in dem Artikel haben , aber hat unsere Fehler Datenbank :

V519 Die ‚feuchte‘ Objekt - Werte zweimal nacheinander zugeordnet. Vielleicht ist das ein Fehler. Physik 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));
  ....
}

Ich gebe zu, ich habe diesen Fehler nicht im Analysatorprotokoll gefunden. Auch ich habe nicht die finden AddBuoyancyForce Funktion in dgbody.cpp . Dies ist normal: Wenn neue Beispiele für Fehler, die mithilfe von Warnungen unseres Analysegeräts gefunden wurden, ein Indikator für die Entwicklung von PVS-Studio sind, ist das Fehlen von Fehlern, die zuvor im Projekt gefunden wurden, ein Indikator für die Entwicklung des Projekts.

Ein kleines offtopic

Ich nehme nicht an zu sagen, dass die folgenden Codefragmente Fehler enthalten oder nicht wie vom Programmierer erwartet funktionieren, aber sie verhalten sich ziemlich verdächtig.

Der Analysator hat zwei Warnungen für dieses Codefragment ausgegeben:

V621 Überprüfen Sie den Operator 'for'. Es ist möglich, dass die Schleife falsch oder gar nicht ausgeführt wird. MultiBodyCar.cpp 942

V654 Die Bedingung 'i <count' der Schleife ist immer falsch. 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());
    }
  }
  ....
}

Vielleicht wird dieser Code zum Debuggen verwendet, dann scheint das Ausschalten der Schleife ein normaler Schritt zu sein. Es wurden auch mehrere ähnliche Punkte entdeckt:

V519 Der Variablen 'ret' werden zweimal nacheinander Werte zugewiesen. Vielleicht ist dies ein Fehler. Überprüfen Sie die Zeilen: 325, 326. dString.cpp 326

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

V519 Der Variablen 'ret' werden zweimal hintereinander Werte zugewiesen. Möglicherweise ist dies ein Fehler. Überprüfen Sie die Zeilen: 1222, 1223. DemoEntityManager.cpp 1223

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

V560 Ein Teil des bedingten Ausdrucks ist immer wahr: (Anzahl <10). dMathDefines.h 726

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

V654 Die Bedingung 'ptr! = Edge' der Schleife ist immer falsch. dgPolyhedra.cpp 1571

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

V763 Der Parameter 'count' wird vor seiner Verwendung immer im Funktionskörper neu geschrieben. ConvexCast.cpp 31

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

V547 Der Ausdruck 'axisCount' ist immer falsch. MultiBodyCar.cpp 650

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

Wahrscheinlich werden viele sagen, dass Sie bei einer solchen Änderung des öffentlich zugänglichen Codes mindestens einen Kommentar schreiben müssen. Nun, ich stimme zu. Einige Dinge, die meiner Meinung nach in einem Haustierprojekt schmerzlos verwendet werden könnten, sind in dem Code, den eine große Anzahl von Menschen verwenden wird, nicht akzeptabel. Die Wahl liegt jedoch bei den Autoren.

Warnung N2

V769 Der Zeiger 'result' im Ausdruck 'result + i' entspricht nullptr. Der resultierende Wert ist sinnlos und sollte nicht verwendet werden. 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;
    }
}

Das Problem ist, dass sich das Ergebnis seit der Initialisierung nicht geändert hat. Der resultierende Zeiger ist nicht sinnvoll und kann nicht verwendet werden.

Warnung N3, N4, N5

V778 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise ist dies ein Tippfehler und die Variable 'm_colorChannel' sollte anstelle von 'm_binormalChannel' verwendet werden. 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]);
  }
}

Es scheint, dass der Programmierer zwei Bedingungen kopiert hat. Der zweite sollte so aussehen:

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

Ein weiterer sehr ähnlicher Fehler wurde ebenfalls gefunden:

V524 Es ist seltsam, dass der Hauptteil der Funktion 'EnabledAxis1' dem Hauptteil der Funktion 'EnabledAxis0' vollständig entspricht. dCustomDoubleHingeActuator.cpp 88

void dCustomDoubleHingeActuator::EnabledAxis0(bool state)
{
  m_axis0Enable = state;  <=
}
void dCustomDoubleHingeActuator::EnabledAxis1(bool state)
{
  m_axis0Enable = state;  <=
}

Hier sollte der Code wie folgt festgelegt werden:

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

Noch ein Copy-Paste:

V525 Der Code enthält die Sammlung ähnlicher Blöcke. Überprüfen Sie die Elemente 'm_x', 'm_y', 'm_y' in den Zeilen 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);  <=
  ....
  }
  ....
}

Höchstwahrscheinlich sollte die Initialisierung der Variablen z folgendermaßen aussehen:

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

Warnungen N6, N7

In Newton Game Dynamics gab es wie in fast jedem großen C- oder C ++ - Projekt einige Warnungen vor unsicherer Arbeit mit Zeigern. Solche Fehler sind oft sehr schwer zu finden und zu debuggen, sie verursachen Programmabstürze, im Allgemeinen sind sie sehr gefährlich und unvorhersehbar. Glücklicherweise kann unser Analysator viele dieser Fehler erkennen. Es scheint offensichtlich, dass es besser ist, einen Scheck einmal zu schreiben und kein Dampfbad zu nehmen, als viel Zeit damit zu verbringen, das Problem zu reproduzieren, den Problemort im Code zu finden und ihn zu debuggen. Hier sind einige der Warnungen:

V522 Möglicherweise wird ein potenzielles Nullzeiger-Gesicht dereferenziert. dgContactSolver.cpp 351

DG_INLINE dgMinkFace* dgContactSolver::AddFace(dgInt32 v0,dgInt32 v1,
                                               dgInt32 v2)
{
  dgMinkFace* const face = NewFace();
  face->m_mark = 0; 
  ....
}

Die Definition der NewFace-Funktion ist klein, daher werde ich sie vollständig wiedergeben:

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

Eine der Funktionen der Exit-Punkte NewFace gibt NULL zurück , wenn eine Nullzeiger-Dereferenzierung zurückgegeben wird und das Programm undefiniert ist.

Es gibt auch eine ähnliche Warnung wie bei einem gefährlicheren Code:

V522 Möglicherweise wird ein potenzieller Nullzeiger-Perimeter dereferenziert. dgPolyhedra.cpp 2541

bool dgPolyhedra::PolygonizeFace(....)
{
  ....
  dgEdge* const perimeter = flatFace.AddHalfEdge
                           (edge1->m_next->m_incidentVertex,
                            edge1->m_incidentVertex);
  perimeter->m_twin = edge1;
  ....
}

Hier ist die Definition von 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;
  }
}

Hier ist NULL der Rückgabewert an zwei Punkten der drei möglichen Austrittspunkte der Funktion.

Insgesamt habe ich 48 V522- Warnungen erhalten . Zum größten Teil sind sie vom gleichen Typ, daher sehe ich keinen Grund, sie im Rahmen dieses Artikels weiter zu beschreiben.

Warnung N8

V668 Es macht keinen Sinn, den Zeiger 'pBits' gegen Null zu testen, da der Speicher mit dem Operator 'new' zugewiesen wurde. Die Ausnahme wird im Falle eines Speicherzuordnungsfehlers generiert. TargaToOpenGl.cpp 166

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

Der Analysator hat eine Situation festgestellt, in der der Wert des vom neuen Operator zurückgegebenen Zeigers mit Null verglichen wird. In der Regel bedeutet dies, dass sich das Programm anders verhält, als es der Programmierer erwartet, wenn es nicht möglich ist, Speicher zuzuweisen. Wenn der neue Operator keinen Speicher zuweisen konnte, wird gemäß dem C ++ - Sprachstandard eine Ausnahme std :: bad_alloc () ausgelöst. Somit wird die Bedingung niemals erfüllt. Dies ist eindeutig nicht das Verhalten, auf das sich der Programmierer verlassen hat. Er plante, die Datei im Falle eines Speicherzuordnungsfehlers zu schließen. Dies wird nicht passieren und ein Ressourcenleck wird auftreten.

Warnungen N9, N10, N11

  • V764 Möglicherweise falsche Reihenfolge der an die Funktion 'CreateWheel' übergebenen Argumente: 'height' und 'radius'. StandardJoints.cpp 791
  • V764 Möglicherweise falsche Reihenfolge der an die Funktion 'CreateWheel' übergebenen Argumente: 'height' und 'radius'. StandardJoints.cpp 833
  • V764 Möglicherweise falsche Reihenfolge der an die Funktion 'CreateWheel' übergebenen Argumente: 'height' und 'radius'. StandardJoints.cpp 884

So sehen die Funktionsaufrufe aus:

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

So sieht die Funktionsdeklaration aus:

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

Diese Diagnose legt nahe, dass beim Aufrufen von Funktionen möglicherweise die Argumente verwechselt wurden.

Warnungen N12, N13

Der Analysator gab Warnungen für zwei ähnliche Methoden mit unterschiedlichen Namen aus:

V621 Überprüfen Sie den 'for'-Operator. Es ist möglich, dass die Schleife falsch oder gar nicht ausgeführt wird. dgCollisionUserMesh.cpp 161

V621 Überprüfen Sie den Operator 'for'. Es ist möglich, dass die Schleife falsch oder gar nicht ausgeführt wird. 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++)
  {
    ....
  }
  ....
}

Das Problem in diesem Teil der Bedingung ist: i <data-> m_faceCount. Da data-> m_faceCount 0 zugewiesen ist, wird diese Schleife nicht einmal ausgeführt. Wahrscheinlich hat der Programmierer beim Schreiben dieses Codes vergessen, das Feld m_faceCount neu zu initialisieren , und dann einfach den Methodenkörper kopiert.

Warnung N14, N15

Der Analysator gab nacheinander zwei Warnungen für ähnliche Codezeilen aus:

V630 Mit der Funktion '_alloca' wird Speicher für ein Array von Objekten zugewiesen, bei denen es sich um Klassen handelt, die Konstruktoren enthalten. dgSkeletonContainer.cpp 1341

V630 Mit der Funktion '_alloca' wird Speicher für ein Array von Objekten zugewiesen, bei denen es sich um Klassen handelt, die Konstruktoren enthalten. 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);

Das Problem mit diesem Codefragment besteht darin, dass sie mit zugewiesenem Speicher als Array von Objekten arbeiten, die einen Konstruktor oder Destruktor haben. Bei dieser Speicherzuweisung für die Klasse wird der Konstruktor nicht aufgerufen. Beim Freigeben des Speichers wird der Destruktor nicht aufgerufen. Das ist äußerst verdächtig. Ein solcher Code kann dazu führen, dass mit nicht initialisierten Variablen und anderen Fehlern gearbeitet wird. Im Vergleich zum Ansatz mit malloc / free ist ein solcher Code außerdem insofern schlecht, als Sie keine eindeutige Fehlermeldung erhalten, wenn Sie versuchen, mehr Speicher zuzuweisen, als der Computer bereitstellen kann. Stattdessen wird ein Segmentierungsfehler angezeigt, wenn Sie versuchen, auf diesen Speicher zuzugreifen. Hier sind einige weitere Analysatormeldungen:
  • 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 .


Im Allgemeinen ist PVS-Studio erneut fehlgeschlagen. Bei der Überprüfung wurden neue interessante Fehler entdeckt. Und das bedeutet, dass der Analysator seine Arbeit gut macht und es uns ermöglicht, die Welt um uns herum ein wenig perfekter zu gestalten. Um den statischen Analysator PVS-Studio in Ihrem Projekt zu testen, können Sie auf den Link klicken .



Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Vladislav Stolyarov. Eine zweite Überprüfung der Newton-Spieldynamik mit PVS-Studio .

More articles:


All Articles