Comparing static analysis in Visual Studio 2012 (Visual C++ 2012) and PVS-Studio

After Visual Studio 2012 was released with a new static analysis unit included in all of the product's editions, a natural question arises: "Is PVS-Studio still relevant as a static analysis tool or can it be replaced by the tool integrated into VS?". A detailed answer with examples is given in this article. We have performed interface and usability comparison as well as a comparison of error diagnosis strength in real software code. The comparison was carried out on the source code of three open-source projects by id Software: Doom 3, Quake 3: Arena, Wolfenstein: Enemy Territory.

Introduction

Picture 5

The task of comparing static code analyzers is very hard and unrewarding. First of all, because you have to develop a comparison methodology, have access to the tools and possess a good collection of error samples (and these samples should be real ones, not synthetic). Besides, comparison should be performed in two separate categories: diagnostic capabilities and usability. An ideal comparison of diagnostic capabilities should account for the number of detected errors, the number of undetected errors and the number of false positives, while comparison by the usability criterion can hardly be represented in figures at all. Someone needs a command line version, while someone else wants to have a tool integrated into the development environment (and there are many different ones). Some programmers need a tool that can be used within a team, while others need tools for individual use. And if we go even further and recall various software (Windows, Linux) and hardware (x86, AMR) platforms... Well, to put it briefly, there's a vacancy in this area for an independent company whose job will be to compare static analysis tools in a way similar to companies testing different antiviruses (for example, Austrian AV-Comparatives). Of course, there exists Gartner with their Magic Quadrant for Static Application Security Testing, but it's obviously not enough. All in all, the niche is vacant for now.

Static analysis unit interface and usability in Visual Studio 2012

Visual Studio 2012 includes the support of static analysis for Visual C++ projects (the /analyze compiler flag). Unfortunately, analysis works only with Visual C++ projects using version 11 of the cl compiler. When opening projects of earlier versions, you can only use analysis if you have a compiler version corresponding to such a project with support of /analyze. Let me remind you that Visual Studio 2010 Professional didn't have static analysis support - it was available only in the Premium and Ultimate versions. Besides, x-64 builds could have not been checked at all in any Visual Studio version. This drawback has been eliminated.

The basic advantage of analysis in Visual Studio 2012 version is certainly the appearance of the new separate Code Analysis tool window. In the previous versions, analysis output was directed into the main window Error List which operated rather slowly and also didn't provide functional and interface means specific to code analysis.

The standard Error List window in fact stores results in a common win32 grid, which causes quite noticeable lags starting with 1-2 thousands of displayed messages. The new window uses a dynamic WPF Listbox that should be linked to the internal data structure, while its performance theoretically doesn't depend upon the size of the displayed list. In practice, it's currently hard to perform real testing of its performance because there are no large projects for the new Visual C++ version to be found. By now we could only obtain only about 1000 messages simultaneously, but even with such a small number you can still see the difference between the new and the old windows.

Each list item in the new Code Analysis window is now expands on selection and contains a brief description of an error, while its code is conveniently displayed as a hyperlink to the corresponding article in MSDN. An expanded item can also contain a list of child items allowing you to review several code fragments with brief descriptions and similar warnings to be united into a group. Code navigation is available for all these child items.

Special attention should be given to the default arrangement of the Code Analysis window among all the rest IDE tool windows. In Visual Studio 11 BETA, the window was located at the bottom of the workspace. In our opinion, the new analysis window items were arranged poorly. In Figure 1 you can see how the window was located by default and that it was too stretched out horizontally. It led to inefficient use of the workspace.

Figure 1 — The Code Analysis window arranged horizontally

Figure 1 — The Code Analysis window arranged horizontally

As you can see from the picture, more than half of the workspace is not used at all. At the same time, while the window occupies half of the screen, it displays only 3 messages. If you expand the child list of a selected message (the More Information reference), there will be even fewer messages fit in the window. The impossibility to collapse a message currently selected seems to be quite inconvenient. In the same way, selecting several messages expands each of them. Fortunately, some of these problems can be solved by moving the window to the Solution Explorer's tab group, for example, stretching it vertically, but issues with spontaneous expanding of selected items still remain.

In Visual Studio 2012 RC, the window is located vertically (Figure 2), which is much more logical.

Figure 2 - The Code Analysis window arranged vertically

Figure 2 - The Code Analysis window arranged vertically

The window has a mechanism of quick message filtering (the Search field).This mechanism allows you to filter off all the messages except for those whose items contain the query entered in the search field. Despite this mechanism allowing you to see only messages referring to a certain file or with a certain code, the window doesn't have any capability to sort the current messages, for example by file name and code. There are also no features allowing you to filter off messages with certain codes or referring to certain files. Considering that Visual C++ 2012 sometimes generates warnings for its own system include files, the impossibility to exclude them from the results is a major inconvenience. A feature to filter messages by groups of diagnostics they refer to would also be nice to have. Currently you can filter any individual group of diagnostics only by relaunching the analysis, having changed analysis parameters in the settings preliminarily (Common properties->Code Analysis Settings). What does it mean from a practical viewpoint? If you analyze quite a large project with all the diagnostics turned on (Microsoft All Rules), you'll find it difficult to locate, for example, only Microsoft Security Rules among the results because of the huge "noise" from the rest of the warnings.

Code Analysis has an integrated mechanism to suppress false positives through adding special marks of the #pragma warning(suppress: xxxx) pattern into the project code. Messages marked in that way are crossed out in the output window and are omitted from the results list at the next analysis of the project. Unfortunately, we haven't found any other way to clear the results window of these crossed out messages than to reanalyze the project. Considering the general monochrome outline of the studio's 2012 version, you can find it difficult to handle the remaining messages after marking several items as false positives. Analysis results obtained in the next iteration won't contain the marked messages anymore, so you will be able to find them only by searching through the project code directly. If you have marked something as a false alarm by mistake, you can't reverse that.

Summing up all said above, we can say that despite the indisputable progress in the mechanism of analysis results display in comparison to the previous version that utilized IDE Error List, the new Code Analysis window's functionality is still quite limited and inconvenient for regular use.

In other words, if you are only starting to use static analysis, the native tools integrated into Visual Studio are a good start. But if you use static analysis regularly and constantly, consider looking at dedicated products possessing interface solutions that had been developed through years.

Drawbacks of the static analysis interface in Visual Studio 2012

Let's pose the basic drawbacks:

  • Disabling certain diagnostic rules requires analysis restart (you cannot hide irrelevant messages without relaunching the analysis), which in its turn takes some time too.
  • You cannot save a log-file with the analysis results for further review. When analyzing large projects, the necessity to restart the analysis can be quite a problem.
  • The analyzer sometimes generates warnings on system files which you obviously shouldn't touch.
  • You can't see time remaining till the end of analysis anywhere. This is rather unpleasant when analysis runs for quite a while.

Although all the listed disadvantages might seem irrelevant at first sight, it's rather difficult to use the static analyzer with them.

Methodology of comparing the analyzers' diagnostic capabilities

To compare diagnostic capabilities of PVS-Studio 4.70 and static analysis unit of Visual Studio 2012 (Visual C++ 2012) RC, we took the source code of three projects by id Software from GitHub: Doom 3, Quake 3: Arena, Wolfenstein: Enemy Territory. We ran both analyzers through them and got the warning lists. Then we reviewed both lists and picked out real errors. We DID NOT pick out poorly written code, probably incorrect constructs and the like. We had chosen only the evident errors.

Our comparison methodology has one shortcoming - because it was an individual person who reviewed the result lists, some of the detected errors could have been missed.

Doom 3

Errors detected in Doom 3 with Visual Studio 2012

Fragment 1

C6283 Primitive array-new scalar-delete mismatch. 'testedPlanes' is allocated with array new [], but deleted with scalar delete. brushbsp.cpp 886

testedPlanes = new bool[planeList.Num()];
BuildBrushBSP_r( node, planeList, testedPlanes, skipContents );
delete testedPlanes;

Memory here is allocated for an array but is released as if it were a pointer to one object. We deal with undefined behavior here, so the result is unpredictable. It's not that bad in case of primary types and there hardly will be any failure in this particular code, but in general it is a mistake and you should use delete[].

Fragment 2

C6283 Primitive array-new scalar-delete mismatch. 'sortIndex' is allocated with array new [], but deleted with scalar delete. image_init.cpp 2214

sortIndex = new int[images.Num()];
delete sortIndex;

The same error was described above - delete[] should be used to release memory.

Fragment 3

C6293 Loop counts down from minimum. Ill-defined for-loop: counts down from minimum. model.cpp 2027

for ( maxY = size-1 ; maxY < size ; maxY-- ) {
  for ( i = 0 ; i < size ; i++ ) {
    if ( data[maxY*size + i] > 1.0 ) {
      break;
    }
  }
  if ( i != size ) {
    break;
  }
}

The loop conditions look very odd here. Although this code can be theoretically correct, there is most likely an error here if you look at the code next to this fragment.

Fragment 4

C6269 Pointer dereference ignored. Possibly incorrect order of operations: dereference ignored. model_lwo.cpp 1251

int sgetI1( unsigned char **bp )
{
  ...
  flen += 1;
  *bp++;
  return i;
}

Because of the operation priorities a different algorithm than the programmer intended for will be executed for the dereferencing and increment operations. The correct code is (*bp)++.

This file contains two more similar mistakes which were not included into the report (like in case with PVS-Studio).

Fragment 5

C6283 Primitive array-new scalar-delete mismatch. 'sortIndex' is allocated with array new [], but deleted with scalar delete. modelmanager.cpp 617

sortIndex = new int[ localModelManager.models.Num()];
delete sortIndex;

This error was already described above - delete[] should be used to release memory.

Errors detected in Doom3 by PVS-Studio

Fragment 1

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 524, 533. anim_blend.cpp(524)

const char *idAnim::AddFrameCommand( const idDeclModelDef *modelDef,
        int framenum, idLexer &src, const idDict *def ) {
  ...
  } else if ( token == "muzzle_flash" ) {
    if( !src.ReadTokenOnLine( &token ) ) {
      return "Unexpected end of line";
    }
    ...
    } else if ( token == "muzzle_flash" ) {
      fc.type = FC_MUZZLEFLASH;
      fc.string = new idStr( "" );
      ...

This function contains two identical if branches with different contents. One of them is most likely to contain a misprint.

Fragment 2

V556 The values of different enum types are compared. af.cpp 895

class idDeclAF_Constraint {
  ...
  declAFConstraintType_t type;
  ...
};

constraintType_t GetType( void ) const { return type; }

bool idAF::Load( idEntity *ent, const char *fileName ) {
  ...
  if (
    file->constraints[j]->name.Icmp(
      constraint->GetName() ) == 0 &&
    file->constraints[j]->type == constraint->GetType() )
  {
  ...

In this code fragment, values of different types are compared, i.e. referring to different enums. Although in some individual cases it can work, this is obviously an error.

Fragment 3

V528 It is odd that pointer to 'char' type is compared with the '\0' value.

Probably meant: *classname != '\0'. game_local.cpp 1250

const char *classname = mapEnt->epairs.GetString( "classname" );
if ( classname != '\0' ) {
  FindEntityDef( classname, false );
}

The programmer wanted to check the classname string here to make sure that it's not empty. However, the comparison doesn't work because the pointer needs to be dereferenced.

Fragment 4

V528 It is odd that pointer to 'char' type is compared with the '\0' value.

Probably meant: *soundShaderName != '\0'. game_local.cpp 1619

soundShaderName = dict->GetString( "s_shader" );
if (soundShaderName != '\0' &&
    dict->GetFloat("s_shakes") != 0.0f){
  soundShader = declManager->FindSound( soundShaderName );

The error is identical to Fragment 3 - pointer dereferencing is needed.

Fragment 5

V514 Dividing sizeof a pointer 'sizeof (clientInPVS)' by another value. There is a probability of logical error presence. game_network.cpp 686

void idGameLocal::ServerWriteSnapshot( 
              int clientNum, int sequence, idBitMsg &msg, 
              byte *clientInPVS, int numPVSClients ) {
  ...
  memcpy( clientInPVS, snapshot->pvs,
          ( numPVSClients + 7 ) >> 3 );
  LittleRevBytes( clientInPVS, sizeof( int ), 
                  sizeof( clientInPVS ) / sizeof ( int ) );
}

Here you can track the whole history of this code fragment's life. clientInPVS was once a local array and sizeof(clientInPVS)/sizeof(int) indeed calculated the number of items. But then clientInPVS appeared to be passed as a parameter into a function, while the code remained the same. As a result, the sizeof(clientInPVS)/sizeof(int) value always equals 1 for a 32-bit platform and 2 for a 64-bit platform. To fix it the number of items should be passed directly.

Fragment 6

V599 The destructor was not declared as a virtual one, although the 'BOBrick' class contains virtual functions. gamebustoutwindow.cpp 509

class BOBrick {
  ...
  virtual void WriteToSaveGame( idFile *savefile );
  virtual void ReadFromSaveGame( idFile *savefile,
                                 idGameBustOutWindow *game );
};

BOBrick *paddle;
void idGameBustOutWindow::ReadFromSaveGame( idFile *savefile ) {
  idWindow::ReadFromSaveGame( savefile );
  // Clear out existing paddle and entities from GUI load
  delete paddle;

In this fragment, the class contains virtual functions but doesn't contain a virtual destructor. Though it's not always a problem, you'd better create a virtual destructor all the time in such a case so that the issue doesn't occur in future.

Fragment 7

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1931, 1933. gamessdwindow.cpp 1931

void idGameSSDWindow::FireWeapon(int key) {
...
} else 
  if(gameStats.levelStats.targetEnt->type == SSD_ENTITY_ASTRONAUT) {
    HitAstronaut(static_cast<SSDAstronaut*>(
    gameStats.levelStats.targetEnt), key);
} else 
  if(gameStats.levelStats.targetEnt->type == SSD_ENTITY_ASTRONAUT) {

Again one and the same condition is checked in different code branches. Most likely, it's an unsuccessfully copied-and-pasted code.

Fragment 8

V535 The variable 'i' is being used for this loop and for the outer loop. matrix.cpp 3128

bool idMatX::IsOrthonormal( const float epsilon ) const {
  for ( int i = 0; i < numRows; i++ ) {
    ...
    for ( i = 1; i < numRows; i++ ) {

What is strange about this code, the i loop counter is used both for the outer and inner loops.

Fragment 9

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

void MD5_Final( MD5_CTX *ctx, unsigned char digest[16] ) {
  ...
  memset( ctx, 0, sizeof( ctx ) ); /* In case it's sensitive */

There should be sizeof(*ctx) here. The code written originally passes the pointer size and the object is zeroed incompletely.

Fragment 10

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

typedef struct {
  ...
} aseMesh_t;

aseMesh_t *currentMesh;
...
ase.currentMesh = &ase.currentObject->mesh;
memset( ase.currentMesh, 0, sizeof( ase.currentMesh ) );

It's not the first time we come across this error when a pointer size is passed into the memset function instead of an object size, while these sizes are not always the same.

Fragment 11

V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. model_lwo.cpp 1251

int sgetI1( unsigned char **bp )
{
   ...
   *bp++;

This is a frequent error too - a pointer value is incremented instead of the value of the object the pointer refers to. The correct code is (*bp)++.

This file also contains two similar errors which were not included in the report.

Fragment 12

V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'j'. surface_polytope.cpp 65

void idSurface_Polytope::FromPlanes( 
    const idPlane *planes, const int numPlanes )
{
  for ( j = 0; j < w.GetNumPoints(); j++ ) {
    for ( k = 0; k < verts.Num(); j++ ) {

The inner loop here runs on the k variable, while it is the j variable which is incremented. That's a common side effect of code copy-and-paste.

Fragment 13

V535 The variable 'i' is being used for this loop and for the outer loop. weapon.cpp 2533

const char *idWeapon::GetAmmoNameForNum( ammo_t ammonum )
{
  ...
  for ( i = 0; i < 2; i++ ) {
    ...
    for( i = 0; i < num; i++ ) {

Again one and the same variable is used both for the inner and outer loop counters.

Fragment 14

V575 The 'memset' function processes '0' elements. Inspect the third argument. win_shared.cpp 177

void Sys_GetCurrentMemoryStatus( sysMemoryStats_t &stats ) {
  ...
  memset( &statex, sizeof( statex ), 0 );

The second and the third arguments are swapped by mistake here - memset(&statex, 0, sizeof( statex)) should be written. What is specific about this error, it's very difficult to notice visually.

Fragment 15

V512 A call of the 'memset' function will lead to underflow of the buffer '& cluster'. aasfile.cpp 1312

void idAASFileLocal::DeleteClusters( void ) {
  aasPortal_t portal;
  aasCluster_t cluster;
  ...
  // first portal is a dummy
  memset( &portal, 0, sizeof( portal ) );
  portals.Append( portal );

  // first cluster is a dummy
  memset( &cluster, 0, sizeof( portal ) );
  clusters.Append( cluster );
}

A very nice mistake. Nothing good comes of code copy-and-paste. The programmer forgot to replace sizeof(portal) with sizeof(cluster) in the second block.

Fragment 16

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

void idMegaTexture::GenerateMegaMipMaps(
    megaTextureHeader_t *header, idFile *outFile )
{
  ...
  byte *newBlock = (byte *)_alloca( tileSize );
  ...
  memset( newBlock, 0, sizeof( newBlock ) );

sizeof(*newBlock) should be written here, otherwise the pointer size is used.

Fragment 17

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. target.cpp 257

#define BIT( num ) ( 1 << ( num ) )
const int BUTTON_ATTACK = BIT(0);
void idTarget_WaitForButton::Think( void ) {
idPlayer *player;
...
if ( player && ( !player->oldButtons & BUTTON_ATTACK ) && 
   ( player->usercmd.buttons & BUTTON_ATTACK ) ) {
        player->usercmd.buttons &= ~BUTTON_ATTACK;

An incorrect condition has occurred here because of the priority of the "!" operator (that is higher than that of the "&" operator). The programmer wanted to check that the low-order bit is equal to zero, but instead it is checked whether all the bits are equal to zero.

Summary table of detected errors (quantity) in Doom 3

Errors detected by Visual Studio 2012: 5.

Errors detected by PVS-Studio: 17.

Intersecting errors among them (detected both by Visual Studio 2012 and PVS-Studio): 1.

Quake 3: Arena

Errors detected in Quake 3: Arena with Visual Studio 2012

Fragment 1

C6287 Redundant test. Redundant code: the left and right sub-expressions are identical. be_ai_move.c 3236

if ((result->flags & MOVERESULT_ONTOPOF_FUNCBOB) ||
    (result->flags & MOVERESULT_ONTOPOF_FUNCBOB))

That's a very odd comparison - the programmer must have copied-and-pasted the condition but forgotten to change the value of the flag to be checked.

Fragment 2

C6059 Bad concatenation. Misuse of length parameter in call to 'strncat'. Pass the number of remaining characters, not the buffer size of 'path'. l_precomp.c 1013

#define MAX_TOKEN  1024
typedef struct token_s
{
  char string[MAX_TOKEN]; //available token
  ...
}
#define MAX_PATH 64
char path[MAX_PATH];
strncat(path, token.string, MAX_PATH);

Incorrect call of strncat. You should pass the number of remaining characters instead of the 'path' buffer's size (that equals MAX_PATH) to this function.

Fragment 2

C6201 Index exceeds stack buffer maximum. Index '32' is out of valid index range '0' to '31' for possibly stack allocated buffer 'bs->teamleader'. ai_cmd.c 1311

...
char teamleader[32];   //netname of the team leader
...
bs->teamleader[sizeof(bs->teamleader)] = '\0';

Missing the array: sizeof() - 1 should have been written.

Fragment 3

C6326 Constant constant comparison. Potential comparison of a constant with another constant. ai_dmq3.c 2513

if ((bs->inventory[INVENTORY_ROCKETLAUNCHER] <= 0 || 
     bs->inventory[INVENTORY_ROCKETS < 10]) &&
    (bs->inventory[INVENTORY_RAILGUN] <= 0 || 
     bs->inventory[INVENTORY_SLUGS] < 10) &&
    (bs->inventory[INVENTORY_BFG10K] <= 0 ||
     bs->inventory[INVENTORY_BFGAMMO] < 10)) {
        return qfalse;
}

The way the error in this fragment was detected is not quite according to the rule formulation. The message tells us that it's an odd thing to compare constants. But the error here is this: a closing parenthesis stands after comparison, not after the array index. So the result becomes an index. That is, this code:

inventory[INVENTORY_ROCKETS < 10]

must be replaced with this:

inventory[INVENTORY_ROCKETS] < 10

Fragment 4

C6200 Index exceeds buffer maximum. Index '3' is out of valid index range '0' to '1' for non-stack buffer 'level.numteamVotingClients'. g_main.c 776

typedef enum {
  TEAM_FREE,
  TEAM_RED,
  TEAM_BLUE,
  TEAM_SPECTATOR,
  TEAM_NUM_TEAMS // = 4
} team_t;
...
int numteamVotingClients[2];// set by CalculateRanks
...
for ( i = 0; i < TEAM_NUM_TEAMS; i++ ) {
  level.numteamVotingClients[i] = 0;
}

A trivial mistake with an index exceeding the array boundaries. The loop goes up to 4, while the array consists of only two items.

Fragment 5

C6059 Bad concatenation. Misuse of length parameter in call to 'strncat'. Pass the number of remaining characters, not the buffer size of 'info'. cl_main.c 2609

strncat(info, "\n", sizeof(info));

Again, incorrect use of the strncat() function.

Fragment 6

C6201 Index exceeds stack buffer maximum. Index '3' is out of valid index range '0' to '2' for possibly stack allocated buffer 'invModulate'. tr_shade_calc.c 628

unsigned char invModulate[3];
// this trashes alpha, but the AGEN block fixes it
invModulate[3] = 255 - backEnd.currentEntity->e.shaderRGBA[3];

An index is outside of the array boundaries.

Errors detected in Quake 3: Arena by PVS-Studio

Fragment 1

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

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

It's simply impossible to calculate the array size using sizeof in this case, and the matrix will be copied incompletely.

Fragment 2

V523 The 'then' statement is equivalent to the 'else' statement. be_aas_sample.c 864

int AAS_TraceAreas(vec3_t start, vec3_t end, int *areas, 
                   vec3_t *points, int maxareas)
{
  ...
  if (front < 0) 
    frac = (front)/(front-back);
  else 
    frac = (front)/(front-back);

The frac variable is calculated identically, though there is a condition being checked before it. The variable should be probably calculated differently.

Fragment 3

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

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

The third argument of memset is the pointer size, not the object size.

Fragment 4

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

char teamleader[32];  //netname of the team leader

void BotMatch_StartTeamLeaderShip(
  bot_state_t *bs, bot_match_t *match)
{
  ...
  bs->teamleader[sizeof(bs->teamleader)] = '\0';

Missing the array. sizeof() - 1 should have been written.

Fragment 5

V557 Array overrun is possible. The value of 'i' index could reach 3. g_main.c 776

int numteamVotingClients[2];// set by CalculateRanks
typedef enum {
  TEAM_FREE,
  TEAM_RED,
  TEAM_BLUE,
  TEAM_SPECTATOR,
  TEAM_NUM_TEAMS
} team_t;

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

The array consists of only two items, while the enum values used as a counter are obviously larger. This naturally causes an array overrun.

Fragment 6

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

void Cvar_Restart_f( void ) {
  ...
  cvar_t  *var;
  ...
  Com_Memset( var, 0, sizeof( var ) );

Again it's the pointer size instead of the object size being passed. The correct code is sizeof(*var).

Fragment 7

V557 Array overrun is possible. The '3' index is pointing beyond array bound. tr_shade_calc.c 628

void RB_CalcColorFromOneMinusEntity( unsigned char *dstColors )
{
  ...
  unsigned char invModulate[3];
  ...
  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];
  // this trashes alpha, but the AGEN block fixes it

Missing the array because there are 3 items, not 4.

Summary table of detected errors (quantity) in Quake 3: Arena

Errors detected by Visual Studio 2012: 6.

Errors detected by PVS-Studio: 7.

Intersecting errors among them (detected both by Visual Studio 2012 and PVS-Studio): 3.

Wolfenstein: Enemy Territory

Errors detected in Wolfenstein: Enemy Territory with Visual Studio 2012

Fragment 1

C6287 Redundant test. Redundant code: the left and right sub-expressions are identical. be_ai_move.c 3572

if ((result->flags & MOVERESULT_ONTOPOF_FUNCBOB) ||
    (result->flags & MOVERESULT_ONTOPOF_FUNCBOB))

Here we have a very strange comparison - the programmer must have copied-and-pasted the condition but forgotten to change the value of the flag to be checked.

Fragment 2

C6059 Bad concatenation. Misuse of length parameter in call to 'strncat'. Pass the number of remaining characters, not the buffer size of 'path'. l_precomp.c 1013

strncat(path, token.string, _MAX_PATH );

Incorrect call of strncat().

Fragment 3

C6290 Logical-NOT bitwise-AND precedence. Bitwise operation on logical result: ! has higher precedence than &. Use && or (!(x & y)) instead. bg_pmove.c 3257

if ( !pm->ps->pm_flags & PMF_LIMBO ) {

An issue with incorrect operation priorities. That is, not the way the programmer expected. Or maybe quite that very way he expected. Anyway, the programmer should have specified it more precisely through parentheses which algorithm exactly he wanted to create.

Fragment 4

C6289 Mutual exclusion over logical-OR is true. Incorrect operator: mutual exclusion over || is always a non-zero constant. Did you intend to use && instead? cg_predict.c 679

if ( ps1->groundEntityNum != ENTITYNUM_WORLD || 
     ps1->groundEntityNum != ENTITYNUM_NONE || 
     ps2->groundEntityNum != ENTITYNUM_WORLD || 
     ps2->groundEntityNum != ENTITYNUM_NONE ) {
        return qfalse;
}

The programmer must have intended to use the && operator here but the logical expression turned out to be incorrect either because of the programmer mixing up things or due to some other reason.

Fragment 5

C6201 Index exceeds stack buffer maximum. Index '32' is out of valid index range '0' to '31' for possibly stack allocated buffer 'bs->teamleader'. ai_cmd.c 1037

...
char teamleader[32];   //netname of the team leader
...
bs->teamleader[sizeof(bs->teamleader)] = '\0';

Missing the array: sizeof() - 1 should have been written.

Fragment 6

C6290 Logical-NOT bitwise-AND precedence. Bitwise operation on logical result: ! has higher precedence than &. Use && or (!(x & y)) instead. ai_dmq3.c 5479

if ( !g_entities[client].r.svFlags & SVF_BOT ) {

Here is again an issue with operation priorities which doesn't allow us to figure out from the code whether or not the result is what the programmer intended.

Fragment 7

C6387 Invalid parameter value. 'params' could be '0': this does not adhere to the specification for the function 'atoi'. ai_script_actions.c 477

qboolean Bot_ScriptAction_Wait( bot_state_t *bs, char *params ) {
if ( !params || !params[0] ) {
  Bot_ScriptError( bs, "Wait requires a duration." );
}

The first check seems to be working when params == null, but the program will crash with the second check.

Fragment 8

C6201 Index exceeds stack buffer maximum. Index '3' is out of valid index range '0' to '2' for possibly stack allocated buffer 'invModulate'. tr_shade_calc.c 679

unsigned char invModulate[3];
// this trashes alpha, but the AGEN block fixes it
invModulate[3] = 255 - backEnd.currentEntity->e.shaderRGBA[3];

An index exceeds the array boundaries.

Fragment 9

C6059 Bad concatenation. Misuse of length parameter in call to 'strncat'. Pass the number of remaining characters, not the buffer size of 'info'. cl_main.c 3791

strncat(info, "\n", sizeof(info));

Again, incorrect use of the strncat() function.

Errors detected in Wolfenstein: Enemy Territory by PVS-Studio

Fragment 1

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

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

It's simply impossible to calculate the array size using sizeof in this case, and the matrix will be copied incompletely.

Fragment 2

V511 The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof (result)' expression. bg_animation.c 585

void BG_ParseConditionBits( char **text_pp, 
    animStringItem_t *stringTable, int condIndex, int result[2] ) {
  ...
  memset( result, 0, sizeof( result ) );

One of the function's arguments is an array. The programmer tried to calculate its size with sizeof(), but the correct way is either to pass the size (which is more correct) or strictly define the size "2", since it is written in the code anyway.

Fragment 3

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

static void BG_ParseCommands( char **input, 
  animScriptItem_t *scriptItem, animModelInfo_t *animModelInfo,
  animScriptData_t *scriptData )
{
  // TTimo gcc: might be used uninitialized
  animScriptCommand_t *command = NULL;
  ...
  memset( command, 0, sizeof( command ) );

The pointer size is calculated instead of the object size here.

Fragment 4

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. bg_pmove.c 3257

static void PM_Weapon( void ) {
  ...
  if ( !pm->ps->pm_flags & PMF_LIMBO ) {
    PM_CoolWeapons();
  }

Mixing up operations' priorities causes the expression to be calculated in a different way than expected.

Fragment 5

V523 The 'then' statement is equivalent to the 'else' statement. bg_pmove.c 4115

static void PM_Weapon( void ) {
  ...
  if ( DotProduct( pml.forward, pm->ps->velocity ) > 0  )
  {
    VectorScale( pml.forward, -1.f * ( fwdmove_knockback / mass ), 
                 kvel );    // -1 as we get knocked backwards
  } else {
    VectorScale( pml.forward, -1.f * ( fwdmove_knockback / mass ),
                 kvel );    // -1 as we get knocked backwards
  }

Regardless the condition, the same code branch is executed. There should be probably another branch.

Fragment 6

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

static qboolean CG_CheckForExistingAnimModelInfo(
    const char *animationGroup, const char *animationScript,
    animModelInfo_t **animModelInfo ) {
  ...
  memset( *animModelInfo, 0, sizeof( *animModelInfo ) );

The pointer size is calculated instead of the object size, as a pointer to the pointer is passed into the function.

Fragment 7

V519 The 'backColor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3180, 3181. cg_draw.c 3181

typedef vec_t vec4_t[4];
static void CG_DrawObjectiveInfo( void ) {
  ...
  vec4_t backColor;
  backColor[0] = 0.2f;
  backColor[1] = 0.2f;
  backColor[2] = 0.2f;
  backColor[2] = 1.f;

A value is written into the third item twice, instead of the fourth item.

Fragment 8

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. cg_newdraw.c 720

typedef enum {qfalse, qtrue}    qboolean;
qboolean eventHandling;
void CG_MouseEvent( int x, int y ) {
  switch ( cgs.eventHandling ) {
  case CGAME_EVENT_SPEAKEREDITOR:
  case CGAME_EVENT_GAMEVIEW:
  case CGAME_EVENT_CAMPAIGNBREIFING:
  case CGAME_EVENT_FIRETEAMMSG:

In switch and case different enums are used.

Fragment 9

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

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

The third argument of memset is the pointer size instead of the object size.

Fragment 10

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

typedef vec_t vec3_t[3];
void RotatePointAroundVertex( vec3_t pnt, float rot_x,
  float rot_y, float rot_z, const vec3_t origin ) {
  ...
  // rotate point
  pnt[0] = ( tmp[3] * ( tmp[8] - tmp[9] ) + pnt[3] * tmp[2] );

Accessing pnt[3] causes an array miss.

Fragment 11

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

char teamleader[32]; //netname of the team leader
...
bs->teamleader[sizeof( bs->teamleader )] = '\0';

Missing the array. sizeof() - 1 should have been written.

Fragment 12

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. ai_dmq3.c

if ( !g_entities[client].r.svFlags & SVF_BOT ) {
  return;
}

Mixing up operations' priorities causes the expression to be calculated in a different way than expected.

Fragment 13

V562 It's odd to compare 0 or 1 with a value of 2. ai_main.c 2659

if ( !level.clients[0].pers.connected == CON_CONNECTED ) {
  return;
}

Operations' priorities again change the essence of the expression.

Fragment 14

V557 Array overrun is possible. The value of 'i' index could reach 4. g_systemmsg.c 157

#define NUM_PLAYER_CLASSES      5
void G_CheckForNeededClasses( void ) {
  qboolean playerClasses[NUM_PLAYER_CLASSES - 1][2];
  ...
  for ( i = 0; i < NUM_PLAYER_CLASSES; i++ ) {
    if ( !playerClasses[i][0] ) {
      cnt++;
    }
  }

Access outside the array boundaries.

Fragment 15

V557 Array overrun is possible. The '3' index is pointing beyond array bound. tr_shade_calc.c 679

void RB_CalcColorFromOneMinusEntity( unsigned char *dstColors ) {
  ...
  unsigned char invModulate[3];
  ...
  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];  
  // this trashes alpha, but the AGEN block fixes it

Again the programmer is missing the mark with the array size and the item number.

Fragment 16

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

void Cvar_Restart_f( void ) {
  cvar_t  *var;
  ...
  memset( var, 0, sizeof( var ) );

Again the pointer size is passed instead of the object size. The correct code is sizeof(*var).

Fragment 17

V519 The 'fwdmove_knockback' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 4097, 4098. bg_pmove.c 4098

static void PM_Weapon( void ) {
  ...
  if ( !( pm->ps->eFlags & EF_PRONE ) && (
          pml.groundTrace.surfaceFlags & SURF_SLICK ) ) {
    float fwdmove_knockback = 0.f;
    float bckmove_knockback = 0.f;

    switch ( pm->ps->weapon ) {
    case WP_MOBILE_MG42:    fwdmove_knockback = 4000.f;
        fwdmove_knockback = 400.f;
        break;
    case WP_PANZERFAUST:    fwdmove_knockback = 32000.f;
        bckmove_knockback = 1200.f;
        break;
    case WP_FLAMETHROWER:   fwdmove_knockback = 2000.f;
        bckmove_knockback = 40.f;
        break;
    }

One and the same variable is assigned two values in the WP_MOBILE_MG42 branch.

Summary table of detected errors (quantity) in Wolfenstein: Enemy Territory

Errors detected by Visual Studio 2012: 9.

Errors detected by PVS-Studio: 17.

Intersecting errors among them (detected both by Visual Studio 2012 and PVS-Studio): 4.

Total table of comparison results

Doom 3

Errors detected by Visual Studio 2012: 5.

Errors detected by PVS-Studio: 17.

Intersecting errors among them (detected both by Visual Studio 2012 and PVS-Studio): 1.

Quake 3: Arena

Errors detected by Visual Studio 2012: 6.

Errors detected by PVS-Studio: 7.

Intersecting errors among them (detected both by Visual Studio 2012 and PVS-Studio): 3.

Wolfenstein: Enemy Territory

Errors detected by Visual Studio 2012: 9.

Errors detected by PVS-Studio: 17.

Intersecting errors among them (detected both by Visual Studio 2012 and PVS-Studio): 4.

Conclusions

The guys from Microsoft have done great work in Visual Studio 2012's static analysis unit. They are good fellows and we thank them for making more people aware of what static code analysis is. But we are intensively developing PVS-Studio too. That's why our tool has a competent and convenient interface as well as powerful diagnostic capabilities which we will go on to improve.

References