Одной из основных сложностей при использовании инструментов статического анализа является работа с ложными срабатываниями. Существует множество способов устранить ложные срабатывания, используя настройки анализатора или изменяя код. Я взял маленький проект Apple II emulator for Windows и покажу, как можно на практике работать с отчётом статического анализатора PVS-Studio. Покажу на примерах, как править ошибки и подавлять ложные срабатывания.
Введение
Я буду описывать идеальный процесс внедрения в проект методологии статического анализа. Он заключается в том, чтобы устранить все ложные срабатывания анализатора и настоящие ошибки так, чтобы анализатор выдавал 0 предупреждений. Именно такой подход мы применяли, работая с проектом Unreal Engine 4.На практике идеальный подход редко возможен. Поэтому в большом проекте разумно воспользоваться альтернативным подходом. Можно скрыть все имеющиеся на данный момент предупреждения и видеть только сообщения, относящиеся к новому или изменённому коду. Для этого в анализаторе PVS-Studio существует специальный механизм, сохраняющий информацию в специальной базе. Подробности можно узнать из статьи: Как внедрить статический анализ в проект, в котором более 10 мегабайт исходного кода?
Итак, скрыв все сообщения, Вы сможете внимательно следить за качеством нового кода. Находя ошибки в новом коде, вы быстро оцените всю мощь и пользу методологии статического анализа. А когда у вас появится свободное время, вы сможете вернуться к скрытым предупреждениям и постепенно вносить необходимые правки в код.
Однако вернемся к идеальному счастливому миру. Представим, что у нас есть время, и мы можем спокойно позаниматься с предупреждениями, которые выдаёт анализатор PVS-Studio.
В этой статье я хочу показать, как работать с предупреждениями анализатора. Причем мы пройдём полный путь — от первой проверки, до окна, в котором будет 0 предупреждений.
Именно поэтому я выбрал маленький проект. Я мог бы взять и побольше, но тогда я устану писать статью, а вы — её читать. Впрочем, вы всё равно устанете. Статья даже для маленького проекта получится большой, но прошу уделить ей внимание. Она может помочь вам более эффективно использовать наш анализатор кода.
Подопытной мышью стал проект Apple II emulator for Windows. Выбор пал на него совершенно случайно. Поэтому не будем на нём останавливаться. Мне было все равно, какой проект взять, главное, чтобы он оказался маленький, но при этом в нем что-то находилось интересное.
Характеристики проекта:
- Размер исходного кода: 3 мегабайта.
- Количество строк кода: 85700.
- Время анализа (8 процессорных ядер): 30 секунд.
Первый запуск
После первого запуска анализатора мы имеем следующие предупреждения:
Рисунок 1. Предупреждения, выданные при первом запуске анализатора PVS-Studio для проекта Apple II emulator for Windows.
В статье я буду обсуждать только предупреждения 1 и 2 уровня, относящиеся к анализу общего назначения (GA). Можно было бы «победить» и 3 уровень, но тогда статья затянется. На 3 уровень мы глянем совсем быстро, и я дам некоторые пояснения, но исправлять ничего не буду.
Микрооптимизации (OP): сейчас не интересно.
Про 64-битные диагностики: в проекте нет 64-битной конфигурации. Не интересно.
Проверив проект, я отсортировал все предупреждения по коду. Это можно сделать, кликнув мышкой на столбец «Code» (см. рисунок N2).
Рисунок 2. Окно с сообщениями PVS-Studio. Сообщения отсортированы по номеру диагностики.
Сортировка по коду облегчает работу с сообщениями. Возникают группы однотипных сообщений. Разобравшись в причине возникновения одного сообщения, можно легко и быстро обработать остальные.
Примечание. У читателя может возникнуть вопрос, почему бы сразу не делать такую сортировку. Дело в том, что хочется позволить пользователям смотреть на сообщения ещё в процессе анализа. Если их сортировать, то новые сообщения в процессе анализа будут добавляться не в конец, а в разные места списка. В результате сообщения будут «прыгать». Работать с таким «дергающимся» списком будет невозможно.
Работа с сообщениями анализатора
В решении (solution) имеется три проекта (они видны в окне Solution Explorer на рисунке N2). Два из них, zlib и zip_lib, проверять не интересно. Поэтому их нужно исключить из анализа. На самом деле достаточно исключить zip_lib, так как zlib уже по умолчанию добавлен в исключения. Это делается в окне настроек PVS-Studio (раздел Don't Check Files):
Рисунок N3. Исключили zip_lib из проверки.
Я исключил ненужный проект заранее. Однако это легко сделать уже после проверки. Причём не обязательно для этого отправляться в настройки. В контекстном меню есть пункт, позволяющий удобно спрятать все сообщения, относящиеся к файлу, или определённому каталогу. Это очень удобно. Рекомендую познакомиться со статьёй "PVS-Studio для Visual C++". В ней описана эта и другие возможности, которые помогут эффективно использовать инструмент.
Итак, всё готово к началу работы над сообщениями. Начнем с диагностик под номером V501 и пойдем далее. Всего мы рассмотрим 32+49 = 81 сообщение. Это достаточно много. Поэтому местами я буду писать подробно, а местами буду краток.
Ложное срабатывание на макросах xxxxxREG
Первые 6 сообщений возникают из-за сложных макросов ADDXXREG, ADCHLREG, SBCHLREG, SBCHLREG. При их раскрытии возникают избыточные конструкции, и анализатор выдаёт, например, такое сообщение:V501 There are identical sub-expressions to the left and to the right of the '^' operator: (tmp >> 8) ^ reg_ixh ^ reg_ixh z80.cpp 3444
Макрос ADDXXREG весьма большой и состоит из других макросов. Поэтому не буду приводить его в статье.
Нам важно, что операция XOR дважды выполняется с переменной reg_ixh. Соответственно выражение можно упростить до (tmp >> 8). Однако никакой ошибки здесь нет. Всего лишь получилась избыточное выражение при подстановке определенных аргументов макроса:
ADDXXREG(reg_ixh, reg_ixl, reg_ixh, reg_ixl, 15, 2);
Это ложные срабатывания, и мы должны их устранить. Подавим все предупреждения, связанные с ними. Для этого в заголовочном файле, где объявлены эти макросы, я добавил следующие комментарии:
- //-V:ADDXXREG:501
- //-V:ADCHLREG:501
- //-V:SBCHLREG:501
- //-V:SBCHLREG:501
Подробности про этот механизм подавления предупреждений можно прочитать в соответствующем разделе документации.
В принципе, можно обойтись одним комментарием. В именах всех макросов имеется «REG». Поэтому можно написать один комментарий: //-V:REG:501. Он подавит предупреждения V501 в тех строках, где будет встречаться «REG». Но это плохо, так как случайно можно спрятать полезное сообщение, не имеющее отношение к перечисленным выше макросам. Чуть улучшить ситуацию можно, добавив для поиска скобку: //-V:REG(:501. В данном случае я считаю, что не стоит лениться и следует написать 4 комментария.
Ошибка в параметрах функции sprintf()
sprintf( sText, "%s %s = %s\n"
, g_aTokens[ TOKEN_COMMENT_EOL ].sToken
, g_aParameters[ PARAM_CATEGORY ].m_sName
, g_aParameters[ eCategory ]
);Предупреждение: V510 The 'sprintf' function is not expected to receive class-type variable as fifth actual argument. debug.cpp 2300
И действительно, пятым фактическим аргументом является структура, имеющая тип Command_t. По все видимости, в качестве аргумента следует использовать g_aParameters[eCategory].m_sName. Внес соответствующую правку в код.
Плохо пахнущий ZeroMemory()
Следующее сообщение говорит нам о том, что один массив заполняется не полностью: V512 A call of the 'memset' function will lead to underflow of the buffer 'pHDD->hd_buf'. harddisk.cpp 491BYTE hd_buf[HD_BLOCK_SIZE+1]; // Why +1?
ZeroMemory(pHDD->hd_buf, HD_BLOCK_SIZE);Последний байт не обнуляется. Не могу сказать, ошибка это или нет. Обратите внимание на комментарий. Возможно, даже сами разработчики не знают, какого размера должен быть массив и следует ли обнулять его целиком.
О таком коде говорят, что он с запахом. Он не обязательно ошибочен, но подозрителен и может стать причиной ошибки в дальнейшем.
Я просто подавлю это предупреждение с помощью комментария. Можно самостоятельно внести правки в файл, а можно воспользоваться пунктом контекстного меню «Mark selected messages as False Alarms»:
Рисунок 3. Добавление в код комментария для подавления предупреждения.
Выбрав этот пункт, анализатор сам вставит в код комментарий:
ZeroMemory(pHDD->hd_buf, HD_BLOCK_SIZE); //-V512Ложное срабатывание на вызове функции memcpy()
unsigned char random[ 256 + 4 ];
memcpy( &memmain[ iByte ], random, 256 );Функция memcpy() копирует только часть буфера 'random'. Анализатор считает это подозрительным и честно сообщает об этом. В данном случае он неправ, и ошибки нет. Я подавил предупреждение, как и в предыдущем случае, с помощью комментария. Это не очень красиво, но как лучше поступить в чужом коде я не знаю.
Лишние действия
nAddress_ = 0;
nAddress_ = (unsigned)*(LPBYTE)(mem + nStack);
nStack++;
nAddress_ += ((unsigned)*(LPBYTE)(mem + nStack)) << 8;Предупреждение: V519 The 'nAddress_' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 568, 569. debugger_assembler.cpp 569
Анализатор обратил внимание, что переменной nAddress_ присваивают различные значения несколько раз подряд. Ошибки здесь никакой нет, просто лишний код. Я удалил первую строчку, где переменной присваивается значение 0. Ещё один вариант избавиться от предупреждения, это заменить второе присваивание на "+=".
Аналогичную ситуацию можно наблюдать ещё в двух файлах:
Файле video.cpp (см. строку 3310 и 3315). Я удалил лишнюю операцию «pSrc += nLen;».
Файле Debug.cpp (см. строку 5867 и 5868). Заменил:
char *p = sLine;
p = strstr( sLine, ":" );char *p = strstr( sLine, ":" );Подробнее на этих фрагментах останавливаться не буду.
Ошибка в операторе switch
Следующая диагностика V519 уже указывает на настоящую серьезную ошибку. Хотя ошибка классическая, и все про неё знают, мы вновь и вновь встречаем её в различных программах.switch( c )
{
case '\\':
eThis = PS_ESCAPE;
case '%':
eThis = PS_TYPE;
break;
default:
sText[ nLen++ ] = c;
break;
}Предупреждение: V519 The 'p' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 5867, 5868. debug.cpp 5868
После «eThis = PS_ESCAPE;» нет оператора 'break'. Из-за этого значение переменной 'eThis' тут же поменяется на PS_STYPE. Это явная ошибка. Чтобы её исправить, я добавил оператор 'break'.
Ошибка: всегда ложное условие
inline static ULONG ConvertZ80TStatesTo6502Cycles(UINT uTStates)
{
return (uTStates < 0) ?
0 : (ULONG) ((double)uTStates / uZ80ClockMultiplier);
}Предупреждение: V547 Expression 'uTStates < 0' is always false. Unsigned type value is never < 0. z80.cpp 5507
Программист хотел защитится от случая, когда в функцию будет передано отрицательное значение. Однако, поскольку переменная 'uTStates' является беззнаковой, то защита не сработает.
Я добавил явное приведение к типу 'INT':
return ((INT)uTStates < 0) ?
0 : (ULONG) ((double)uTStates / uZ80ClockMultiplier);Излишняя настороженность анализатора
В следующей функции анализатор беспокоится, что может произойти выход за границу массива.void SetCurrentImageDir(const char* pszImageDir)
{
strcpy(g_sCurrentDir, pszImageDir);
int nLen = strlen( g_sCurrentDir );
if( g_sCurrentDir[ nLen - 1 ] != '\\' )
....
}Предупреждение: V557 Array underrun is possible. The value of 'nLen — 1' index could reach -1. applewin.cpp 553
Если в функцию передать пустую строку, то её длина будет равна нулю. Тогда произойдёт выход за границу массива: g_sCurrentDir[ 0 — 1 ].
Анализатор не знает, возможна такая ситуация или нет. Поэтому на всякий случай и предупреждает.
Я тоже не знаю, возможна такая ситуация или нет. Если возможна, то анализатор нашел ошибку. Если нет — это ложное срабатывание.
Я решил считать, что это ложное срабатывание. Однако будет более полезным улучшить код, а не добавлять комментарий для подавления предупреждения. Поэтому пусть в функции будет добавлена дополнительная проверка:
if (nLen == 0)
return;Есть ещё одно место, где теоретически может произойти выход за границу массива. Но надо стараться не превратить статью в толстый справочник. Поэтому описывать это предупреждение не буду и просто подавлю его с помощью комментария. См. этот же файл (строка 556).
Присваивание вместо сравнения
if ((bytenum == 3) && (byteval[1] = 0xAA))
{Предупреждение: V560 A part of conditional expression is always true: (byteval[1] = 0xAA). diskimagehelper.cpp 439
Я уверен, что хотели выполнить операцию '==', а не '='. Если нужно присваивание, намного естественней было бы написать так:
if (bytenum == 3)
{
byteval[1] = 0xAA;Поэтому это ошибка, и её стоит исправить:
if ((bytenum == 3) && (byteval[1] == 0xAA))Ложное срабатывание из-за макросов
if ((TRACKS_MAX>TRACKS_STANDARD) && ....)Предупреждение: V560 A part of conditional expression is always true: ((35 + 5) > 35). diskimagehelper.cpp 548
Если раскрыть макросы, то получится выражение ((35 + 5) > 35). Выражение всегда истинно, но это не ошибка.
Этот тот случай, когда я даже не знаю, как лучше поступить. Схалтурю и просто подавлю ложное срабатывание с помощью комментария: //-V560.
Лишняя переменная
В процессе рефакторинга иногда остаются «потерянные» переменные. Они как-то используются в коде, но на самом деле больше не нужны. Видимо именно так произошло с переменной bForeground:BOOL bForeground;
....
bForeground = FALSE;
....
if( bForeground )
dwCoopFlags |= DISCL_FOREGROUND;
else
dwCoopFlags |= DISCL_BACKGROUND;
....
if( hr == DIERR_UNSUPPORTED && !bForeground && bExclusive )И больше переменная 'bForeground' нигде не изменяется и не используется. Это приводит к возникновению предупреждения: V560 A part of conditional expression is always true: !bForeground. mouseinterface.cpp 690
Это интересный пример для философии. Ложное это срабатывание или нет? Даже человеку сложно дать ответ. Анализатор прав, выявив аномалию. Но с точки зрения человека этот фрагмент может являться недописанным кодом. И тогда всё в порядке.
Будем считать, что это ещё один пример «кода с запахом». Я удалил переменную ' bForeground' из кода.
Неопределённо поведение
*(mem+addr++) =
(opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;Предупреждение: V567 Undefined behavior. The 'addr' variable is modified while being used twice between sequence points. cpu.cpp 564
Неизвестно, как будет вычисляться выражение:
- Возможно, сначала увеличится переменная 'addr', а затем она будет использована в правой части выражения.
- А возможно наоборот.
*(mem+addr) =
(opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;
addr++;Неверные аргументы при вызове функции wsprintf() и аналогичных ей
Есть несколько ошибок, связанных с передачей неправильного количества фактических аргументов в функции форматированного вывода. Всего таких ошибок было найдено 10, но мы рассмотрим только одну:wsprintf( sText, TEXT("%s full speed Break on Opcode: None")
, sAction
, g_iDebugBreakOnOpcode
, g_aOpcodes65C02[ g_iDebugBreakOnOpcode ].sMnemonic
);Предупреждение: V576 Incorrect format. A different number of actual arguments is expected while calling 'wsprintfA' function. Expected: 3. Present: 5. debug.cpp 939
При формировании строки два последних параметра не учитываются. Мне, как стороннему человеку, сложно сказать, являются эти параметры лишними, или ошибка допущена в строке форматирования.
Я решил, что параметры лишние, и удалил их.
Аналогичные проблемы наблюдается в следующих участках кода:
- Expected: 8. Present: 9. debug.cpp 7377
- Expected: 3. Present: 4. debugger_help.cpp 1263
- Expected: 3. Present: 4. debugger_help.cpp 1265
- Expected: 3. Present: 4. debugger_help.cpp 1267
- Expected: 3. Present: 4. debugger_help.cpp 1282
- Expected: 3. Present: 4. debugger_help.cpp 1286
- Expected: 3. Present: 4. debugger_help.cpp 1288
- Expected: 5. Present: 4. debugger_help.cpp 1332
- Expected: 3. Present: 4. frame.cpp 691
- Expected: 3. Present: 4. frame.cpp 695
Ещё есть пара мест, где для распечатки значений указателя используется "%08X". На 32-битной системе это на практике работает. Но в 64-битной системе будет распечатана только часть указателя. Правильным решением является использовать "%p". Соответствующие участки кода:
- To print the value of pointer the '%p' should be used. tfe.cpp 507
- To print the value of pointer the '%p' should be used. tfe.cpp 507
Ложные срабатывания на двойных сравнениях
Хотя анализатор не виноват, он выдал два ложных срабатывания на повторяющихся условиях. Рассмотрим один случай:if (nAddress <= _6502_STACK_END)
{
sprintf( sText,"%04X: ", nAddress );
PrintTextCursorX( sText, rect );
}
if (nAddress <= _6502_STACK_END)
{
DebuggerSetColorFG( DebuggerGetColor( FG_INFO_OPCODE ));
sprintf(sText, " %02X",(unsigned)*(LPBYTE)(mem+nAddress));
PrintTextCursorX( sText, rect );
}Предупреждение: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 2929, 2935. debugger_display.cpp 2935
Ошибки нет. Программист просто разделил два набора действий. С точки зрения анализатора такой код подозрителен. Вдруг условия должны отличаться? Тем не менее с ложным срабатыванием надо что-то делать. Я решил объединить два условных оператора в один:
if (nAddress <= _6502_STACK_END)
{
sprintf( sText,"%04X: ", nAddress );
PrintTextCursorX( sText, rect );
DebuggerSetColorFG( DebuggerGetColor( FG_INFO_OPCODE ));
sprintf(sText, " %02X",(unsigned)*(LPBYTE)(mem+nAddress));
PrintTextCursorX( sText, rect );
}Думаю, читаемость кода от этого не пострадала, но попутно мы избавились от ложного срабатывания.
Второй случай аналогичен: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 2237, 2245. debugger_display.cpp 2245
Рисунок 5. В середине длинной статьи рекомендуют вставлять картинку, чтобы читатель отдохнул. Я не знаю, какую относящуюся к статье картинку вставить. Поэтому вот вам котик.
Разыменование указателя до проверки
Всего анализатор выдал 3 предупреждения на эту тему. К сожалению, текст программы в этих местах достаточно запутан. Поэтому для упрощения я приведу не настоящий код, а псевдокод. Для первых 2 предупреждений код выглядит приблизительно так:int ZEXPORT unzGetGlobalComment(char *szComment)
{
....
if (A)
{
*szComment='\0';
return UNZ_ERRNO;
}
....
if ((szComment != NULL) && X)
....
}Предупреждение: V595 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 1553, 1558. unzip.c 1553
Как видите, переданный указатель 'szComment' может быть равен NULL. Об этом свидетельствует проверка (szComment != NULL).
Однако имеется участок кода, в котором указатель смело разыменовывается без выполнения проверки. Это опасно. Возможно, что на практике никогда не возникает ситуаций, когда 'szComment' равен 0. Но код опасен, и его следует исправить.
Аналогично: V595 The 'pToken_' pointer was utilized before it was verified against nullptr. Check lines: 811, 823. debugger_parser.cpp 811
А вот с последним, третьим случаем, всё сложнее. Я уже устал доказывать, что подобный код неверен и его стоит исправить. Функция короткая, поэтому я приведу её целиком
bool ArgsGetValue ( Arg_t *pArg,
WORD * pAddressValue_, const int nBase )
{
TCHAR *pSrc = & (pArg->sArg[ 0 ]);
TCHAR *pEnd = NULL;
if (pArg && pAddressValue_)
{
*pAddressValue_ =
(WORD)(_tcstoul( pSrc, &pEnd, nBase) & _6502_MEM_END);
return true;
}
return false;
}Предупреждение: V595 The 'pArg' pointer was utilized before it was verified against nullptr. Check lines: 204, 207. debugger_parser.cpp 204
Указатель 'pArg' может равен нулю, о чём свидетельствует условие «if (pArg && pAddressValue_)». Но до того, как указатель будет проверен, он используется в выражении:
TCHAR *pSrc = & (pArg->sArg[ 0 ]);Это выражение приводит к неопределённому поведению программы. Нельзя разыменовывать нулевые указатели.
Многие возражают, что в таком коде не происходит обращение к памяти, а просто вычисляется некий адрес. Следовательно, никакой проблемы нет. Однако это слишком узкое толкование неопределенного поведения. Вообще не надо гадать, как будет вести себя компилятор и как код будет работать. Так нельзя писать, и нет смысл обсуждать, почему именно.
Неопределенное поведение в таком коде — это не только обращение к нулевому адресу памяти (которого действительно может и не быть). Например, компилятор вполне вправе сократить условие проверки до «if (pAddressValue_)». Раз есть выражение «pArg->xxx», значит указатель точно не нулевой и его не надо проверять.
Более подробно обсуждать этот вопрос смысла нет. Я предлагаю познакомиться со специальной статьёй: Разыменовывание нулевого указателя приводит к неопределённому поведению.
Исправить код просто. Достаточно перенести объявление переменной внутрь 'if'.
Пугающее выражение
Анализатор смутило следующее выражение:if ((cx > 4) & (cx <= 13))Предупреждение: V602 Consider inspecting the '(cx > 4)' expression. '>' possibly should be replaced with '>>'. debug.cpp 8933
Анализатор видит, что используется оператор '&', операндами которого являются применяемый типа 'bool'. Это странно. Обычно в таких случаях принято использовать логический оператор '&&'.
Оператор '&' принято использовать для битовых операций. Поэтому анализатор предположил возможность, что и здесь хотели работать с битами:
if ((cx >> 4) & (cx <= 13))Анализатор здесь перемудрил и неправ. Однако вина программиста здесь тоже есть. Этот код с запахом. Намного естественней будет написать:
if (cx > 4 && cx <= 13)Неуточняемое поведение и ужас в макросах
Непонятно, какой результат получится при сдвиге вправо отрицательных значений. Лучше так не делать, так как на другом компиляторе поведение кода может измениться.const short SPKR_DATA_INIT = (short)0x8000;
if (g_nSpeakerData == (SPKR_DATA_INIT >> 2))Предупреждение: V610 Unspecified behavior. Check the shift operator '>>'. The left operand 'SPKR_DATA_INIT' is negative. speaker.cpp 450
Выход: можно объявить константу SPKR_DATA_INIT беззанковой. Правда тогда понадобится сделать ещё несколько мелких правок, чтобы не появились предупреждения компилятора и анализатора о сравнении знаковых/беззнаковых чисел.
Анализатор нашел ещё 3 таких опасных места:
- The left operand 'SPKR_DATA_INIT' is negative. speaker.cpp 453
- The left operand '~0x180' is negative. tfe.cpp 869
- The left operand '~0x100' is negative. tfe.cpp 987
Кстати, когда я начал править два последних предупреждения, то нашел ещё 2 ошибки. Можно сказать, что анализатор помог найти их косвенным путём.
Вот как используется макрос:
SET_PP_16(TFE_PP_ADDR_SE_BUSST, busst & ~0x180);Он раскрывается в длинную строку. Поэтому я покажу только её часть:
..... = (busst & ~0x180 >> 8) & 0xFF; .....Приоритет оператора сдвига >> больше, чем приоритет оператора &. См. таблицу: приоритеты операций.
Программист ожидал, что код будет работать так:
..... = ((busst & ~0x180) >> 8) & 0xFF; .....А на само деле он работает так:
..... = (busst & (~0x180 >> 8)) & 0xFF; .....Вот поэтому анализатор PVS-Studio и предупреждает: «the left operand '~0x180' is negative».
Вот как опасно использовать макросы!
Дырки в безопасноти
В проекте очень опасным образом используются функции sprintf(), wsprintf() и так далее. Если совсем кратко, то функции используются следующим образом:sprintf(buf, STR);Если строка STR будет содержать управляющие символы, такие как "%s", то последствия будут непредсказуемы.
Обычно такой код рассматривается как дыра в безопасности (см. подробности).
Однако для эмулятора, думаю, это не критично. Никто не будет его атаковать. Однако такой код опасен сам по себе. Он может легко привести к падению программы или к неправильной её работе.
Правильно делать так: sprintf(buf, "%s", STR);
Анализатор нашел достаточно много опасных вызовов функций. Всего он выдал 21 предупреждение.
Противоположные условия
// TO DO: Need way of determining if DirectX init failed
if (soundtype != SOUND_WAVE)
{
if (soundtype == SOUND_WAVE)
soundtype = SOUND_SMART;Предупреждение: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 270, 272. speaker.cpp 270
Судя по комментарию код не дописан. Сложно сказать, как поступать с таким кодом. Я решил закомментировать второй бессмысленный 'if':
if (soundtype != SOUND_WAVE)
{
//if (soundtype == SOUND_WAVE)
// soundtype = SOUND_SMART;Неудачное выравнивание кода
Код выглядит так, как будто оба действия относятся к оператору 'if':{
if ((Slot4 == CT_MockingboardC) || (Slot4 == CT_Phasor))
m_PropertySheetHelper.GetConfigNew().m_Slot[4] = CT_Empty;
m_PropertySheetHelper.GetConfigNew().m_Slot[5] = CT_SAM;
}Предупреждение: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. pagesound.cpp 229
Как я понял, ошибки в коде нет. Однако это не ложное срабатывание. Анализатор абсолютно прав, предупреждая о таком коде. Следует обязательно поправить выравнивание:
{
if ((Slot4 == CT_MockingboardC) || (Slot4 == CT_Phasor))
m_PropertySheetHelper.GetConfigNew().m_Slot[4] = CT_Empty;
m_PropertySheetHelper.GetConfigNew().m_Slot[5] = CT_SAM;
}Неправильная работа с функцией strncat()
strncat( sText, CHC_DEFAULT, CONSOLE_WIDTH );
strncat( sText, pHelp , CONSOLE_WIDTH );Предупреждение: V645 The 'strncat' function call could lead to the 'sText' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. debugger_help.cpp 753
Третий аргумент функции — количесто символов, которые ещё можно добавить к строке. Поэтому правильно и безопасно сделать так:
strncat( sText, CHC_DEFAULT, sizeof(sText) - strlen(sText) - 1);
strncat( sText, pHelp , sizeof(sText) - strlen(sText) - 1);Подробности можно узнать, познакомившись с описанием диагностики V645.
Лишние проверки
Уже очень давно оператор 'new' генерирует исключение std::bad_alloc, если не может выделить память. Однако в программах можно по-прежнему встретить ненужные проверки:BYTE* pNewImageBuffer = new BYTE [uNewImageSize];
_ASSERT(pNewImageBuffer);
if (!pNewImageBuffer)
return false;Предупреждение: V668 There is no sense in testing the 'pNewImageBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. diskimagehelper.cpp 197
_ASSERT и проверку можно и нужно убрать. Здесь в них нет никакого смысла.
Аналогичная ситуация:
- mouseinterface.cpp 175
- serialcomms.cpp 839
- savestate.cpp 108
- savestate.cpp 218
- speech.cpp 40
Самостоятельное объявление системных типов
В проекте состоятельно определяются некоторые типы данных:typedef unsigned long ULONG;
typedef void *LPVOID;
typedef unsigned int UINT;Явной ошибки здесь нет. Будем считать что это «код с запахом» и подавим предупреждения с помощью комментария //-V677.
Нарушен «Закон Большой Двойки»
Например, в классе CConfigNeedingRestart объявлен operator =, но нет конструктора копирования. Это нарушает "Закон Большой Двойки".Класс достаточно большой, поэтому не буду здесь приводить фрагменты кода. Поверьте на слово.
В этом классе все поля являются простыми типами, поэтому свой собственный operator = вообще не нужен. Класс будет успешно копироваться автоматически.
С классом Disk_t ситуация аналогична. И там и там можно удалить operator =.
Предупреждения анализатора:
- V690 The 'CConfigNeedingRestart' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. config.h 7
- V690 The 'Disk_t' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. disk.cpp 74
Опечатка
int nHeight=nHeight=g_aFontConfig[ FONT_CONSOLE ]._nFontHeight; Предупреждение: V700 Consider inspecting the 'T foo = foo = ...' expression. It is odd that variable is initialized through itself. debugger_display.cpp 1226
Просто опечатка. Заменил на:
int nHeight = g_aFontConfig[ FONT_CONSOLE ]._nFontHeight;Излишнее беспокойство анализатора по поводу перечислений
В перечислении 'AppMode_e' есть следующие именованные константы: MODE_LOGO, MODE_PAUSED, MODE_RUNNING, MODE_DEBUG, MODE_STEPPING.Анализатор переживает, что не все они используются в этом switch():
switch (g_nAppMode)
{
case MODE_PAUSED : _tcscat(.....); break;
case MODE_STEPPING: _tcscat(.....); break;
}Предупреждение: V719 The switch statement does not cover all values of the 'AppMode_e' enum: MODE_DEBUG, MODE_LOGO, MODE_RUNNING. frame.cpp 217
В этом примере мне за анализатор даже немного стыдно. Эмпирические алгоритмы подвели. Ложное срабатывание. Есть несколько способов его устранить. Например, можно добавить ветку «default».
switch (g_nAppMode)
{
case MODE_PAUSED : _tcscat(.....); break;
case MODE_STEPPING: _tcscat(.....); break;
default: break;
}Ещё одно аналогичное ложное срабатывание: V719 The switch statement does not cover all values of the 'AppMode_e' enum: MODE_DEBUG, MODE_LOGO. frame.cpp 1210
Я обещал бегло взглянуть на предупреждения третьего уровня
Мы не рекомендуем (по крайней мере на первых этапах) вообще заглядывать на 3 уровень. Там много ложных, неинтересных или специфичных сообщений. Сейчас именно такая ситуация.Например, здесь достаточно много предупреждений V601.
inline int IsDebugBreakpointHit()
{
if ( !g_bDebugNormalSpeedBreakpoints )
return false;
return _IsDebugBreakpointHit();
}Предупреждение: V601 The 'false' value is implicitly cast to the integer type. debug.h 210
Функция возвращает тип 'int'. Но при этом написано «return false».
Анализатор прав, что придирается к этому коду, но на практике за этим очень редко скрывается настоящая ошибка. Поэтому это предупреждение и попало на 3 уровень.
Теперь пример специфичной диагностики:
double g_fClksPerSpkrSample;
....
if ((double)g_nRemainderBufferSize != g_fClksPerSpkrSample)Предупреждение: V550 An odd precise comparison. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. speaker.cpp 197
Правилен этот код или нет — очень сильно зависит от приложения и от того, какие значения помещаются в переменные типа 'double'.
Ряд пользователей в восторге от этой диагностики. Другие говорят, что в double они кладут целые числа и знают, что делают, когда их сравнивают. Всем не угодишь.
Запуск после правок ошибок
Теперь, поправив все сообщения (1 и 2 уровень), мы можем перезапустить анализатор. Результат ожидаем — все предупреждения исчезли (см. рисунок 6).
Рисунок 6. Больше нет предупреждений 1 и 2 уровня.
Этот идеальный вариант, применимый только к маленьким проектам. Тем не менее, я надеюсь, что мне удалось показать, что нет ничего сложного в работе с сообщениями анализатора. Хотя часть сообщений были ложными, с ними не возникло затруднений, и все их удалось устранить.
Подведём итоги
Нас часто спрашивают, какое количество ложных срабатываний выдаёт наш анализатор. У нас нет ответа. Собрать такую статистику очень сложно, и она мало что даёт. Количество ложных срабатываний очень сильно меняется от проекта к проекту.Ещё есть проблема с интерпретированием данных. Например, из-за неудачного макроса, который активно используется во всём проекте, количество ложных срабатываний может быть в 20 раз больше, чем полезных сообщений. Но это не беда. Достаточно подавить предупреждения в этом макросе, и количество ложных сообщений может сразу уменьшиться на 90%.
Следующая сложность дать ответ состоит в том, что программисты не учитывают, что некоторые предупреждения сложно классифицировать. Эти сообщения не выявляют ошибки, но выявляют «код с запахом». Такой код следует поправить, так как хотя сейчас он работает корректно, в дальнейшем он может стать причиной дефекта. В статье было показано несколько примеров таких диагностик.
Но программисты тяготеют к двоичной логике. И настаивают получить ответ: «Это ложное сообщение? Да/Нет?». Если вы внимательно прочитали статью, я надеюсь, так строго вопрос вы ставить не будите.
Как видите, говорить обобщенно о количестве ложных срабатываний сложно. Но если взять конкретный маленький проект, то дать ответ именно для него можно.
Итоги по диагностическим сообщениям, выданным анализатором PVS-Studio для проекта Apple II emulator for Windows:
- Всего выдано сообщений (General Analysis, первый и второй уровень): 81
- Найдено ошибок: 57
- Найдено фрагментов «кода с запахом», которые следует исправить: 9
- Ложные срабатывания: 15
- Сообщения, указывающие на ошибки: 70 %
- Сообщения, указывающие на «код с запахом»: 11 %
- Ложные срабатывания: 19 %
Заключение
Попробуйте статический анализатор PVS-Studio на своём проекте. Вы можете скачать демонстрационную версию по адресу: http://www.viva64.com/ru/pvs-studio-download/И предлагаю порекомендовать попробовать наш статический анализатор своим коллегам и друзьям. Прошу написать сообщение в twitter или любую иную новостную ленту. Спасибо.
P.S. Вы можете следить за нашими новыми статьями и вообще узнавать новости из мира Си/Си++, если подпишитесь на мой twitter: https://twitter.com/Code_Analysis
Спасибо всем за внимание.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey karpov. An Ideal Way to Integrate a Static Code Analyzer into a Project.
Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
