# Сравнение 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](http://cppcheck.sourceforge.net/), а особенно, Daniel Marjamäki \- крутые ребята, делающие полезное дело\. Их инструмент \- достоин внимания, а сами они \- молодцы\. Тем более что их продукт полностью free, а значит каждый, кто интересуется статическим анализом кода, может очень легко его посмотреть\. Наш бизнес \- это инструмент статического анализа C/C\+\+ кода [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. И раз уж так получилось, что мы являемся конкурентами, то мне приходится отвечать на вопросы наших пользователей: "А вы сравнивали PVS\-Studio с Cppcheck?"\. На такие вопросы мне приходится отвечать одним из следующих вариантов: * Понимаете, сравнить инструменты анализа кода сложно, потому что\.\.\. \(и далее по тексту раздела "Введение"\); * Вы можете сами скачать и сравнить; * "Мы" или "они" лучше для всех быть не могут\. Они могут быть лучше только для кого\-то\. В любом случае все эти ответы хоть и правильные, но не всегда устраивают людей\. Поэтому мы вынуждены выполнить сравнение PVS\-Studio и Cppcheck, и опубликовать его, чтобы было, что отвечать людям более конкретно\. Эта статья НЕ служит целью показать, что определенные функции одного продукта хороши, а другие плохи\. Это просто один из способов сравнить анализаторы, чтобы читатели могли САМИ сделать выбор в пользу наиболее подходящего им инструмента\. ## Методика сравнения Для сравнения PVS\-Studio 4\.61 и Cppcheck 1\.54 мы взяли исходный код трех проектов id Software с GitHub: [Doom 3](https://github.com/id-Software/DOOM-3), [Quake 3: Arena](https://github.com/id-Software/Quake-III-Arena), [Wolfenstein: Enemy Territory](https://github.com/id-Software/Enemy-Territory)\. Прогнали на нем оба анализатора, получили список ошибок\. Затем просмотрели \(вручную\) весь список\. И отобрали те ошибки, которые являются реальными\. Мы НЕ отбирали неудачный код, возможно некорректные конструкции и т\.п\. Только явные ошибки\. У нашего сравнения есть недостатки: 1. Набор анализируемых файлов оказался немного разным\. Это произошло из\-за того, что Cppcheck анализировал всю папку с файлами, а PVS\-Studio \- только те файлы, которые включены в файл проекта \.vcproj\. 1. Из\-за того, что списки найденных проблем просматривались человеком, то возможно, что некоторые обнаруженные ошибки были пропущены\. Не было цели показать, что один из инструментов нашел больше, чем другой, но все\-таки вероятность этого есть\. Таким образом, кому интересно РЕАЛЬНО сравнить 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\. ```cpp 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 ```cpp 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\. ```cpp 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 '&' ```cpp 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\) ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp void idGameSSDWindow::FireWeapon(int key) { ... } else if(gameStats.levelStats.targetEnt->type == SSD_ENTITY_ASTRONAUT) { HitAstronaut(static_cast( 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp #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\. ```cpp 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 ```cpp 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 ```cpp 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'\. ```cpp 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\. ```cpp 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 ```cpp static void do_uid(int x) { printf("%d", 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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\(\)\. ```cpp 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\. ```cpp 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\. ```cpp 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\. ```cpp 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\. ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp 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 ```cpp #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 ```cpp 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 ```cpp 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 ```cpp 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?"\. ## Дополнительные ссылки 1. [Cppcheck](http://cppcheck.sourceforge.net/)\. 1. Терминология\. [Обзор инструмента Cppcheck](https://pvs-studio.ru/ru/blog/terms/0083/)\. 1. [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. 1. [id Software on GitHub](https://github.com/id-Software)\.