Get best of the week

Volver a comprobar Newton Game Dynamics con el analizador estático PVS-Studio

Foto 1

Recientemente, en Internet, descubrí el motor de física Newton Game Dynamics. Sabiendo que tales proyectos generalmente tienen una gran cantidad de código complejo, pensé que sería interesante verificarlo con el analizador estático PVS-Studio. Mi entusiasmo fue estimulado aún más por el hecho de que mi colega Andrei Karpov ya probó este proyecto en 2014, lo que significa que también es una buena oportunidad para demostrar el desarrollo de nuestro analizador en los últimos seis años. También vale la pena señalar que en el momento de escribir este artículo, la última versión de Newton Game Dynamics está fechada el 27 de febrero de 2020, es decir, este proyecto también se está desarrollando activamente durante los últimos 6 años. Por lo tanto, espero que, además de nuestro equipo, este artículo sea de interés para los desarrolladores del motor, que podrán eliminar algunos errores y corregir su código.

Salida del analizador


En 2014, PVS-Studio emitió:

  • 48 alertas de primer nivel;
  • 79 segundo nivel;
  • 261 tercer nivel.

Para 2020, sin embargo:

  • 124 advertencias de primer nivel;
  • 272 segundo nivel;
  • 787 del tercer nivel (entre los cuales también hay otros interesantes).

Hay advertencias mucho más interesantes que en el artículo de Andrey , pero las estudiaremos con más detalle.

Descripción de advertencias


Advertencia N1

V519 A la variable 'tmp [i] [2]' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 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);
  }
  ....
}

El elemento de matriz tmp [i] [2] se inicializa dos veces seguidas. Muy a menudo, dicho código habla de copiar y pegar. Puede solucionar esto eliminando la inicialización innecesaria si no es necesaria, o reemplazando el índice de matriz por uno posterior; esto ya depende del valor de la variable de conteo . Además, me gustaría describir otra advertencia V519 , que Andrei no tiene en el artículo, pero tiene nuestra base de datos de errores :

V519 Al objeto 'húmedo' se le asignan valores dos veces sucesivamente. Quizás esto sea un error. física 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));
  ....
}

Lo admito, no encontré este error en el registro del analizador. Además, no he encontrado el AddBuoyancyForce función en dgbody.cpp . Esto es normal: si los nuevos ejemplos de errores encontrados utilizando las advertencias de nuestro analizador son un indicador del desarrollo de PVS-Studio, entonces la ausencia de errores encontrados anteriormente en el proyecto es un indicador del desarrollo del proyecto.

Un pequeño tema

no creo que diga que los siguientes fragmentos de código contienen errores o no funcionan según lo esperado por el programador, pero se comportan de manera bastante sospechosa.

El analizador emitió dos advertencias para este fragmento de código:

V621 Considere inspeccionar el operador 'para'. Es posible que el bucle se ejecute incorrectamente o no se ejecute en absoluto. MultiBodyCar.cpp 942

V654 La condición 'i <count' del bucle siempre es falsa. 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());
    }
  }
  ....
}

Quizás este código se usa para depurar, luego apagar el bucle parece un movimiento normal. También se descubrieron otros puntos similares:

V519 A la variable 'ret' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 325, 326. dString.cpp 326

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

V519 A la variable 'ret' se le asignan valores dos veces seguidas. Quizás sea un error. Líneas de verificación: 1222, 1223. DemoEntityManager.cpp 1223

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

V560 Una parte de la expresión condicional siempre es verdadera: (cuenta <10). dMathDefines.h 726

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

V654 La condición 'ptr! = Edge' del bucle siempre es falsa. dgPolyhedra.cpp 1571

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

El parámetro 'count' V763 siempre se reescribe en el cuerpo de la función antes de usarse. ConvexCast.cpp 31

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

V547 La expresión 'axisCount' siempre es falsa. MultiBodyCar.cpp 650

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

Probablemente, muchos dirán que al hacer un cambio en el código almacenado en el dominio público, al menos debe escribir un comentario. Bueno, estoy de acuerdo. Algunas cosas que podrían usarse sin dolor en un proyecto de mascotas, en mi opinión, son inaceptables en el código que utilizará una gran cantidad de personas. Sin embargo, la elección depende de los autores.

Advertencia N2

V769 El puntero 'resultado' en la expresión 'resultado + i' es igual a nullptr. El valor resultante no tiene sentido y no debe usarse. 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;
    }
}

El problema es que el resultado no ha cambiado desde la inicialización. El puntero resultante no tendrá sentido, no se puede usar.

Advertencia N3, N4, N5

V778 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'm_colorChannel' en lugar 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]);
  }
}

Parece que el programador ha copiado dos condiciones. El segundo debería verse así:

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

También se encontró otro error muy similar:

V524 Es extraño que el cuerpo de la función 'EnabledAxis1' sea completamente equivalente al cuerpo de la función 'EnabledAxis0'. dCustomDoubleHingeActuator.cpp 88

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

Aquí el código debe arreglarse así:

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

Otra copia y pega:

V525 El código contiene la colección de bloques similares. Verifique los elementos 'm_x', 'm_y', 'm_y' en las líneas 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);  <=
  ....
  }
  ....
}

Lo más probable es que la inicialización de la variable z se vea así:

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

Advertencias N6, N7

En Newton Game Dynamics, como en casi cualquier proyecto grande de C o C ++, hubo algunas advertencias sobre el trabajo inseguro con punteros. Tales errores son a menudo muy difíciles de encontrar y depurar; causan bloqueos del programa; en general, son muy peligrosos e impredecibles. Afortunadamente, nuestro analizador puede detectar muchos de estos errores. Parece obvio que es mejor escribir un cheque una vez y no tomar un baño de vapor, que pasar mucho tiempo reproduciendo el problema, encontrando el lugar del problema en el código y depurándolo. Estas son algunas de las advertencias:

V522 Puede haber una desreferenciación de una 'cara' de puntero nulo potencial. dgContactSolver.cpp 351

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

La definición de la función NewFace es pequeña, así que la daré completa:

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

Una de las funciones de los puntos de salida NewFace devuelve NULL , si se produce una desreferencia de puntero nulo devuelto y habrá un comportamiento indefinido del programa.

También hay una advertencia similar a una pieza de código más peligrosa:

V522 Puede haber una desreferenciación de un 'perímetro' de puntero nulo potencial. dgPolyhedra.cpp 2541

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

Aquí está la definición de 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;
  }
}

Aquí, NULL es el valor de retorno en dos puntos de los tres posibles puntos de salida de la función.

En total, recibí 48 advertencias V522 . En su mayor parte, son del mismo tipo, por lo que no veo ninguna razón para describir más en el marco de este artículo.

Advertencia N8

V668 No tiene sentido probar el puntero 'pBits' contra nulo, ya que la memoria se asignó utilizando el operador 'nuevo'. La excepción se generará en caso de error de asignación de memoria. TargaToOpenGl.cpp 166

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

El analizador detectó una situación en la que el valor del puntero devuelto por el nuevo operador se compara con cero. Como regla, esto significa que el programa, si es imposible asignar memoria, se comportará de manera diferente a lo que el programador espera. Si el nuevo operador no pudo asignar memoria, entonces, de acuerdo con el estándar del lenguaje C ++ , se produce una excepción std :: bad_alloc (). Por lo tanto, la condición nunca se cumplirá. Claramente, este no es el comportamiento con el que contaba el programador. Planeaba cerrar el archivo en caso de un error de asignación de memoria. Esto no sucederá y se producirá una fuga de recursos.

Advertencias N9, N10, N11

  • V764 Posible orden incorrecto de argumentos pasados ​​a la función 'CreateWheel': 'altura' y 'radio'. StandardJoints.cpp 791
  • V764 Posible orden incorrecto de argumentos pasados ​​a la función 'CreateWheel': 'altura' y 'radio'. StandardJoints.cpp 833
  • V764 Posible orden incorrecto de argumentos pasados ​​a la función 'CreateWheel': 'altura' y 'radio'. StandardJoints.cpp 884

Así es como se ven las llamadas a funciones:

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

Así es como se ve la declaración de función:

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

Este diagnóstico sugiere que al llamar a funciones, quizás los argumentos se confundieron.

Advertencias N12, N13

El analizador emitió advertencias para dos métodos similares con nombres diferentes:

V621 Considere la posibilidad de inspeccionar el operador 'para'. Es posible que el bucle se ejecute incorrectamente o no se ejecute en absoluto. dgCollisionUserMesh.cpp 161

V621 Considere inspeccionar el operador 'for'. Es posible que el bucle se ejecute incorrectamente o no se ejecute en absoluto. 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++)
  {
    ....
  }
  ....
}

El problema en esta parte de la condición es: i <data-> m_faceCount. Como data-> m_faceCount tiene asignado 0, este bucle no se ejecutará ni una sola vez. Probablemente, al escribir este código, el programador olvidó reiniciar el campo m_faceCount y luego simplemente copió el cuerpo del método.

Advertencia N14, N15

El analizador emitió dos advertencias para líneas de código similares en sucesión:

V630 La función '_alloca' se usa para asignar memoria a una matriz de objetos que son clases que contienen constructores. dgSkeletonContainer.cpp 1341

V630 La función '_alloca' se usa para asignar memoria a una matriz de objetos que son clases que contienen constructores. 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);

El problema con este fragmento de código es que funcionan con memoria asignada como una matriz de objetos que tienen un constructor o destructor. Con esta asignación de memoria para la clase, no se llamará al constructor. Al liberar memoria, no se llamará al destructor. Esto es extremadamente sospechoso. Tal código puede llevar a trabajar con variables no inicializadas y otros errores. Además, en comparación con el enfoque que usa malloc / free , dicho código es malo ya que si intenta asignar más memoria de la que puede proporcionar la máquina, no recibirá un mensaje de error claro. En su lugar, obtiene un error de segmentación cuando intenta acceder a esta memoria. Aquí hay algunos mensajes más del analizador:
  • 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 general, PVS-Studio falló nuevamente, durante la verificación, se descubrieron nuevos errores interesantes. Y esto significa que el analizador hace bien su trabajo, permitiéndonos hacer que el mundo que nos rodea sea un poco más perfecto. Para probar el analizador estático PVS-Studio en su proyecto, puede hacer clic en el enlace .



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Vladislav Stolyarov. Una segunda comprobación de la dinámica de juego de Newton con PVS-Studio .

More articles:


All Articles