The PVS-Studio developers' team has carried out comparison of the own static code analyzer PVS-Studio with the open-source Cppcheck static code analyzer. As a material for comparison, the source codes of the three open-source projects by id Software were chosen: Doom 3, Quake 3: Arena, Wolfenstein: Enemy Territory. The article describes the comparison methodology and lists of detected errors. The conclusions section at the end of the article contains "non-conclusions" actually, as we consciously avoid drawing any conclusions: you can reproduce our comparison and draw your own ones.

Introduction

The task of comparing static code analyzers is very hard and unrewarding. First of all, because you have to work out a comparison methodology, get access to the tools and have a good error sample base (and these samples should be real ones, not synthetic). Besides, comparison should be performed in two separate categories: diagnostic capabilities and usability. A really good 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 to 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.

Addressing the Cppcheck's authors

The authors and contributors of Cppcheck, and especially Daniel Marjamäki, are tough guys doing good and useful work. Their tool is worth considering and they themselves are good guys, especially given that their product is absolutely free, which means that everyone interested in static code analysis can easily study the tool.

Our business is a static analysis tool of C/C++ code called PVS-Studio. Since it has happened that we are rivals, I have to answer questions from our users: "Did you compare PVS-Studio to Cppcheck?" I have to choose one of the following versions of the answer to this question:

You see, it's difficult to compare static code analysis tools because... (then go to the text of the "Introduction" section);

You can download and compare them yourself;

"We" or "they" cannot be the best for everyone. They can be the best only for somebody.

Anyway, although all these answers are correct, they don't satisfy people. That's why we have to carry out comparison of PVS-Studio and Cppcheck and publish it so that we can have a more concrete answer for our users.

This article DOES NOT has the goal to show that certain functions of one product are good and others functions are bad. It is simply one of the ways to compare analyzers so that readers can choose a tool most suitable for them BY THEMSELVES.

Comparison methodology

To compare PVS-Studio 4.61 and Cppcheck 1.54 we took the source codes of the three projects by id Software from GitHub: Doom 3, Quake 3: Arena, Wolfenstein: Enemy Territory. We ran the both analyzers on them and got lists of errors. Then we selected those errors that were real. We DID not select poorly written code, possibly incorrect constructs, etc. - only evident errors.

Our methodology has disadvantages:

The set of files to be analyzed appeared to be a bit different. The reason was that Cppcheck analyzed the whole folder with the files, while PVS-Studio analyzed only those files that were included into the project file .vcproj.

Because the lists of detected issues were reviewed by a human, some errors could have been missed. We didn't have the goal to show that one of the tools had found more errors than the other, but still it is probable.

Thus, those who are interested in REALLY comparing PVS-Studio and Cppcheck are welcome to download the tools, the source codes and perform the comparison themselves. The trial version of PVS-Studio will be quite sufficient to find and review all the errors. And Cppcheck is free at all. This is the only way for you to know which of the two tools is more suitable personally for you.

Errors detected in Doom3 by Cppcheck

Fragment 1

..\..\[Build]\Doom3\id-Software-DOOM-3-a9c49da

eo\idlib\hashing\MD5.cpp(252):

Using size of pointer ctx instead of size of its data.

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

sizeof(*ctx) should be written here. The way it's put originally means that the pointer size is passed and the object is zeroed incompletely.

Fragment 2

..\..\[Build]\Doom3\id-Software-DOOM-3-a9c49da

eo\renderer\Image_init.cpp(2214)

Mismatching allocation and deallocation: sortIndex

void idImageManager::PrintMemInfo( MemInfo_t *mi ) { int *sortIndex; ... sortIndex = new int[images.Num()]; ... delete sortIndex;

The source of the error is this: memory is allocated as for an array, while it is released as for one item. In this particular case (for int) this is not a problem, but if an array of objects with a destructor were created, only one destructor would be called, not all of them. The correct code is delete [] sortIndex.

Fragment 3

..\..\[Build]\Doom3\id-Software-DOOM-3-a9c49da

eo\renderer\MegaTexture.cpp(542)

Using size of pointer newBlock instead of size of its data.

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

The same error as in Fragment 1 - sizeof(*newBlock)) should be written;

Fragment 4

..\..\[Build]\Doom3\id-Software-DOOM-3-a9c49da

eo\sys\win32\win_shared.cpp(177)

memset() called to fill 0 bytes of '&'

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

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

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 Cppcheck: 4.

Errors detected by PVS-Studio: 17.

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

Note that the set of analyzed files may not be absolutely identical.

Errors detected in Quake 3: Arena by Cppcheck

Fragment 1

..\..\[Build]\Quake3\id-Software-Quake-III-Arena-dbe4ddb\code\q3_ui\ui_servers2.c 580

Using sizeof with a numeric constant as function argument might not be what you intended.

static void ArenaServers_Remove( void ) { ... memcpy( &g_arenaservers.favoriteaddresses[i], &g_arenaservers.favoriteaddresses[i+1], (g_arenaservers.numfavoriteaddresses - i - 1)* sizeof(MAX_ADDRESSLENGTH));

A strange expression sizeof(MAX_ADDRESSLENGTH) is used in this code. It will always be the size of this variable's type, not its value. Perhaps there should be just MAX_ADDRESSLENGTH without sizeof().

Fragment 2

..\..\[Build]\Quake3\id-Software-Quake-III-Arena-dbe4ddb\code\qcommon\files.c 549

Memory leak: buf

static void FS_CopyFile( char *fromOSPath, char *toOSPath ) { ... byte *buf; ... buf = malloc( len ); if (fread( buf, 1, len, f ) != len) Com_Error( ERR_FATAL, "Short read in FS_Copyfiles()

" ); fclose( f ); if( FS_CreatePath( toOSPath ) ) { return; } ... }

It's quite possible here that memory allocated for buf will remain unreleased. This is a typical example showing what for smart pointers were invented in C++.

Fragment 3

..\..\[Build]\Quake3\id-Software-Quake-III-Arena-dbe4ddb\code\renderer\tr_shade_calc.c 628

Array 'invModulate[3]' index 3 out of bounds

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 missed the mark with the array size and item number.

Fragment 4

..\..\[Build]\Quake3\id-Software-Quake-III-Arena-dbe4ddb\code\server\sv_rankings.c 947

Assert statement modifies 'j'.

assert( (j++) < 68 );

This is an odd fragment, as assert usually is absent in the release-build. Thus, we cannot understand whether j++ should be OUTSIDE assert or this code is only for the debug-version indeed.

Fragment 5

..\..\[Build]\Quake3\id-Software-Quake-III-Arena-dbe4ddb\code\splines\math_matrix.h 87

Using sizeof for array given as function argument returns the size of pointer.

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. The src variable is just a pointer.

Fragment 6

..\..\[Build]\Quake3\id-Software-Quake-III-Arena-dbe4ddb\lcc\src\2html.c 131

printf format string has 2 parameters but 3 are given

static void do_uid(int x) { printf("<a href='#%d'>%d</a>", x, x, x); }

printf prints only two numbers, while there are three parameters being passed. It's either an odd parameter being passed or the programmer forgot to print it.

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 Cppcheck: 6.

Errors detected by PVS-Studio: 7.

Intersecting errors among them (detected both by Cppcheck and PVS-Studio): 2.

Note that the set of analyzed files may not be absolutely identical.

Errors detected in Wolfenstein: Enemy Territory by Cppcheck

Fragment 1

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\curl-7.12.2\docs\examples\sepheaders.c 76

Resource leak: bodyfile bodyfile = fopen( bodyfilename,"w" ); ... // no fclose for bodyfile

Here we have a classical resource leak - the file is opened but cannot be closed. Of course, this code is located in the examples file, which justifies it. But the leak is still here.

Fragment 2

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\curl-7.12.2\src\main.c 3765

Undefined behavior: variable is used as parameter and destination in s[n]printf().

sprintf( dirbuildup,"%s%s%s",dirbuildup, DIR_CHAR, tempdir );

The string is printed into itself. It may cause issues in most cases.

Fragment 3

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\game\bg_animation.c 585

Using sizeof for array given as function argument returns the size of pointer.

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 4

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\game\bg_animation.c 776

Using size of pointer command instead of size of its data.

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 5

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\qcommon\cvar.c 905

Using size of pointer var instead of size of its data.

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

Again and again the pointer size is used instead of the object size.

Fragment 6

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\splines\math_matrix.h 94

Using sizeof for array given as function argument returns the size of pointer.

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 7

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\game\bg_pmove.c 4097

Redundant assignment of "fwdmove_knockback" in switch 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.

Fragment 8

..\..\[Build]\Enemy Territory\id-Software-Enemy-Territory-40342a9\src\game\q_math.c 422

Array 'pnt[3]' index 3 out of bounds 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.

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 Cppcheck: 8.

Errors detected by PVS-Studio: 17.

Intersecting errors among them (detected both by Cppcheck and PVS-Studio): 6.

Note that the set of analyzed files may not be absolutely identical.

Total table of comparison results

Doom 3

Errors detected by Cppcheck: 4.

Errors detected by PVS-Studio: 17.

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

Quake 3: Arena

Errors detected by Cppcheck: 6.

Errors detected by PVS-Studio: 7.

Intersecting errors among them (detected both by Cppcheck and PVS-Studio): 2.

Wolfenstein: Enemy Territory

Errors detected by Cppcheck: 8.

Errors detected by PVS-Studio: 17.

Intersecting errors among them (detected both by Cppcheck and PVS-Studio): 6.

Note that the set of analyzed files may not be absolutely identical.

"Non-conclusions"

I don't want to draw any conclusions from this comparison's results. They don't mean that one of the tools is better. We just ran the two analyzers and found these errors - that's all. Everyone makes conclusions on his/her own. And you'd better check the projects by yourself before that. Perhaps we have even missed something. What for is this article, then? Well, now I just have the answer to the users' question: "Did you compare your tool with Cppcheck?".

References