>
>
Сравнение Cppcheck и PVS-Studio

Евгений Рыжков
Статей: 125

Сравнение Cppcheck и PVS-Studio

Команда разработчиков PVS-Studio сравнила свой статический анализатор кода PVS-Studio с открытым (free, open source) статическим анализатором кода Cppcheck. Для сравнения были выбраны исходные коды трех открытых проектов id Software: Doom 3, Quake 3: Arena, Wolfenstein: Enemy Territory. В статье приведена методика сравнения, списки обнаруженных ошибок. В заключении приведены "не выводы", так как мы сознательно не делаем никаких выводов. Каждый может сам повторить наше сравнение и сделать свои выводы.

Введение

Задача сравнения статических анализаторов кода - очень сложная и неблагодарная. Прежде всего, потому что надо разработать методику сравнения, иметь доступ к инструментам, хорошую базу примеров ошибок (причем лучше реальных, а не синтетических). Кроме этого сравнение вообще стоит делать по двум отдельным категориям: диагностические возможности и удобство использования. Сравнение диагностических возможностей "по-хорошему" должно учитывать количество обнаруженных ошибок, количество необнаруженных ошибок, количество ложных срабатываний. Сравнение удобства использования - вообще трудно оценить в цифрах. Кому-то нужна command line версия, а кому-то интегрированная в среду разработки (а ведь их много разных). Кому-то нужно использование в команде, а кому-то - индивидуальное. А если еще вспомнить про разные программные (Windows, Linux) и аппаратные (x86, AMR) платформы... Короче говоря, здесь вполне вакантно место для независимой некоммерческой организации, которая будет заниматься сравнением инструментов анализа кода аналогично компаниям, тестирующим разные антивирусные продукты (например, австрийская AV-Comparatives). Есть, конечно же, Gartner с их (Magic Quadrant for Static Application Security Testing), но этого явно недостаточно. В общем, ниша свободна.

Обращение к авторам Cppcheck

Авторы и контрибьюторы Cppcheck, а особенно, Daniel Marjamäki - крутые ребята, делающие полезное дело. Их инструмент - достоин внимания, а сами они - молодцы. Тем более что их продукт полностью free, а значит каждый, кто интересуется статическим анализом кода, может очень легко его посмотреть.

Наш бизнес - это инструмент статического анализа C/C++ кода PVS-Studio. И раз уж так получилось, что мы являемся конкурентами, то мне приходится отвечать на вопросы наших пользователей: "А вы сравнивали PVS-Studio с Cppcheck?". На такие вопросы мне приходится отвечать одним из следующих вариантов:

  • Понимаете, сравнить инструменты анализа кода сложно, потому что... (и далее по тексту раздела "Введение");
  • Вы можете сами скачать и сравнить;
  • "Мы" или "они" лучше для всех быть не могут. Они могут быть лучше только для кого-то.

В любом случае все эти ответы хоть и правильные, но не всегда устраивают людей. Поэтому мы вынуждены выполнить сравнение PVS-Studio и Cppcheck, и опубликовать его, чтобы было, что отвечать людям более конкретно.

Эта статья НЕ служит целью показать, что определенные функции одного продукта хороши, а другие плохи. Это просто один из способов сравнить анализаторы, чтобы читатели могли САМИ сделать выбор в пользу наиболее подходящего им инструмента.

Методика сравнения

Для сравнения PVS-Studio 4.61 и Cppcheck 1.54 мы взяли исходный код трех проектов id Software с GitHub: Doom 3, Quake 3: Arena, Wolfenstein: Enemy Territory. Прогнали на нем оба анализатора, получили список ошибок. Затем просмотрели (вручную) весь список. И отобрали те ошибки, которые являются реальными. Мы НЕ отбирали неудачный код, возможно некорректные конструкции и т.п. Только явные ошибки.

У нашего сравнения есть недостатки:

  • Набор анализируемых файлов оказался немного разным. Это произошло из-за того, что Cppcheck анализировал всю папку с файлами, а PVS-Studio - только те файлы, которые включены в файл проекта .vcproj.
  • Из-за того, что списки найденных проблем просматривались человеком, то возможно, что некоторые обнаруженные ошибки были пропущены. Не было цели показать, что один из инструментов нашел больше, чем другой, но все-таки вероятность этого есть.

Таким образом, кому интересно РЕАЛЬНО сравнить PVS-Studio и Cppcheck, мы предлагаем самостоятельно скачать инструменты, скачать исходники и выполнить сравнение. Trial-версии PVS-Studio будет достаточно, чтобы найти и просмотреть все ошибки. А Cppcheck вообще бесплатен. Только так вы будете знать, какой из этих двух инструментов лучше подходит именно для вас.

Обнаруженные ошибки в Doom 3 с помощью Cppcheck

Фрагмент 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). Как написано сейчас - передается размер указателя и обнуляется объект не полностью.

Фрагмент 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;

Ошибка связана с тем, что память выделяется как для массива, а освобождается как от одного элемента. Конкретно в этом случае (для int) это не проблема, но если бы создавался массив объектов с деструктором, то вызван бы был только один деструктор, а не все. Правильный вариант delete [] sortIndex.

Фрагмент 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 ) );

Такая же ошибка, как и во фрагменте 1 - надо писать sizeof(*newBlock));

Фрагмент 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 );

Здесь перепутаны местами второй и третий аргументы - должно быть memset(&statex, 0, sizeof( statex)). Особенность данной ошибки в том, что глазами ее заметить очень сложно.

Обнаруженные ошибки в Doom 3 с помощью PVS-Studio

Фрагмент 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( "" );
    ...

В этой функции две одинаковые ветки if с разным содержанием. Скорее всего, в одной из них опечатка.

Фрагмент 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() )
  {
  ...

В этом фрагменте кода сравниваются значения разных типов, т.е. принадлежащих разным enum. Хотя в каких-то отдельных случаях это может и работать, но это - явная ошибка.

Фрагмент 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 );
}

Здесь хотели проверить строку classname, чтобы убедиться, что она не пустая. Однако сравнение не отрабатывает, так как надо разыменовать указатель.

Фрагмент 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 );

Ошибка повторяет фрагмент 3 - нужно разыменование указателя.

Фрагмент 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 ) );
}

Здесь видна целая история из жизни данного фрагмента кода. Когда-то clientInPVS был локальным массивом и sizeof(clientInPVS)/sizeof(int) вычислял действительно количество элементов. Однако со временем clientInPVS стали передавать как параметр в функцию. Но код остался прежним. В результате значение sizeof(clientInPVS)/sizeof(int) всегда равняется или 1 для 32-битной, или 2 для 64-битной платформы. Для исправления надо явно передавать количество элементов.

Фрагмент 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;

В данном фрагменте класс содержит виртуальные функции, но не содержит виртуального деструктора. И хотя это не всегда проблема, лучше всегда заводить виртуальный деструктор в таком случае, чтобы проблема не возникла позже.

Фрагмент 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) {

Опять проверяется одно и то же условие в разных ветках кода. Скорее всего просто неудачно скопировано.

Фрагмент 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++ ) {

Странность кода заключается в том, что счетчик цикла i используется и для внешнего, и для внутреннего цикла.

Фрагмент 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 */

Здесь должно быть sizeof(*ctx), а как написано сейчас - передается размер указателя и обнуляется объект не полностью.

Фрагмент 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 ) );

Не первый раз встречающаяся ошибка, вместо размера объекта в функцию memset передается размер указателя, который всегда один и тот же.

Фрагмент 11

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

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

Не менее распространенная проблема - увеличение значения указателя вместо увеличения значения объекта, на который он указывает. Правильный вариант - (*bp)++.

В этом же файле есть еще две похожие ошибки, которые не включены в отчет.

Фрагмент 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++ ) {

Здесь внутренний цикл идет по переменной k, однако увеличивается переменная j. Издержки копирования кода.

Фрагмент 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++ ) {

Вновь для внутреннего и внешнего счетчиков цикла используется одна и та же переменная.

Фрагмент 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 );

Здесь перепутаны местами второй и третий аргументы - должно быть memset(&statex, 0, sizeof( statex)). Особенность данной ошибки в том, что глазами ее заметить очень сложно.

Фрагмент 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 );
}

Очень красивая ошибка. Копирование кода до добра не доводит. Во втором блоке забыли заменить sizeof(portal) на sizeof(cluster).

Фрагмент 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), иначе используется размер указателя.

Фрагмент 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;

Здесь из-за приоритета оператора "!" (который выше, чем у "&") получилось некорректное условие. Хотели проверить, что младший бит равен нулю, а проверили, что все биты равны нулю.

Сводная таблица обнаруженных ошибок (количество) в Doom 3

Обнаружено ошибок с помощью Cppcheck: 4.

Обнаружено ошибок с помощью PVS-Studio: 17.

Из них пересекающихся ошибок (обнаружены и Cppcheck, и PVS-Studio): 3.

Обратите внимание, что возможно набор проверяемых файлов не полностью идентичен.

Обнаруженные ошибки в Quake 3: Arena с помощью Cppcheck

Фрагмент 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));

В этом коде используется странное выражение sizeof(MAX_ADDRESSLENGTH). Это всегда будет размер типа этой переменной, а не ее значение. Вероятно, должно быть просто MAX_ADDRESSLENGTH без sizeof().

Фрагмент 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;
  }
  ...
}

Здесь вполне возможна ситуация, когда выделенная для buf память так и останется не освобожденной. Классический пример того, зачем в C++ придумали умные указатели.

Фрагмент 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

Опять промахиваемся с размером массива и количеством элементов.

Фрагмент 4

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

Assert statement modifies 'j'.

assert( (j++) < 68 );

Странный фрагмент - ведь assert в release-сборке обычно отсутствует. Таким образом, не понятно, то ли j++ должно быть ВНЕ assert, то ли это код действительно только для debug-версии.

Фрагмент 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 ) );
}

Рассчитать размер массива с помощью sizeof в этом случае просто не возможно и матрица будет скопирована не полностью. Переменная src - это просто указатель.

Фрагмент 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 печатает два числа, а параметров передается три. То ли лишний параметр передают, то ли забыли его напечатать.

Обнаруженные ошибки в Quake 3: Arena с помощью PVS-Studio

Фрагмент 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 ) );
}

Рассчитать размер массива с помощью sizeof в этом случае просто не возможно и матрица будет скопирована не полностью.

Фрагмент 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);

Здесь переменная frac рассчитывается одинаковым образом, хотя перед этим проверяется условие. Возможно, она должна рассчитываться по-разному.

Фрагмент 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 ) );

Третьим аргументом memset оказывается размер указателя, а не размер объекта.

Фрагмент 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';

Промах по массиву - надо было писать sizeof() - 1.

Фрагмент 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;
}

Массив состоит всего лишь из двух элементов, а значения enum, используемые в качестве счетчика явно больше этого. Из-за чего естественно возникает выход за границы массива.

Фрагмент 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 ) );

Опять передается не размер объекта, а размер указателя. Правильный вариант sizeof(*var).

Фрагмент 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

Промахиваемся по массиву из-за того, что элементов не 4, а 3.

Сводная таблица обнаруженных ошибок (количество) в Quake 3: Arena

Обнаружено ошибок с помощью Cppcheck: 6.

Обнаружено ошибок с помощью PVS-Studio: 7.

Из них пересекающихся ошибок (обнаружены и Cppcheck, и PVS-Studio): 2.

Обратите внимание, что возможно набор проверяемых файлов не полностью идентичен.

Обнаруженные ошибки в Wolfenstein: Enemy Territory с помощью Cppcheck

Фрагмент 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

Здесь классическая утечка ресурсов - файл открывается, но не закрывается. Конечно, этот код находится в файле examples, что снимает с него вину. Но утечка от этого не пропадает.

Фрагмент 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 );

Строка печатается сама в себя. Это может привести к проблемам в большинстве случаев.

Фрагмент 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 ) );

Одним из аргументов функции является массив. Его размер пытаются вычислить с помощью sizeof(), хотя надо явно или передавать размер (что более правильно), или жестко написать размер "2 * sizeof(int)", раз в этом коде он все равно прописан.

Фрагмент 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 ) );

Здесь вместо размера объекта вычисляется размер указателя.

Фрагмент 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 ) );

Снова и снова вместо размера объекта используется размер указателя.

Фрагмент 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 ) );
}

Рассчитать размер массива с помощью sizeof в этом случае просто не возможно и матрица будет скопирована не полностью.

Фрагмент 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;
}

Здесь в ветке WP_MOBILE_MG42 два раза присваивается значение одной и той же переменной.

Фрагмент 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] );

Обращение к pnt[3] приводит к промаху мимо массива.

Обнаруженные ошибки в Wolfenstein: Enemy Territory с помощью PVS-Studio

Фрагмент 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 ) );
}

Рассчитать размер массива с помощью sizeof в этом случае просто не возможно и матрица будет скопирована не полностью.

Фрагмент 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 ) );

Одним из аргументов функции является массив. Его размер пытаются вычислить с помощью sizeof(), хотя надо явно или передавать размер (что более правильно), или жестко написать размер "2", раз в этом коде он все равно прописан.

Фрагмент 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 ) );

Здесь вместо размера объекта вычисляется размер указателя.

Фрагмент 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();
}

Приоритет операторов приводит к тому, что выражение вычисляется не так, как ожидал программист.

Фрагмент 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
  }

Вне зависимости от условия выполняется одинаковая ветка кода. Возможно, здесь должна быть другая ветка.

Фрагмент 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 ) );

Вычисляется размер указателя, а не размер объекта, так как в функцию передается указатель на указатель.

Фрагмент 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;

Вместо четвертого элемента дважды записывают в третий.

Фрагмент 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:

В switch и в case используются разные enum.

Фрагмент 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 ) );

Третьим аргументом memset оказывается размер указателя, а не размер объекта.

Фрагмент 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] );

Обращение к pnt[3] приводит к промаху мимо массива.

Фрагмент 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';

Промах по массиву - надо было писать sizeof() - 1.

Фрагмент 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;
}

Из-за приоритета операторов вычисление выражения произойдет не так, как ожидал программист.

Фрагмент 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;
}

Приоритет операторов снова меняет суть выражения.

Фрагмент 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++;
    }
  }

Доступ вне границ массива.

Фрагмент 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

Опять промахиваемся с размером массива и количеством элементов.

Фрагмент 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 ) );

Опять передается не размер объекта, а размер указателя. Правильный вариант sizeof(*var).

Фрагмент 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;
    }

Здесь в ветке WP_MOBILE_MG42 два раза присваивается значение одной и той же переменной.

Сводная таблица обнаруженных ошибок (количество) в Wolfenstein: Enemy Territory

Обнаружено ошибок с помощью Cppcheck: 8.

Обнаружено ошибок с помощью PVS-Studio: 17.

Из них пересекающихся ошибок (обнаружены и Cppcheck, и PVS-Studio): 6.

Обратите внимание, что возможно набор проверяемых файлов не полностью идентичен.

Итоговая таблица результатов сравнения

Doom 3

Обнаружено ошибок с помощью Cppcheck: 4.

Обнаружено ошибок с помощью PVS-Studio: 17.

Из них пересекающихся ошибок (обнаружены и Cppcheck, и PVS-Studio): 3.

Quake 3: Arena

Обнаружено ошибок с помощью Cppcheck: 6.

Обнаружено ошибок с помощью PVS-Studio: 7.

Из них пересекающихся ошибок (обнаружены и Cppcheck, и PVS-Studio): 2.

Wolfenstein: Enemy Territory

Обнаружено ошибок с помощью Cppcheck: 8.

Обнаружено ошибок с помощью PVS-Studio: 17.

Из них пересекающихся ошибок (обнаружены и Cppcheck, и PVS-Studio): 6.

Обратите внимание, что возможно набор проверяемых файлов не полностью идентичен.

"Не выводы"

Я не хочу делать никакие выводы из результатов этого сравнения. Эти результаты не значат, что один инструмент лучше. Просто мы запустили два инструмента, и нашли вот такие ошибки - и все. Выводы каждый делает сам. И лучше - проверив проекты самостоятельно. Возможно, мы даже что-то даже пропустили. Зачем же тогда эта статья? Просто теперь мне есть, что отвечать пользователям на вопрос: "А вы сравнивались с Cppcheck?".

Дополнительные ссылки