Get best of the week

Revérifier Newton Game Dynamics avec l'analyseur statique PVS-Studio

Image 1

Récemment, sur Internet, j'ai découvert le moteur physique Newton Game Dynamics. Sachant que de tels projets ont généralement une grande quantité de code complexe, j'ai pensé qu'il serait intéressant de le vérifier avec l'analyseur statique PVS-Studio. Mon enthousiasme a été stimulé par le fait que mon collègue Andrei Karpov a déjà testé ce projet en 2014, ce qui signifie que c'est également une bonne occasion de démontrer le développement de notre analyseur au cours des six dernières années. Il convient également de noter qu'au moment de la rédaction de ce document, la dernière version de Newton Game Dynamics est datée du 27 février 2020, c'est-à-dire que ce projet se développe également activement depuis 6 ans. Ainsi, j'espère qu'en plus de notre équipe, cet article intéressera les développeurs du moteur, qui pourront se débarrasser de quelques bugs et corriger leur code.

Sortie analyseur


En 2014, PVS-Studio a publié:

  • 48 alertes de premier niveau;
  • 79 deuxième niveau;
  • 261 troisième niveau.

Pour 2020, cependant:

  • 124 avertissements de premier niveau;
  • 272 deuxième niveau;
  • 787 du troisième niveau (parmi lesquels il y en a aussi des intéressants).

Il y a des avertissements beaucoup plus intéressants que dans l'article d'Andrey , mais nous les étudierons plus en détail.

Description des avertissements


Avertissement N1

V519 La variable 'tmp [i] [2]' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 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);
  }
  ....
}

L'élément de tableau tmp [i] [2] est initialisé deux fois de suite. Le plus souvent, un tel code parle de copier-coller. Vous pouvez résoudre ce problème en supprimant l'initialisation inutile si elle n'est pas nécessaire, ou en remplaçant l'index du tableau par un autre - cela dépend déjà de la valeur de la variable de comptage . Ensuite, je voudrais décrire un autre avertissement V519 , qu'Andrei n'a pas dans l'article, mais a notre base de données d'erreurs :

V519 L'objet 'humide' reçoit des valeurs successives deux fois. C'est peut-être une erreur. physique 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));
  ....
}

J'avoue que je n'ai pas trouvé cette erreur dans le journal de l'analyseur. De plus, je ne trouve pas la AddBuoyancyForce fonction dans dgbody.cpp . Ceci est normal: si de nouveaux exemples d'erreurs détectés à l'aide des avertissements de notre analyseur sont un indicateur du développement de PVS-Studio, alors l'absence d'erreurs détectées plus tôt dans le projet est un indicateur du développement du projet.

Un petit sujet hors sujet,

je ne peux pas prétendre dire que les fragments de code suivants contiennent des erreurs ou ne fonctionnent pas comme prévu par le programmeur, mais ils se comportent plutôt de manière suspecte.

L'analyseur a émis deux avertissements pour ce fragment de code:

V621 Envisagez d'inspecter l'opérateur «pour». Il est possible que la boucle ne soit pas exécutée correctement ou ne soit pas exécutée du tout. MultiBodyCar.cpp 942

V654 La condition 'i <count' de la boucle est toujours fausse. 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());
    }
  }
  ....
}

Peut-être que ce code est utilisé pour le débogage, puis désactiver la boucle semble être un mouvement normal. Plusieurs autres points similaires ont également été découverts:

V519 La variable «ret» reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 325, 326. dString.cpp 326

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

V519 La variable 'ret' se voit attribuer des valeurs deux fois de suite. Il s'agit peut-être d'une erreur. Vérifiez les lignes: 1222, 1223. DemoEntityManager.cpp 1223

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

V560 Une partie de l'expression conditionnelle est toujours vraie: (nombre <10). dMathDefines.h 726

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

V654 La condition 'ptr! = Edge' de la boucle est toujours fausse. dgPolyhedra.cpp 1571

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

V763 Le paramètre 'count' est toujours réécrit dans le corps de la fonction avant d'être utilisé. ConvexCast.cpp 31

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

V547 L'expression 'axisCount' est toujours fausse. MultiBodyCar.cpp 650

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

Beaucoup diront probablement que lorsque vous effectuez un tel changement dans le code stocké dans le domaine public, vous devez au moins écrire un commentaire. Eh bien, je suis d'accord. Certaines choses qui pourraient être utilisées sans douleur dans un projet pour animaux de compagnie, à mon avis, sont inacceptables dans le code qu'un grand nombre de personnes utiliseront. Cependant, le choix appartient aux auteurs.

Avertissement N2

V769 Le pointeur «résultat» dans l'expression «résultat + i» est égal à nullptr. La valeur résultante est insensée et ne doit pas être utilisée. 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;
    }
}

Le problème est que le résultat n'a pas changé depuis l'initialisation. Le pointeur résultant n'a pas de sens, il ne peut pas être utilisé.

Avertissement N3, N4, N5

V778 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable 'm_colorChannel' devrait être utilisée à la place de '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]);
  }
}

Il semble que le programmeur ait copié deux conditions. Le second devrait ressembler à ceci:

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

Une autre erreur très similaire a également été trouvée:

V524 Il est étrange que le corps de la fonction "EnabledAxis1" soit entièrement équivalent au corps de la fonction "EnabledAxis0". dCustomDoubleHingeActuator.cpp 88

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

Ici, le code doit être corrigé comme ceci:

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

Un autre copier-coller:

V525 Le code contient la collection de blocs similaires. Vérifiez les éléments «m_x», «m_y», «m_y» aux lignes 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);  <=
  ....
  }
  ....
}

Très probablement, l'initialisation de la variable z devrait ressembler à ceci:

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

Avertissements N6, N7

Dans Newton Game Dynamics, comme dans presque tous les grands projets C ou C ++, il y avait des avertissements concernant un travail dangereux avec des pointeurs. De telles erreurs sont souvent très difficiles à trouver et à déboguer; elles provoquent des plantages de programmes; en général, elles sont très dangereuses et imprévisibles. Heureusement, notre analyseur est capable de détecter bon nombre de ces erreurs. Il semble évident qu’il est préférable d’écrire un chèque une fois et de ne pas prendre de bain de vapeur, que de passer beaucoup de temps à reproduire le problème, à trouver le point problématique dans le code et à le déboguer. Voici certains des avertissements:

V522 Il pourrait y avoir un déréférencement d'un pointeur nul potentiel 'face'. dgContactSolver.cpp 351

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

La définition de la fonction NewFace est petite, je vais donc la donner dans son intégralité:

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

L'une des fonctions des points de sortie NewFace renvoie NULL , si un déréférencement de pointeur null est renvoyé, il y aura un comportement indéfini du programme.

Il existe également un avertissement similaire à un morceau de code plus dangereux:

V522 Il pourrait y avoir un déréférencement d'un pointeur nul potentiel «périmètre». dgPolyhedra.cpp 2541

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

Voici la définition d' 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;
  }
}

Ici, NULL est la valeur de retour en deux points des trois points de sortie possibles de la fonction.

Au total, j'ai reçu 48 avertissements V522 . Pour la plupart, ils sont du même type, donc je ne vois aucune raison de les décrire davantage dans le cadre de cet article.

Avertissement N8

V668 Il est inutile de tester le pointeur «pBits» par rapport à null, car la mémoire a été allouée à l'aide de l'opérateur «nouveau». L'exception sera générée en cas d'erreur d'allocation de mémoire. TargaToOpenGl.cpp 166

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

L'analyseur a détecté une situation où la valeur du pointeur renvoyé par le nouvel opérateur est comparée à zéro. En règle générale, cela signifie que le programme, s'il est impossible d'allouer de la mémoire, se comportera différemment de ce que le programmeur attend. Si le nouvel opérateur n'a pas pu allouer de mémoire, alors, selon la norme du langage C ++ , une exception std :: bad_alloc () est levée. Ainsi, la condition ne sera jamais remplie. Ce n'est clairement pas le comportement sur lequel le programmeur comptait. Il prévoyait de fermer le fichier en cas d'erreur d'allocation de mémoire. Cela ne se produira pas et une fuite de ressources se produira.

Avertissements N9, N10, N11

  • V764 Ordre incorrect possible des arguments passés à la fonction 'CreateWheel': 'hauteur' et 'rayon'. StandardJoints.cpp 791
  • V764 Ordre incorrect possible des arguments passés à la fonction 'CreateWheel': 'hauteur' et 'rayon'. StandardJoints.cpp 833
  • V764 Ordre incorrect possible des arguments passés à la fonction 'CreateWheel': 'hauteur' et 'rayon'. StandardJoints.cpp 884

Voici à quoi ressemblent les appels de fonction:

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

Voici à quoi ressemble la déclaration de fonction:

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

Ce diagnostic suggère que lors de l'appel de fonctions, les arguments étaient peut-être mélangés.

Avertissements N12, N13

L'analyseur a émis des avertissements pour deux méthodes similaires avec des noms différents:

V621 Envisagez d'inspecter l'opérateur «pour». Il est possible que la boucle ne soit pas exécutée correctement ou ne soit pas exécutée du tout. dgCollisionUserMesh.cpp 161

V621 Envisagez d'inspecter l'opérateur «pour». Il est possible que la boucle ne soit pas exécutée correctement ou ne soit pas exécutée du tout. 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++)
  {
    ....
  }
  ....
}

Le problème dans cette partie de la condition est: i <data-> m_faceCount. Étant donné que data-> m_faceCount est affecté à 0, cette boucle ne sera pas exécutée une seule fois. Probablement, lors de l'écriture de ce morceau de code, le programmeur a oublié de réinitialiser le champ m_faceCount , puis a simplement copié le corps de la méthode.

Avertissement N14, N15

L'analyseur a émis deux avertissements successifs pour des lignes de code similaires:

V630 La fonction '_alloca' est utilisée pour allouer de la mémoire à un tableau d'objets qui sont des classes contenant des constructeurs. dgSkeletonContainer.cpp 1341

V630 La fonction '_alloca' est utilisée pour allouer de la mémoire à un tableau d'objets qui sont des classes contenant des constructeurs. 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);

Le problème avec ce fragment de code est qu'ils fonctionnent avec la mémoire allouée en tant que tableau d'objets qui ont un constructeur ou un destructeur. Avec cette allocation de mémoire pour la classe, le constructeur ne sera pas appelé. Lors de la libération de mémoire, le destructeur ne sera pas appelé. C'est extrêmement suspect. Un tel code peut conduire à travailler avec des variables non initialisées et d'autres erreurs. De plus, par rapport à l'approche utilisant malloc / free , un tel code est mauvais en ce que si vous essayez d'allouer plus de mémoire que la machine ne peut en fournir, vous ne recevrez pas de message d'erreur clair. Au lieu de cela, vous obtenez une erreur de segmentation lorsque vous essayez d'accéder à cette mémoire. Voici quelques messages supplémentaires de l'analyseur:
  • 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 .


En général, PVS-Studio a échoué à nouveau, lors de la vérification, de nouvelles erreurs intéressantes ont été découvertes. Et cela signifie que l'analyseur fait bien son travail, ce qui nous permet de rendre le monde autour de nous un peu plus parfait. Pour essayer l'analyseur statique PVS-Studio sur votre projet, vous pouvez cliquer sur le lien .



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Vladislav Stolyarov. Une deuxième vérification de Newton Game Dynamics avec PVS-Studio .

More articles:


All Articles