Get best of the week

Verificando novamente Newton Game Dynamics com analisador estático PVS-Studio

Imagem 1

Recentemente, na Internet, descobri o mecanismo de física Newton Game Dynamics. Sabendo que esses projetos geralmente possuem uma grande quantidade de código complexo, pensei que seria interessante verificá-lo com o analisador estático PVS-Studio. Meu entusiasmo foi ainda mais estimulado pelo fato de meu colega Andrei Karpov já ter testado esse projeto em 2014, o que significa que também é uma boa oportunidade para demonstrar o desenvolvimento de nosso analisador nos últimos seis anos. Também é importante notar que, no momento da redação deste artigo, a versão mais recente do Newton Game Dynamics é datada de 27 de fevereiro de 2020, ou seja, esse projeto também está se desenvolvendo ativamente nos últimos 6 anos. Portanto, espero que, além de nossa equipe, este artigo seja de interesse dos desenvolvedores do mecanismo, que poderão se livrar de alguns bugs e corrigir seu código.

Saída do analisador


Em 2014, o PVS-Studio emitiu:

  • 48 alertas de primeiro nível;
  • 79 segundo nível;
  • 261 terceiro nível.

Para 2020, no entanto:

  • 124 avisos de primeiro nível;
  • 272 segundo nível;
  • 787 do terceiro nível (entre os quais também há outros interessantes).

Existem avisos muito mais interessantes do que no artigo de Andrey , mas vamos estudá-los com mais detalhes.

Descrição dos avisos


Aviso N1

V519 A variável 'tmp [i] [2]' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 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);
  }
  ....
}

O elemento da matriz tmp [i] [2] é inicializado duas vezes seguidas. Na maioria das vezes, esse código fala de copiar e colar. Você pode corrigir isso removendo a inicialização desnecessária, se não for necessária, ou substituindo o índice da matriz por um subsequente - isso já depende do valor da variável count . Além disso, gostaria de descrever outro aviso V519 , que Andrei não possui no artigo, mas possui nosso banco de dados de erros :

V519 O objeto 'úmido' recebe valores duas vezes sucessivos. Talvez isso seja um erro. 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));
  ....
}

Admito que não encontrei esse erro no log do analisador. Além disso, eu não encontrou o AddBuoyancyForce função em dgbody.cpp . Isso é normal: se novos exemplos de erros encontrados usando os avisos do nosso analisador são um indicador do desenvolvimento do PVS-Studio, a ausência de erros encontrados anteriormente no projeto é um indicador do desenvolvimento do projeto.

Um pequeno tópico off-line,

não posso fingir que os fragmentos de código a seguir contêm erros ou não funcionam conforme o esperado pelo programador, mas se comportam de maneira suspeita.

O analisador emitiu dois avisos para este fragmento de código:

V621 Considere inspecionar o operador 'for'. É possível que o loop seja executado incorretamente ou não seja executado. MultiBodyCar.cpp 942

V654 A condição 'i <count' do loop é sempre 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());
    }
  }
  ....
}

Talvez esse código seja usado para depuração, então desligar o loop parece uma jogada normal. Vários outros pontos semelhantes também foram descobertos:

V519 A variável 'ret' recebe valores duas vezes sucessivamente. Talvez isso seja um erro Verifique as linhas: 325, 326. dString.cpp 326

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

V519 A variável 'ret' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 1222, 1223. DemoEntityManager.cpp 1223

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

V560 Uma parte da expressão condicional é sempre verdadeira: (contagem <10). dMathDefines.h 726

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

V654 A condição 'ptr! = Edge' do loop é sempre falsa. dgPolyhedra.cpp 1571

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

V763 O parâmetro 'count' é sempre reescrito no corpo da função antes de ser usado. ConvexCast.cpp 31

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

V547 A expressão 'axisCount' é sempre falsa. MultiBodyCar.cpp 650

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

Provavelmente, muitos dirão que, ao fazer uma alteração no código armazenado em domínio público, você deve pelo menos escrever um comentário. Bem, eu concordo. Algumas coisas que poderiam ser usadas sem esforço em um projeto para animais de estimação, na minha opinião, são inaceitáveis ​​no código que um grande número de pessoas usará. No entanto, a escolha é dos autores.

Aviso N2

V769 O ponteiro 'result' na expressão 'result + i' é igual a nullptr. O valor resultante não faz sentido e não deve ser usado. 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;
    }
}

O problema é que o resultado não mudou desde a inicialização. O ponteiro resultante não fará sentido, não pode ser usado.

Aviso N3, N4, N5

V778 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'm_colorChannel' deva ser usada em vez 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 o programador copiou duas condições. O segundo deve ficar assim:

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

Outro erro muito semelhante também foi encontrado:

V524 É estranho que o corpo da função 'EnabledAxis1' seja totalmente equivalente ao corpo da função 'EnabledAxis0'. dCustomDoubleHingeActuator.cpp 88

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

Aqui o código deve ser corrigido assim:

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

Outra cópia-pasta:

V525 O código contém a coleção de blocos semelhantes. Verifique os itens 'm_x', 'm_y', 'm_y' nas linhas 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);  <=
  ....
  }
  ....
}

Provavelmente, a inicialização da variável z deve ficar assim:

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

Avisos N6, N7

No Newton Game Dynamics, como em quase qualquer projeto grande de C ou C ++, havia alguns avisos sobre trabalho inseguro com ponteiros. Esses erros geralmente são muito difíceis de encontrar e depurar; eles causam falhas no programa; em geral, são muito perigosos e imprevisíveis. Felizmente, nosso analisador é capaz de detectar muitos desses erros. Parece óbvio que é melhor escrever um cheque uma vez e não tomar banho de vapor, do que gastar muito tempo reproduzindo o problema, localizando o problema no código e depurando-o. Aqui estão alguns dos avisos:

V522 Pode haver desreferenciação de um potencial ponteiro nulo 'face'. dgContactSolver.cpp 351

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

A definição da função NewFace é pequena, então eu darei por completo:

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

Uma das funções dos pontos de saída NewFace retorna NULL , se ocorrer a desreferenciação de ponteiro nulo retornada e haverá um comportamento indefinido do programa.

Também há um aviso semelhante a um trecho de código mais perigoso:

V522 Pode haver desreferenciamento de um potencial 'nulo' de ponteiro nulo. dgPolyhedra.cpp 2541

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

Aqui está a definição 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;
  }
}

Aqui, NULL é o valor de retorno em dois pontos dos três possíveis pontos de saída da função.

No total, recebi 48 avisos do V522 . Na maioria das vezes, eles são do mesmo tipo, por isso não vejo nenhum motivo para descrever mais na estrutura deste artigo.

Aviso N8

V668 Não há sentido em testar o ponteiro 'pBits' contra nulo, pois a memória foi alocada usando o operador 'novo'. A exceção será gerada no caso de erro de alocação de memória. TargaToOpenGl.cpp 166

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

O analisador detectou uma situação em que o valor do ponteiro retornado pelo novo operador é comparado a zero. Como regra, isso significa que o programa, se for impossível alocar memória, não se comportará como o programador espera. Se o novo operador não puder alocar memória, de acordo com o padrão da linguagem C ++ , será lançada uma exceção std :: bad_alloc (). Assim, a condição nunca será cumprida. Esse não é claramente o comportamento com o qual o programador estava contando. Ele planejava fechar o arquivo em caso de erro de alocação de memória. Isso não acontecerá e ocorrerá um vazamento de recursos.

Avisos N9, N10, N11

  • V764 Possível ordem incorreta de argumentos transmitida para a função 'CreateWheel': 'height' e 'radius'. StandardJoints.cpp 791
  • V764 Possível ordem incorreta de argumentos transmitida para a função 'CreateWheel': 'height' e 'radius'. StandardJoints.cpp 833
  • V764 Possível ordem incorreta de argumentos transmitida para a função 'CreateWheel': 'height' e 'radius'. StandardJoints.cpp 884

É assim que as chamadas de função são exibidas:

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

É assim que a declaração da função se parece:

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

Esse diagnóstico sugere que, ao chamar funções, talvez os argumentos tenham sido misturados.

Avisos N12, N13

O analisador emitiu avisos para dois métodos semelhantes com nomes diferentes:

V621 Considere inspecionar o operador 'for'. É possível que o loop seja executado incorretamente ou não seja executado. dgCollisionUserMesh.cpp 161

V621 Considere inspecionar o operador 'for'. É possível que o loop seja executado incorretamente ou não seja executado. 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++)
  {
    ....
  }
  ....
}

O problema nesta parte da condição é: i <dados-> m_faceCount. Como data-> m_faceCount é atribuído como 0, esse loop não será executado nem uma vez. Provavelmente, ao escrever esse trecho de código, o programador esqueceu de reinicializar o campo m_faceCount e, em seguida, simplesmente copiou o corpo do método.

Aviso N14, N15

O analisador emitiu dois avisos para linhas de código semelhantes em sucessão:

V630 A função '_alloca' é usada para alocar memória para uma matriz de objetos que são classes que contêm construtores. dgSkeletonContainer.cpp 1341

V630 A função '_alloca' é usada para alocar memória para uma matriz de objetos que são classes que contêm construtores. 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);

O problema com esse trecho de código é que eles trabalham com memória alocada como uma matriz de objetos que possuem um construtor ou destruidor. Com essa alocação de memória para a classe, o construtor não será chamado. Ao liberar memória, o destruidor não será chamado. Isso é extremamente suspeito. Esse código pode levar ao trabalho com variáveis ​​não inicializadas e outros erros. Além disso, comparado com a abordagem usando malloc / free , esse código é ruim, pois se você tentar alocar mais memória do que a máquina pode fornecer, você não receberá uma mensagem de erro clara. Em vez disso, você recebe um erro de segmentação ao tentar acessar esta memória. Aqui estão mais algumas mensagens do analisador:
  • 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 .


Em geral, o PVS-Studio falhou novamente, durante a verificação, novos erros interessantes foram descobertos. E isso significa que o analisador faz seu trabalho bem, permitindo que tornemos o mundo ao nosso redor um pouco mais perfeito. Para experimentar o analisador estático PVS-Studio em seu projeto, você pode clicar no link .



Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Vladislav Stolyarov. Uma segunda verificação da Newton Game Dynamics com PVS-Studio .

More articles:


All Articles