# Analyzing the Quake III Arena GPL project

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](https://pvs-studio.com/en/blog/posts/cpp/0120/) 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](https://pvs-studio.com/en/blog/posts/0128/) allows you to do that easily\.

**Fragment N1\.**

Diagnostic message [V511](https://pvs-studio.com/en/docs/warnings/v511/)\.

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

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

Correct code:

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

**Fragment N2\.**

Diagnostic message [V501](https://pvs-studio.com/en/docs/warnings/v501/)\.

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

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

**Fragment N3\.**

Diagnostic message [V510](https://pvs-studio.com/en/docs/warnings/v510/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v570/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v568/)\.

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

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

Correct code:

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

**Fragment N6\.**

Diagnostic message [V595](https://pvs-studio.com/en/docs/warnings/v595/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v557/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v579/)\.

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

```cpp
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:

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

**Fragment N9\.** 

Diagnostic message [V557](https://pvs-studio.com/en/docs/warnings/v557/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v521/)\.

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

```cpp
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:

```cpp
Q1_MAX_MAP_EDGES * sizeof(q1_dedge_t);
```

**Fragment N11\.**

Diagnostic message [V595](https://pvs-studio.com/en/docs/warnings/v595/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v501/)\.

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

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

**Fragment N13\.**

Diagnostic message [V517](https://pvs-studio.com/en/docs/warnings/v517/)\.

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

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

**Fragment N14\.**

Diagnostic message [V579](https://pvs-studio.com/en/docs/warnings/v579/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v519/)\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v537/)\.

Consider reviewing the correctness of 'scale\_x' item's usage\.

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

Correct code:

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

**Fragment N17\.**

Diagnostic message [V557](https://pvs-studio.com/en/docs/warnings/v557/)\.

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

```cpp
int   numteamVotingClients[2];

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

**Fragment N18\.**

Diagnostic message [V591](https://pvs-studio.com/en/docs/warnings/v591/)\.

Non\-void function should return a value\.

```cpp
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](https://pvs-studio.com/en/docs/manual/0024/)\.