Andrey Karpov

Feb 06 2012

Tags:

#Cpp #GameDev

Analyzing the Quake III Arena GPL project

Feb 06 2012

Author: Andrey Karpov

As you know, the id Software company has laid out source codes of many of their games. We already checked some of these projects earlier. This time we decided to analyze the Quake III Arena GPL source code. Analysis was performed with PVS-Studio 4.54.

Unfortunately, the post about the check appeared to be bare and without detailed comments. We haven't found such errors in Quake III Arena GPL as to write an interesting article about. Moreover, some of the found errors we have already seen when checking the Doom 3 game's code.

Below we will cite code fragments with various errors and corresponding diagnostic messages that helped us to find them. As usually, we want to note that these are far not all the errors the PVS-Studio analyzer can detect in this project:

If an error is found several times, only one case is described.
The article does not contain descriptions of unessential errors.
We do not cite code fragments about which we cannot decide quickly whether or not there is an error.

Readers can study diagnostic messages generated by PVS-Studio themselves if they wish. The new trial mode allows you to do that easily.

Fragment N1.

Diagnostic message 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 ) );
}

Correct code:

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

Fragment N2.

Diagnostic message 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;
  }
  ...
}

Fragment N3.

Diagnostic message 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);
  ...
}

Fragment N4.

Diagnostic message 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];
  ...
}

Fragment N5.

Diagnostic message 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 ) );
}

Correct code:

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

Fragment N6.

Diagnostic message 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;
  }
  ...
}

Fragment N7.

Diagnostic message 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';
  ...
}

Fragment N8.

Diagnostic message 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 ) );
  ...
}

Correct code:

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

Fragment N9.

Diagnostic message 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];
  ...  
}

Fragment N10.

Diagnostic message 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);
  ...
}

Correct code:

Q1_MAX_MAP_EDGES * sizeof(q1_dedge_t);

Fragment N11.

Diagnostic message 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");
  }
  ...
}

Fragment N12.

Diagnostic message 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)
  {
  ...
}

Fragment N13.

Diagnostic message 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();
  ...
}

Fragment N14.

Diagnostic message 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) );
  ...
}
Correct code:
memset( pBuffer, 0, (nLen+1) * sizeof(char) );

Fragment N15.

Diagnostic message 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;
  ...
}

Fragment N16.

Diagnostic message 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;
  ...
}

Correct code:

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

Fragment N17.

Diagnostic message 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;
  }
  ...
}

Fragment N18.

Diagnostic message V591.

Non-void function should return a value.

static ID_INLINE int BigLong(int l)
{ LongSwap(l); }
Correct code:
static ID_INLINE int BigLong(int l)
{ return LongSwap(l); }

Conclusion

It took me about three hours to find all these errors. This time includes downloading the source codes, the analysis itself, analyzing the results and copying out the most interesting code fragments. As you see, a static analyzer is a very efficient tool of error search. However, it is even more efficient when used regularly.

#Cpp #GameDev