Андрей Карпов

06 Фев 2012

Теги:

#Cpp #GameDev

Проверка проекта Quake III Arena GPL

06 Фев 2012

Автор: Андрей Карпов

Как известно, компания id Software выложила исходные коды многих своих игр. Мы уже проверяли некоторые из этих проектов. На этот раз мы решили проанализировать исходный код Quake III Arena GPL. Для анализа использовался статический анализатор PVS-Studio версии 4.54.

К сожалению, заметка о проверке получилась сухой и без развернутых комментариев. В Quake III Arena GPL мы не нашли ошибок о которых можно написать интересную статью. Вдобавок, некоторые из найденных ошибок мы уже встречали при проверке кода игры Doom 3.

Ниже будут показаны фрагменты кода с различными ошибками и диагностические сообщения, которые помогли их найти. Как всегда отметим, что это далеко не все ошибки, которые может выявить анализатор PVS-Studio в этом проекте:

Если ошибка встречается несколько раз, то описывается только один случай.
В статье не описаны несущественные ошибки.
Не приводятся фрагменты кода, где автор затрудняется быстро сделать вывод, имеется там ошибка или нет.

Читатель при желании может сам более внимательно изучить диагностические сообщения, выдаваемые PVS-Studio. Новая модель триала позволяет это легко сделать.

Фрагмент N1.

Диагностическое сообщение V511.

The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof(src)' expression.

ID_INLINE mat3_t::mat3_t( float src[ 3 ][ 3 ] ) {
  memcpy( mat, src, sizeof( src ) );
}

Должно быть:

memcpy( mat, src, sizeof(float) * 3 * 3);

Фрагмент N2.

Диагностическое сообщение V501.

There are identical sub-expressions '(result->flags & 64)' to the left and to the right of the '||' operator.

void BotMoveToGoal(....)
{
  ...
  if ((result->flags & MOVERESULT_ONTOPOF_FUNCBOB) ||
      (result->flags & MOVERESULT_ONTOPOF_FUNCBOB))
  {
    ms->reachability_time = AAS_Time() + 5;
  }
  ...
}

Фрагмент N3.

Диагностическое сообщение V510.

The 'ScriptError' function is not expected to receive class-type variable as third actual argument.

typedef struct punctuation_s
{
  char *p;
  int n;
  struct punctuation_s *next;
} punctuation_t;

punctuation_t *punctuations;

int PS_ExpectTokenType(script_t *script, ....)
{
  ...
  ScriptError(script, "expected %s, found %s",
    script->punctuations[subtype], token->string);
  ...
}

Фрагмент N4.

Диагностическое сообщение V570.

The 'p->org[0]' variable is assigned to itself.

void CG_ParticleSnowFlurry (qhandle_t pshader, centity_t *cent)
{
  ...
  p->org[0] = p->org[0];
  p->org[1] = p->org[1];
  p->org[2] = p->org[2];
  ...
}

Фрагмент N5.

Диагностическое сообщение V568.

It's odd that the argument of sizeof() operator is the '& itemInfo' expression.

void CG_RegisterItemVisuals( int itemNum ) {
  itemInfo_t  *itemInfo;
  ...
  memset( itemInfo, 0, sizeof( &itemInfo ) );
}

Должно быть:

memset( itemInfo, 0, sizeof( *itemInfo ) );

Фрагмент N6.

Диагностическое сообщение V595.

The 'item' pointer was utilized before it was verified against nullptr. Check lines: 3865, 3869.

void Item_Paint(itemDef_t *item) {
  vec4_t red;
  menuDef_t *parent = (menuDef_t*)item->parent;
  red[0] = red[3] = 1;
  red[1] = red[2] = 0;

  if (item == NULL) {
    return;
  }
  ...
}

Фрагмент N7.

Диагностическое сообщение V557.

Array overrun is possible. The 'sizeof (bs->teamleader)' index is pointing beyond array bound.

typedef struct bot_activategoal_s
{
  ...
  float leadbackup_time;
  char teamleader[32];
  float askteamleader_time;
  ...
} bot_state_t;

void BotTeamAI(bot_state_t *bs) {
  ...
  bs->teamleader[sizeof(bs->teamleader)] = '\0';
  ...
}

Фрагмент N8.

Диагностическое сообщение V579.

The Com_Memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument.

void Cvar_Restart_f( void ) {
  cvar_t *var;
  ...
  // clear the var completely, since we
  // can't remove the index from the list
  Com_Memset( var, 0, sizeof( var ) );
  ...
}

Должно быть:

Com_Memset( var, 0, sizeof( *var ) );

Фрагмент N9.

Диагностическое сообщение V557.

Array overrun is possible. The '3' index is pointing beyond array bound.

void RB_CalcColorFromOneMinusEntity( unsigned char *dstColors )
{
  int *pColors = ( int * ) dstColors;
  unsigned char invModulate[3];
  int c;
  ...
  invModulate[0]=255-backEnd.currentEntity->e.shaderRGBA[0];
  invModulate[1]=255-backEnd.currentEntity->e.shaderRGBA[1];
  invModulate[2]=255-backEnd.currentEntity->e.shaderRGBA[2];
  invModulate[3]=255-backEnd.currentEntity->e.shaderRGBA[3];
  ...  
}

Фрагмент N10.

Диагностическое сообщение V521.

Such expressions using the ',' operator are dangerous. Make sure the expression is correct.

void Q1_AllocMaxBSP(void)
{
  ...
  q1_allocatedbspmem +=
    Q1_MAX_MAP_CLIPNODES * sizeof(q1_dclipnode_t);
  ...
  q1_allocatedbspmem +=
    Q1_MAX_MAP_EDGES , sizeof(q1_dedge_t);
  ...
  q1_allocatedbspmem +=
    Q1_MAX_MAP_MARKSURFACES * sizeof(unsigned short);
  ...
}

Должно быть:

Q1_MAX_MAP_EDGES * sizeof(q1_dedge_t);

Фрагмент N11.

Диагностическое сообщение V595.

The 'node' pointer was utilized before it was verified against nullptr. Check lines: 769, 770.

void FloodPortals_r (node_t *node, int dist)
{
  ...
  if (node->occupied)
    Error("FloodPortals_r: node already occupied\n");
  if (!node)
  {
    Error("FloodPortals_r: NULL node\n");
  }
  ...
}

Фрагмент N12.

Диагностическое сообщение V501.

There are identical sub-expressions 'fabs(dir[1]) > test->radius' to the left and to the right of the '||' operator.

int VL_FindAdjacentSurface(....)
{
  ...
  if (fabs(dir[0]) > test->radius ||
      fabs(dir[1]) > test->radius ||
      fabs(dir[1]) > test->radius)
  {
  ...
}

Фрагмент N13.

Диагностическое сообщение V517.

The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 3333, 3335.

void CMainFrame::OnClipSelected() 
{
  ...
  if (g_bPatchBendMode)
    Patch_BendHandleENTER();
  else if (g_bPatchBendMode)
    Patch_InsDelHandleENTER();
  ...
}

Фрагмент N14.

Диагностическое сообщение V579.

The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument.

void CXYWnd::Paste()
{
  ...
  char* pBuffer = new char[nLen+1];
  memset( pBuffer, 0, sizeof(pBuffer) );
  ...
}

Должно быть:

memset( pBuffer, 0, (nLen+1) * sizeof(char) );

Фрагмент N15.

Диагностическое сообщение V519.

The 'numQuadCels' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1004, 1006.

static void setupQuad( long xOff, long yOff )
{
    ...
  numQuadCels  = (.....);
  numQuadCels += numQuadCels/4 + numQuadCels/16;
  numQuadCels += 64;
  numQuadCels  = (.....);
  numQuadCels += numQuadCels/4;
  numQuadCels += 64;
  ...
}

Фрагмент N16.

Диагностическое сообщение V537.

Consider reviewing the correctness of 'scale_x' item's usage.

void Terrain_AddMovePoint(....) {
  ...
  x = ( v[ 0 ] - p->origin[ 0 ] ) / p->scale_x;
  y = ( v[ 1 ] - p->origin[ 1 ] ) / p->scale_x;
  ...
}

Должно быть:

y = ( v[ 1 ] - p->origin[ 1 ] ) / p->scale_y;

Фрагмент N17.

Диагностическое сообщение V557.

Array overrun is possible. The value of 'i' index could reach 3.

int   numteamVotingClients[2];

void CalculateRanks( void ) {
  ...
  for ( i = 0; i < TEAM_NUM_TEAMS; i++ ) {
    level.numteamVotingClients[i] = 0;
  }
  ...
}

Фрагмент N18.

Диагностическое сообщение V591.

Non-void function should return a value.

static ID_INLINE int BigLong(int l)
{ LongSwap(l); }
Должно быть:
static ID_INLINE int BigLong(int l)
{ return LongSwap(l); }

Заключение

Чтобы найти все эти ошибки мне понадобилось около трех часов времени. В него включено время на скачивание исходных кодов, проверку, анализ результатов и выписывания понравившихся фрагментов кода. Как видите, статический анализатор является очень эффективным инструментом поиска ошибок. Впрочем, ещё более он эффективен при регулярном использовании.

#Cpp #GameDev