Хабрахабр

Ошибки, которые не находит статический анализ кода, потому, что он не используется

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

Идея очень проста. Поищем на GitHub примеры pull requests, в комментариях к которым указано, что это исправление ошибки. И попробуем найти эти ошибки с помощью статического анализатора кода PVS-Studio. Если исправленная ошибка находится анализатором, то это означает, что её можно было бы исправить ещё на этапе написания кода. А чем раньше ошибка исправляется, тем дешевле это обходится.

В самом GitHub тоже есть глюк (или фича), которая не позволяет искать комментарии в pull requests в проектах, написанных только на определённых языках программирования. К сожалению, GitHub подкачал и не позволил сделать большую шикарную статью на эту тему. Независимо от того, что я указываю искать комментарии в проектах на языке C++, C# или Java, выдаются результаты по всем языкам, включая PHP, Python, JavaScript и так далее. Ну или я не «умею его готовить». Тем не менее, их достаточно, чтобы продемонстрировать полезность инструментов статического анализа кода при его регулярном использовании. В результате искать подходящие случаи оказалось крайне утомительным занятием, и я ограничусь только несколькими примерами.

Ответ несложен: программистам не понадобилось бы ждать её проявления, а затем искать и исправлять дефектный код.
Посмотрим на ошибки, которые мог бы сразу обнаружить PVS-Studio: Что, если ошибка была бы отловлена на ранней стадии?

До исправления ошибки код выглядел так: Первый пример взят из проекта SatisfactoryModLoader.

// gets an API function from the mod handler
SML_API PVOID getAPIFunction(std::string name) } if (!found) { std::string msg = ...; MessageBoxA(NULL, msg.c_str(), "SatisfactoryModLoader Fatal Error", MB_ICONERROR); abort(); }
}

Этот код содержал ошибку, на которую PVS-Studio сразу выдал бы предупреждение:

ModFunctions.cpp 44 V591 Non-void function should return a value.

Программист не пользовался анализатором кода, поэтому ему пришлось самостоятельно искать ошибку. Приведенная выше функция не имеет 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;
}

Любопытно, что в коммите автор указал баг как критический: "fixed critical bug where API functions were not returned".

Во втором коммите из истории проекта 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;

В старой версии кода не производилось никакой проверки cpu на равенство нулевому указателю. Если вдруг окажется, что в качестве второго аргумента в функцию mc6809dis_direct будет передан нулевой указатель, то в теле функции произойдет его разыменование. Результат плачевен и непредсказуем.

Ну живет она в коде и живет, а если разыменование случится — программа спокойно упадёт и всё». Разыменование нулевого указателя — один из самых частых паттернов, по поводу которых нам говорят: «Это не критическая ошибка. Странно и грустно слышать такое от C++ программистов, но жизнь есть жизнь.

В любом случае, в данном проекте такое разыменование всё-таки превратилось в баг, о чем нам говорит заголовок коммита: "Bug fix---NULL dereference".

Если бы разработчик проекта пользовался PVS-Studio, он бы еще два с половиной месяца назад (именно столько прошло с момента внесения ошибки) мог проверить свой код и обнаружить предупреждение:

Check lines: 1814, 1821. V595 The 'cpu' pointer was utilized before it was verified against nullptr. 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, но оно прибавлялось непосредственно к указателю. Измененное значение указателя больше нигде не использовалось, а ожидаемое значение переменной так и оставалось неизмененным.

PVS-Studio выдал бы на этот код предупреждение:

morton_common.h 22 V1001 The 'firstbit_location' variable is assigned but is not used by the end of the function.

Диагностика 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 выдал предупреждение:

Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. V629 Consider inspecting the 'vma->vm_pgoff << 12' expression. mmu.c 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.

Надеюсь, вам понравилось. Итак, я применил новый подход, чтобы продемонстрировать пользу регулярного использования статического анализатора кода. На момент написания статьи в нём реализовано около 700 диагностических правил для обнаружения разнообразнейших паттернов ошибок. Скачайте и попробуйте статический анализатор кода PVS-Studio для проверки собственных проектов. Поддерживаются языки C, C++, C# и Java.

Errors that static code analysis does not find because it is not used Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov.

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть