Читатели наших статей иногда отмечают, что статический анализатор кода PVS-Studio выявляет большое количество ошибок, незначительных и не влияющих на работу приложения. Это действительно так. По большей части важные ошибки уже исправлены благодаря ручному тестированию, отзывам пользователей и другим дорогостоящим методам. При этом многие из этих ошибок можно было найти еще на этапе написания кода и исправить с минимальными потерями времени, репутации и денег. В этой статье будет приведено несколько примеров реальных ошибок, которые можно было бы сразу исправить, если бы авторы проекта использовали статический анализ кода.
Идея очень проста. Мы будем искать примеры запросов на вытягивание на GitHub, в которых указано, что проблема связана с исправлением ошибки. Затем попробуем найти эти ошибки с помощью статического анализатора кода PVS-Studio. Если ошибку смог найти анализатор, то это ошибка, которая могла быть обнаружена на этапе написания кода. Чем раньше ошибка будет исправлена, тем дешевле она будет стоить.
К сожалению, GitHub нас подвел, и у нас не получилось сделать большую шикарную статью на эту тему. В самом GitHub есть глюк (или фича), который не позволяет искать комментарии пулл-реквестов в проектах, написанных только на определенных языках программирования. Или я не умею его готовить. Несмотря на то, что я указываю искать комментарии в проектах C, C++, C#, результаты приведены для всех языков, включая PHP, Python, JavaScript и другие. В результате поиск подходящих случаев оказался чрезвычайно утомительным, и я приведу лишь несколько примеров. Однако их достаточно, чтобы продемонстрировать полезность инструментов статического анализа кода при регулярном использовании.
Что, если ошибка была обнаружена на самой ранней стадии? Ответ прост: программистам не нужно было бы ждать, пока он проявит себя, а потом искать и исправлять дефектный код.
Давайте посмотрим на ошибки, которые PVS-Studio мог сразу обнаружить:
Первый пример взят из проекта SatisfactoryModLoader. До исправления ошибки код выглядел так:
// gets an API function from the mod handler
SML_API PVOID getAPIFunction(std::string name) {
bool found = false;
for (Registry reg : modHandler.APIRegistry) {
if (reg.name == name) {
found = true;
}
}
if (!found) {
std::string msg = ...;
MessageBoxA(NULL,
msg.c_str(),
"SatisfactoryModLoader Fatal Error",
MB_ICONERROR);
abort();
}
}В этом коде была ошибка, на которую PVS-Studio немедленно выдавал предупреждение:
V591 Непустая функция должна возвращать значение. ModFunctions.cpp 44
Вышеупомянутая функция не имеет инструкции return, поэтому она вернет формально неопределенное значение. Программист не использовал анализатор кода, поэтому ему пришлось искать ошибку самостоятельно. Функция после редактирования:
// gets an API function from the mod handler
SML_API PVOID getAPIFunction(std::string name) {
bool found = false;
PVOID func = NULL;
for (Registry reg : modHandler.APIRegistry) {
if (reg.name == name) {
func = reg.func;
found = true;
}
}
if (!found) {
std::string msg = ...;
MessageBoxA(NULL,
msg.c_str(),
"SatisfactoryModLoader Fatal Error",
MB_ICONERROR);
abort();
}
return func;
}Любопытно, что в коммите автор пометил ошибку как критическую: «исправлена критическая ошибка, из-за которой функции API не возвращались».
Во втором коммите из истории проекта mc6809 были внесены правки в следующий код:
void mc6809dis_direct( mc6809dis__t *const dis, mc6809__t *const cpu, const char *const op, const bool b16 ) { assert(dis != NULL); assert(op != NULL);addr.b[MSB] = cpu->dp; addr.b[LSB] = (*dis->read)(dis, dis->next++);...if (cpu != NULL) { ... } }
Автор поправил только одну строчку. Он заменил выражение
addr.b[MSB] = cpu->dp;для следующего
addr.b[MSB] = cpu != NULL ? cpu->dp : 0;В старой версии кода не было никакой проверки нулевого указателя. Если случится так, что в функцию mc6809dis_direct в качестве второго аргумента будет передан нулевой указатель, то произойдет его разыменование в теле функции. Результат плачевный и непредсказуемый.
Разыменование нулевого указателя — один из самых распространенных паттернов, о которых нам говорят: «Это не критическая ошибка. Кого волнует, что он процветает в коде? Если произойдет разыменование, программа тихо рухнет, и все». Странно и грустно слышать это от программистов на C++, но жизнь случается.
Так или иначе, в этом проекте такое разыменование превратилось в баг, о чем нам говорит тема коммита: «Исправление ошибки — -NULL разыменование».
Если бы разработчик проекта использовал PVS-Studio, он мог бы проверить и найти предупреждение еще два с половиной месяца назад. Это когда ошибка была введена. Вот предупреждение:
V595 Указатель cpu использовался до того, как он был проверен на соответствие nullptr. Контрольные линии: 1814, 1821. mc6809dis.c 1814
Таким образом, баг был бы исправлен в момент его появления, что сэкономило бы время и нервы разработчика :).
Пример еще одного интересного фикса был найден в проекте libmorton.
Код, который нужно исправить:
template<typename morton>
inline bool findFirstSetBitZeroIdx(const morton x,
unsigned long* firstbit_location)
{
#if _MSC_VER && !_WIN64
// 32 BIT on 32 BIT
if (sizeof(morton) <= 4) {
return _BitScanReverse(firstbit_location, x) != 0;
}
// 64 BIT on 32 BIT
else {
*firstbit_location = 0;
if (_BitScanReverse(firstbit_location, (x >> 32))) { // check first part
firstbit_location += 32;
return true;
}
return _BitScanReverse(firstbit_location, (x & 0xFFFFFFFF)) != 0;
}
#elif _MSC_VER && _WIN64
....
#elif __GNUC__
....
#endif
}В своем редактировании программист заменяет выражение «firstbit_location += 32» на «*firstbit_location += 32». Программист ожидал, что 32 будет добавлено к значению переменной, на которую ссылается указатель firstbit_location, но 32 было добавлено к самому указателю. Измененное значение указателя больше нигде не использовалось, а ожидаемое значение переменной оставалось неизменным.
PVS-Studio выдаст предупреждение на этот код:
V1001 Переменная firstbit_location назначена, но не используется к концу функции. morton_common.h 22
Ну и что плохого в модифицированном, но в дальнейшем неиспользуемом выражении? Диагностика V1001 не похожа на то, что она предназначена для обнаружения особо опасных ошибок. Несмотря на это, была обнаружена важная ошибка, которая повлияла на логику программы.
Более того, оказалось, что найти эту ошибку не так-то просто! Он не только находится в программе с момента создания файла, но и пережил множество правок в соседних строках и просуществовал в проекте целых 3 (!) года! Все это время логика программы была нарушена, и она работала не так, как ожидали разработчики. Если бы они использовали PVS-Studio, баг был бы обнаружен гораздо раньше.
В заключение давайте посмотрим на еще один хороший пример. Пока собирал багфиксы на GitHub, несколько раз натыкался на фикс с следующим содержанием. Исправленная ошибка была здесь:
int kvm_arch_prepare_memory_region(...)
{
...
do {
struct vm_area_struct *vma = find_vma(current->mm, hva);
hva_t vm_start, vm_end;
...
if (vma->vm_flags & VM_PFNMAP) {
...
phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) +
vm_start - vma->vm_start;
...
}
...
} while (hva < reg_end);
...
}PVS-Studio выдал предупреждение на этот фрагмент кода:
V629 Попробуйте проверить выражение vma-›vm_pgoff ‹‹ 12. Битовый сдвиг 32-битного значения с последующим расширением до 64-битного типа. мму.к 1795
Я проверил объявления переменных, используемых в выражении «phys_addr_t pa = (vma-›vm_pgoff ‹‹ PAGE_SHIFT) + vm_start — vma-›vm_start;» и обнаружил, что приведенный выше код эквивалентен к следующему синтетическому примеру:
void foo(unsigned long a, unsigned long b)
{
unsigned long long x = (a << 12) + b;
}Если значение 32-битной переменной a больше 0xFFFFF, 12 старших битов будут иметь по крайней мере одно ненулевое значение. После сдвига этой переменной влево эти значащие биты будут потеряны, что приведет к записи неверной информации в x.
Чтобы исключить потерю старших бит, нам нужно сначала привести a к типу unsigned long long и только после этого сдвинуть переменную:
pa = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT;
pa += vm_start - vma->vm_start;Таким образом, правильное значение всегда будет записано в pa.
Все бы ничего, но этот баг, как и первый пример из статьи, тоже оказался критическим. Об этом автор написал в комментарии. Более того, эта ошибка нашла свое отражение в огромном количестве проектов. Чтобы в полной мере оценить масштаб трагедии, предлагаю посмотреть на количество результатов при поиске этого исправления на GitHub. Страшно, не так ли?
Поэтому я применил новый подход, чтобы продемонстрировать преимущества обычного использования статического анализатора кода. Я надеюсь, что вам понравилось. Скачай и попробуй статический анализатор кода PVS-Studio для проверки собственных проектов. На момент написания в ней реализовано около 700 диагностических правил для обнаружения различных шаблонов ошибок. Поддерживает C, C++, C# и Java.
