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.
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.
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:
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.
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:
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.
Fragment 1
..\..\[Build]\Doom3\id-Software-DOOM-3-a9c49da\neo\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\neo\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\neo\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\neo\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.
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.
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.
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()\n" );
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 itAgain 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.
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 itMissing the array because there are 3 items, not 4.
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.
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 bodyfileHere 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.
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 itAgain 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.
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.
Errors detected by Cppcheck: 4.
Errors detected by PVS-Studio: 17.
Intersecting errors among them (detected both by Cppcheck and PVS-Studio): 3.
Errors detected by Cppcheck: 6.
Errors detected by PVS-Studio: 7.
Intersecting errors among them (detected both by Cppcheck and PVS-Studio): 2.
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.
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?".