Содержание статьи
Зачем нужен статический анализатор
Современные приложения — это огромные массивы кода и библиотек. Уследить за ошибками в них — дело непростое. Можно забыть высвободить ресурсы, сделать «удачную» опечатку, при которой код собирается, но ведет себя неправильно, допустить утечку памяти... Короче, есть множество ситуаций, которые способны превратить отладку в изматывающее приключение. И если ты оказался в одной из них, это вовсе не значит, что ты плохой программист. Просто люди могут уставать, отвлекаться и допускать ошибки.
Для помощи программистам придумали специальные тулзы — статические анализаторы кода, которые помогают в разработке и выявляют разные ошибки: логические баги, опечатки, опасные конструкции, несоответствия каким‑то стандартам и конвенциям и так далее. Статические анализаторы применяются именно на этапе разработки, а не тестирования, поэтому позволяют править код на месте, что намного проще и быстрее.
Давай посмотрим, как обращаться с анализатором на примере PVS-Studio. Я покажу, какие ошибки будут им ловиться, а за чем придется следить самостоятельно. PVS-Studio «понимает» C/C++, C# и Java, поддерживает Windows, Linux и macOS. Но поскольку показывать я буду всё на рабочем проекте, то сфокусируюсь на C++ и Windows.
Установка и настройка
Итак, качаем PVS-Studio с сайта разработчика. На момент написания статьи актуальная версия — 7.19.
Промо
Чтобы получить бесплатную пробную лицензию на 30 дней, перейди по ссылке. В поле «промокод» будет вставлен код xakep, который компания‑разработчик предоставила для читателей «Хакера».
Что интересно, инсталлятор сразу нашел у меня на машине Visual Studio 2022 и IntelliJ IDEA Ultimate 2022 и сразу предложил интегрироваться в них. Не отказываемся, потому что будет удобно использовать статанализатор не как отдельную тулзу, а именно как плагин для IDE.
Теперь откроем Visual Studio и пройдемся по самым интересным настройкам.
Обрати внимание на IntermoduladAnalysisCpp, что означает «межмодульный анализ». Эта настройка применяется только к проектам на C++. Разработчики уверяют, что при включении этой опции анализатор производит анализ более эффективно. Проверим! Вот два сканирования: без этой опции и вместе с ней.
Как видишь, стало больше на одну ошибку класса Low. Не очень впечатляет, но галочку лучше оставить.
Следующая интересная опция — No Noise. Она исключает низкоприоритетные предупреждения, чтобы было проще ориентироваться в выдаче анализатора.
Рекомендую заглянуть и во вкладку Detectable Errors — здесь можно включать или выключать обнаружение определенных групп ошибок.
PVS-Studio умеет определять потенциальные уязвимости, связанные с безопасностью кода. Среди нацеленных на это наборов правил:
- CWE — база конструкций языка, которые содержат потенциальные уязвимости;
- AUTOSAR — набор правил для стандарта C++ 14 для встраиваемых отказоустойчивых систем;
- MISRA — правила, выработанные компанией MISRA для встраиваемых систем, где важны безопасность и отказоустойчивость;
- OWASP — правила для безопасной разработки веб‑приложений;
- SEI-CERT — правила, созданные центром CERT, опять же, для повышения надежности ПО.
Рекомендую включить все эти подсказки и обращать на них внимание при написании кода. Они значительно увеличат шанс не пропустить потенциальный баг.
Помимо этого, PVS-Studio определяет проблемы 64-битного кода, предлагает оптимизации производительности и даже содержит несколько правил (Custom), которые сделаны специально по заявкам трудящихся.
Анализатор на практике: разбираем примеры
Теперь перейдем к обзору найденных при помощи PVS-Studio ошибок и недочетов. Весь код, который будет тут представлен для примера, — реальный, из крупного опенсорсного проекта, который развивается несколько лет. «Синтетических» ошибок, искусственно созданных для демонстрации возможностей статанализатора, я приводить не буду, ведь это не так интересно.
Запускаем сканирование! Первый недочет, найденный анализатором и имеющий рейтинг Medium:
V560 A part of conditional expression is always true: threadArg
Если нажать на номер ошибки, откроется сайт разработчика с пояснением (как на русском, так и на английском языках): «Анализатор обнаружил потенциально возможную ошибку внутри логического условия. Часть логического выражения всегда истинно и оценено как опасное». Разберемся...
EncryptionThreadPool.c 205:static TC_THREAD_PROC EncryptionThreadProc (void *threadArg){ EncryptionThreadPoolWorkItem *workItem; if (threadArg) {#ifdef DEVICE_DRIVER SetThreadCpuGroupAffinity ((USHORT) *(WORD*)(threadArg));#else SetThreadGroupAffinityFn SetThreadGroupAffinityPtr = (SetThreadGroupAffinityFn) GetProcAddress (GetModuleHandle (L"kernel32.dll"), "SetThreadGroupAffinity"); if (SetThreadGroupAffinityPtr && threadArg) { GROUP_AFFINITY groupAffinity = {0}; groupAffinity.Mask = ~0ULL; groupAffinity.Group = *(WORD*)(threadArg); SetThreadGroupAffinityPtr(GetCurrentThread(), &groupAffinity, NULL); }#endif }Анализатор ругается вот на эту строку:
if (SetThreadGroupAffinityPtr && threadArg)Просмотрим начало всей функции: если указатель threadArg валиден, мы входим в тело if. Далее, в нем есть еще один вложенный if с условием SetThreadGroupAffinityPtr , на который как раз и жалуется анализатор. Но threadArg мы ведь уже проверили, нет нужды его проверять опять! Код избыточный, хоть и не опасный. Смело убираем часть условия && и идем дальше.
Анализатор жалуется вот на этот код:
ci = crypto_open (); if (!ci) return FALSE; if (QueryPerformanceFrequency (&benchmarkPerformanceFrequency) == 0) { if (ci) crypto_close (ci); MessageBoxW (hwndDlg, GetString ("ERR_PERF_COUNTER"), lpszTitle, ICON_HAND); return FALSE; }Ошибка:
V547. Expression is always true/false
«Условие всегда истинно или ложно». В этом примере проверка существования ci уже есть:
if (!ci) return FALSE;Далее идет еще одна, хотя никаких манипуляций с ci нет:
if (ci) crypto_close (ci);Опять же некритично, но код можно почистить и похвалить анализатор за помощь. Едем дальше, нашли сразу три недостатка в такой функции:
void DisableCPUExtendedFeatures (){ g_hasSSE2 = 0; g_hasISSE = 0; g_hasMMX = 0; g_hasSSE2 = 0; g_hasISSE = 0; g_hasMMX = 0; g_hasAVX = 0; g_hasAVX2 = 0; g_hasBMI2 = 0; g_hasSSE42 = 0; g_hasSSE41 = 0; g_hasSSSE3 = 0; g_hasAESNI = 0; g_hasCLMUL = 0;}Ошибка:
V1048. Variable 'g_hasSSE2' was assigned the same value
Подсказка с сайта: «Анализатор обнаружил присваивание переменной значения, которое уже содержится в ней. Скорее всего, это логическая ошибка». При этом ошибки одинаковые для g_hasSSE2, g_hasISSE, g_hasMMX.
Смотрим внимательнее и понимаем, что безболезненно можем удалить вот эти строки:
g_hasSSE2 = 0; g_hasISSE = 0; g_hasMMX = 0;Дальше вот такое интересное предупреждение:
V1042 This file is marked with copyleft license, which requires you to open the derived source code. jitterentropy.h 23Оно сработало на стандартный текст лицензии GNU. Анализатор поясняет: «Анализатор обнаружил в файле copyleft лицензию, которая обязывает открыть остальной исходный код. Это может быть неприемлемо для многих коммерческих проектов». С одной стороны, мы тут вроде бы анализируем код, а не юридические тонкости. С другой — в мире еще есть разработчики, которые не знают о копилефте и думают, что раз код открытый, значит можно брать. Опять же, если ошибка не полезна, ты можешь ее отключить.
А вот интересно сработали правила OWASP (правда на тестовом коде проекта):
derive_key_streebog ("password", 8, "\x12\x34\x56\x78", 4, 5, dk, 96);Ошибка:
V5013 [OWASP-2.10.4] Storing credentials inside source code can lead to security issues. Tests.c 1762
Пояснение: «Анализатор обнаружил в коде данные, которые могут являться конфиденциальными». Ну, тут и добавить нечего. Анализатор заподозрил, что в коде прямым текстом указаны пароли, и оказался прав! Но в данном случае это механизм тестирования, и пароль некритичный.
Теперь включим анализ 64-битного кода. Анализатор сразу заприметил вот такую строчку:
GlobalMemoryStatus (&memoryStatus);Ошибка:
V303 The function 'GlobalMemoryStatus' is deprecated in the Win64 system. It is safer to use the 'GlobalMemoryStatusEx' function. Random.c 875
Другими словами, на 64-битной платформе функцию GlobalMemoryStatus применять не рекомендуется и ее лучше заменить на GlobalMemoryStatusEx. Впрочем, о той же ошибке сообщает и сам Visual Studio.
У анализатора могут быть и ложные срабатывания. К примеру, получаем вот такую ошибку уровня High:
V547 Expression is always true. Crypto.c 1465)
Подозрение вызвала вот эта строка:
VcProtectMemory (encID, pCryptoInfo->ks, MAX_EXPANDED_KEY,И следующая:
pCryptoInfo->ks2, MAX_EXPANDED_KEY,Взглянем на всю функцию.
static void VcInternalProtectKeys (PCRYPTO_INFO pCryptoInfo, uint64 encID){#ifdef TC_WINDOWS_DRIVER VcProtectMemory (encID, pCryptoInfo->ks, MAX_EXPANDED_KEY, pCryptoInfo->ks2, MAX_EXPANDED_KEY);#else VcProtectMemory (encID, pCryptoInfo->ks, MAX_EXPANDED_KEY, pCryptoInfo->ks2, MAX_EXPANDED_KEY, pCryptoInfo->master_keydata, MASTER_KEYDATA_SIZE, pCryptoInfo->k2, MASTER_KEYDATA_SIZE);#endif}Далее посмотрим на прототип VcProtectMemory:
void VcProtectMemory (uint64 encID, unsigned char* pbData, size_t cbData, unsigned char* pbData2, size_t cbData2, unsigned char* pbData3, size_t cbData3, unsigned char* pbData4, size_t cbData4)Очевидно, это ошибка анализатора (точнее, синтаксического парсера, как я понимаю), потому что никаких условий здесь нет и всегда истинными они быть не могут. Видимо, причина такого поведения — крупный макрос, который передается в качестве аргумента.
Или вот интересное поведение. Вопросы вызвал такой код.
BOOL bNewDesktopSet = FALSE; // wait for SwitchDesktop to succeed before using it for current thread while (true) { if (SwitchDesktop (pParam->hDesk)) { bNewDesktopSet = TRUE; break; } Sleep (SECUREDESKTOP_MONOTIR_PERIOD); } if (bNewDesktopSet) { SetThreadDesktop (pParam->hDesk);Анализатор ругается на строку if ( со следующим вердиктом:
V547 Expression 'bNewDesktopSet' is always true. Dlgcode.c 14113
Давай разбираться: bNewDesktopSet инициализируется как FALSE при объявлении, далее входим в цикл, в котором переключение bNewDesktopSet на TRUE возможно только в том случае, если сработает WinAPI SwitchDesktop. Де‑юре анализатор прав, но прав ли он по сути? Во‑первых, мы не можем быть уверены, произойдет ли событие SwitchDesktop (, потому что за поведение WinAPI мы не отвечаем. Во‑вторых, взгляни на архитектуру кода: выполнение тела if отдано на откуп поведению функции WinAPI SwitchDesktop, которая или сработает (будет переход), или образует вечный цикл, потому как while (. На мой взгляд, ошибки «Expression ... is always true» в таком случае быть не должно.
Дальше встречаем такую ошибку:
V112 Dangerous magic number
А вот строка‑нарушитель:
unsigned __int32 CRC = 0xffffffff;Ошибка означает, что используется «магическая» константа, о предназначении который знает только тот, кто ее написал.
При разработке кода системного уровня подобные ошибки будут мельтешить в глазах и мешать обращать внимание на более важные вещи. Поэтому просто отключай их, чтобы не мешали.
Выводы
Проверив наш проект, анализатор сделал несколько сотен замечаний — и это только по категории General, без включения дополнительных проверок, иначе счет пошел бы на тысячи! Разбирать их все в статье нет смысла, так она превратится в многостраничный отчет по разбору выдачи PVS-Studio — со ссылками на документацию и выдержками из Страуструпа.
Возможно, у тебя возникнет вопрос о том, зачем вообще нужны сторонние статические анализаторы, если во многие IDE они и так встроены. Все дело в количестве и качестве проверок. Когда анализатор — это основной проект для разработчика, то можно ожидать, что анализ будет более тщательным и глубоким (примерно как Windows Defender не заменит настоящий продвинутый файрвол). Так и оказалось в случае с PVS-Studio.
Не думай при этом, что статический анализатор — это волшебная палочка, которая выручит тебя из любой беды и будет писать хороший код за тебя. Работа с анализатором — это прежде всего разгребание отчетов и вдумчивое чтение кода. Автоматический инструмент тоже будет ошибаться, просто не так же, как человек. Зато вместе вы достигнете большего! Для разработчиков больших проектов это однозначно мастхэв.
